On Wed, Dec 16, 2020 at 04:50:42PM -0300, Martin Pieuchot wrote: > On 16/12/20(Wed) 12:50, Scott Cheloha wrote: > > 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"? > > > > Something like the attached patch? > > > > I think the idea of a "dead channel" communicates the intent. Nobody > > broadcasts wakeups on the dead channel. If you don't want to receive > > wakeup broadcasts you sleep on the dead channel. Hence, "deadchan". > > Why did we choose to use a variable over NULL? Any technical reason?
The sleep subsytem requires a non-NULL value for ident. Changing this seems not trivial. > I'm wondering it the locality of the variable might not matter in a > distant future. Did you dig a bit deeper about the FreeBSD solution? > Why did they choose a per-CPU value? Currently all sleep channels are hashed into IIRC 128 buckets. If all timeouts use the same sleep channel then this queue may get overcrowded. I guess only instrumentation and measurements will tell us how bad the sleep queue is hashed. > > Index: kern/kern_synch.c > > =================================================================== > > RCS file: /cvs/src/sys/kern/kern_synch.c,v > > retrieving revision 1.172 > > diff -u -p -r1.172 kern_synch.c > > --- kern/kern_synch.c 7 Dec 2020 16:55:29 -0000 1.172 > > +++ kern/kern_synch.c 16 Dec 2020 18:50:12 -0000 > > @@ -87,6 +87,12 @@ sleep_queue_init(void) > > TAILQ_INIT(&slpque[i]); > > } > > > > +/* > > + * Threads that do not want to receive wakeup(9) broadcasts should > > + * sleep on deadchan. > > + */ > > +static int __deadchan; > > +int *deadchan = &__deadchan; > > > > /* > > * During autoconfiguration or after a panic, a sleep will simply > > Index: sys/systm.h > > =================================================================== > > RCS file: /cvs/src/sys/sys/systm.h,v > > retrieving revision 1.148 > > diff -u -p -r1.148 systm.h > > --- sys/systm.h 26 Aug 2020 03:29:07 -0000 1.148 > > +++ sys/systm.h 16 Dec 2020 18:50:12 -0000 > > @@ -107,6 +107,8 @@ extern struct vnode *rootvp; /* vnode eq > > extern dev_t swapdev; /* swapping device */ > > extern struct vnode *swapdev_vp;/* vnode equivalent to above */ > > > > +extern int *deadchan; /* dead wakeup(9) channel */ > > + > > struct proc; > > struct process; > > #define curproc curcpu()->ci_curproc > > Index: dev/sdmmc/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 > > --- dev/sdmmc/sdmmc_io.c 31 Dec 2019 10:05:33 -0000 1.41 > > +++ dev/sdmmc/sdmmc_io.c 16 Dec 2020 18:50:12 -0000 > > @@ -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(deadchan, PPAUSE, "pause", SEC_TO_NSEC(1)); > > return (retry >= 0) ? 0 : ETIMEDOUT; > > } > > > > -- :wq Claudio