On Tue, Dec 15, 2020 at 01:47:24PM +0100, Mark Kettenis wrote:
> > Date: Tue, 15 Dec 2020 13:32:22 +0100
> > From: Claudio Jeker <cje...@diehard.n-r-g.com>
> >
> > On Fri, Dec 11, 2020 at 07:07:56PM -0600, Scott Cheloha wrote:
> > > Hi,
> > >
> > > I'd like to remove lbolt from the kernel. I think having it in the
> > > kernel complicates otherwise simple code.
> > >
> > > We can start with sdmmc(4).
> > >
> > > The goal in sdmmc_io_function_enable() is calling
> > > sdmmc_io_function_ready()
> > > up to six times and sleep 1 second between each attempt. Here's rewritten
> > > code that does with without lbolt.
> > >
> > > ok?
> > >
> > > Index: sdmmc_io.c
> > > ===================================================================
> > > RCS file: /cvs/src/sys/dev/sdmmc/sdmmc_io.c,v
> > > retrieving revision 1.41
> > > diff -u -p -r1.41 sdmmc_io.c
> > > --- sdmmc_io.c 31 Dec 2019 10:05:33 -0000 1.41
> > > +++ sdmmc_io.c 12 Dec 2020 01:04:59 -0000
> > > @@ -231,8 +231,8 @@ sdmmc_io_function_enable(struct sdmmc_fu
> > > {
> > > struct sdmmc_softc *sc = sf->sc;
> > > struct sdmmc_function *sf0 = sc->sc_fn0;
> > > + int chan, retry = 5;
> > > u_int8_t rv;
> > > - int retry = 5;
> > >
> > > rw_assert_wrlock(&sc->sc_lock);
> > >
> > > @@ -244,7 +244,7 @@ sdmmc_io_function_enable(struct sdmmc_fu
> > > sdmmc_io_write_1(sf0, SD_IO_CCCR_FN_ENABLE, rv);
> > >
> > > while (!sdmmc_io_function_ready(sf) && retry-- > 0)
> > > - tsleep_nsec(&lbolt, PPAUSE, "pause", INFSLP);
> > > + tsleep_nsec(&chan, PPAUSE, "pause", SEC_TO_NSEC(1));
> > > return (retry >= 0) ? 0 : ETIMEDOUT;
> > > }
> > >
> >
> > Why not use &retry as wait channel instead of adding a new variable
> > chan? Result is the same. Would it make sense to allow NULL as wait
> > channel to make the tsleep not wakeable. At least that could be used in a
> > few places where timeouts are implemented with tsleep and would make the
> > intent more obvious.
>
> Or have an appropriately named global variable? Something like "int nowake"?
I like this. Brief aside into other BSDs:
--
FreeBSD and NetBSD call this operation a "pause" instead of a "sleep".
The idea is that a sleeping thread can be woken up with e.g.
wakeup(9) but that a paused thread cannot be awoken in this way.
Paused threads can still be interrupted with a signal.
NetBSD has kpause(9):
https://man.netbsd.org/kpause.9
FreeBSD has a whole bunch of pause interfaces:
https://www.freebsd.org/cgi/man.cgi?query=pause&sektion=9&manpath=FreeBSD+12.2-RELEASE+and+Ports
It kind-of sounds like what we want. From that page:
> The pause() function is a wrapper around tsleep() that suspends
> execution of the current thread for the indicated timeout. The
> thread can not be awakened early by signals or calls to wakeup(),
> wakeup_one() or wakeup_any(). The pause_sig() function is a variant
> of pause() which can > be awakened early by signals.
FreeBSD implements it with a special per-CPU pause channel.
Look at FreeBSD's _sleep():
https://github.com/freebsd/freebsd/blob/d551da60d42039156f003de6644e9e147ed167a3/sys/kern/kern_synch.c#L173
--
So with that in mind, my thought is to start with a global "int pause"
channel that we all collectively agree not to pass to wakeup(9). We
can advance the concept more if need be.
I'm happy to fuss with the name.
int pause_chan?
