Hi Iain,

On Fri, Jan 19, 2007 at 10:16:11AM +0000, Iain Hibbert wrote:

> Here is my implementation, seems stable. Points to note:
> 
> - callout locking is changed to use splclock() as per documentation
> 
> - uses callout_setflag(c, f) rather than extend callout_init()
> - deprecates CALLOUT_INITIALIZER_SETFUNC

I don't like the idea of callout_setflag(), but until the guts of "struct
callout" are opaque to the consumers then it can't really be claimed to be
a stable API, so no argument from me.

>  #define	CALLOUT_LOCK(s)							\
>  do {									\
> -	s = splsched();							\
> +	s = splclock();							\
>  	simple_lock(&callout_slock);					\
>  } while (/*CONSTCOND*/0)

I'm not sure about this change. splsched() seems more appropriate to me as
it's a symbolic level.

> +callout_thread(void *arg)
...
> +		/*
> +		 * loop here forever, snoozing when there is no work
> +		 */
> +		if (CIRCQ_EMPTY(q)) {
> +			error = ltsleep(q, PUSER - 1, "callout", 0,
> +							    &callout_slock);
> +			if (error)
> +				panic("%s: ltsleep error=%d", __func__, error);
> +		} else {
> +			c = CIRCQ_FIRST(q);
> +			CIRCQ_REMOVE(&c->c_list);
> +
> +			c->c_flags &= ~CALLOUT_PENDING;
> +			c->c_flags |= (CALLOUT_FIRED|CALLOUT_INVOKING);
> +
> +			func = c->c_func;
> +			arg = c->c_arg;
> +
> +			CALLOUT_UNLOCK(s);
> +			(*func)(arg);
> +			CALLOUT_LOCK(s);
> +		}
> +	}

There's no real need to check for an error from ltsleep() here. KNFed it
would be:

			error = ltsleep(q, PUSER - 1, "callout", 0,
			    &callout_slock);

Actually, thinking about it this could lead to deadlock. Since the lock
order is sched_lock -> callout_slock, entering tsleep() at this point
would acquire sched_lock while we already hold callout_slock.

> softclock()
...
> -			c->c_flags = (c->c_flags  & ~CALLOUT_PENDING) |
> -			    (CALLOUT_FIRED|CALLOUT_INVOKING);
> -
> -			func = c->c_func;
> -			arg = c->c_arg;
> -
> -			CALLOUT_UNLOCK(s);
> -			(*func)(arg);
> -			CALLOUT_LOCK(s);
> +			if (c->c_flags & CALLOUT_THREAD) {
> +				CIRCQ_INSERT(&c->c_list, &timeout_work);
> +				wakeup(&timeout_work);
> +			} else {

There is the same lock order reversal here - callout_slock needs to be
dropped before calling wakeup().

Thanks,
Andrew