wakeup_flags patch.

[Attilio Rao]

2007/7/2, Jeff Roberson <jroberson at 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