Date: Tue, 3 Jul 2007 00:58:07 +0200 From: "Attilio Rao" <attilio@freebsd.org> To: "Jeff Roberson" <jroberson@chesapeake.net> Cc: arch@freebsd.org Subject: Re: wakeup_flags patch. Message-ID: <3bbf2fe10707021558i413eed3cv5c5f38a820e9d249@mail.gmail.com> In-Reply-To: <20070701160540.Y552@10.0.0.1> References: <20070701160540.Y552@10.0.0.1>
next in thread | previous in thread | raw e-mail | index | archive | help
2007/7/2, Jeff Roberson <jroberson@chesapeake.net>: > http://people.freebsd.org/~jeff/wakeupflags.diff > > It didn't workout very cleanly since the flags have to go through three > layers. I could define wakeup and sleepq flags to be the same and skip a > bunch of conditionals. However, we'd then have to know which flags were > free to use in each case. Are there any further opinions on the style? Honestly, IMHO we should not depend by precise values of flags passed between layers. Your code is clean enough to not require those magics too. Specific to the patch: > +void > +wakeup_flags(void *ident, int flags) > +{ > + int sqflag; > + > + sqflag = SLEEPQ_SLEEP; > + if (flags & WAKEUP_LOCAL) > + sqflag |= SLEEPQ_LOCAL; > + if (flags & WAKEUP_TAIL) > + sqflag |= SLEEPQ_TAIL; > + sleepq_lock(ident); > + if (flags & WAKEUP_ONE) { > + sleepq_signal(ident, sqflag, -1, 0); > + sleepq_release(ident); > + } else if (flags & WAKEUP_ALL) > + sleepq_broadcast(ident, sqflag, -1, 0); > + else > + panic("wakeup_flags: Bad flags %d\n", flags); > +} > + I would arrange this a little bit differently in order to make it homogeneous with other locking primitives: void wakeup_flags(void *ident, int flags) { int sqflag; MPASS(flags & (WAKEUP_ONE | WAKEUP_ALL)); ... if (flags & WAKEUP_ONE) { sleepq_signal(ident, sqflag, -1, 0); sleepq_release(ident); } else sleepq_broadcast(ident, sqflag, -1, 0); } > @@ -943,7 +943,7 @@ > resetpriority(td); > } > td->td_slptime = 0; > - sched_add(td, SRQ_BORING); > + sched_add(td, SRQ_BORING|flags); > } This doesn't imply a style violation? (There are several of this). It should be: sched_add(td, SRQ_BORING | flags); > @@ -716,13 +718,17 @@ > * the tail of sleep queues. > */ > besttd = NULL; > - TAILQ_FOREACH(td, &sq->sq_blocked[queue], td_slpq) { > - if (besttd == NULL || td->td_priority < besttd->td_priority) > - besttd = td; > - } > + if ((flags & SLEEPQ_TAIL) == 0) { > + TAILQ_FOREACH(td, &sq->sq_blocked[queue], td_slpq) { > + if (besttd == NULL || > + td->td_priority < besttd->td_priority) > + besttd = td; > + } > + } else All those brackets here are not really necessary, IMHO. > -void setrunnable(struct thread *); > +void setrunnable(struct thread *, int); Wouldn't be better to have style(9) matching prototypes (with named arguments)? > @@ -88,6 +88,8 @@ > #define SLEEPQ_PAUSE 0x02 /* Used by pause. */ > #define SLEEPQ_SX 0x03 /* Used by an sx lock. */ > #define SLEEPQ_INTERRUPTIBLE 0x100 /* Sleep is interruptible. */ > +#define SLEEPQ_LOCAL 0x200 /* Wake-up on the local cpu. */ > +#define SLEEPQ_TAIL 0x400 /* Wake-up from the tail. */ This is not related to your patch, but this let me think that we should have an effective mask of 0xFF for filtering between consumers and flags (in particular when consumers are checked). However, actually sleepqueues consumers identifier isn't basically used. However, I'd like to see this ported for condvar since I strongly hope one day msleep/wakeup will be deprecated in favor of condvar. Thanks, Attilio -- Peace can only be achieved by understanding - A. Einstein
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3bbf2fe10707021558i413eed3cv5c5f38a820e9d249>