cvs commit: src/sys/kern kern_timeout.c src/sys/sys callout.h src/share/man/man9 timeout.9

[Nate Lawson]

On Tue, 6 Apr 2004, Colin Percival wrote:
>   FreeBSD src repository
>
>   Modified files:
>     sys/kern             kern_timeout.c
>     sys/sys              callout.h
>     share/man/man9       timeout.9
>   Log:
>   Introduce a callout_drain() function.  This acts in the same manner as
>   callout_stop(), except that if the callout being stopped is currently
>   in progress, it blocks attempts to reset the callout and waits until the
>   callout is completed before it returns.
>
>   This makes it possible to clean up callout-using code safely, e.g.,
>   without potentially freeing memory which is still being used by a callout.
>
>   Reviewed by:    mux, gallatin, rwatson, jhb
>
>   Revision  Changes    Path
>   1.21      +18 -11    src/share/man/man9/timeout.9
>   1.86      +90 -1     src/sys/kern/kern_timeout.c
>   1.25      +3 -0      src/sys/sys/callout.h

Useful, but a few issues.

> @@ -71,6 +72,32 @@
>  #endif
>
>  static struct callout *nextsoftcheck;	/* Next callout to be checked. */
> +/*
> + * Locked by callout_lock:
> + *   curr_callout    - If a callout is in progress, it is curr_callout.

Style gives a blank line before each comment block.

> +static int wakeup_ctr;
> +/*
> + * Locked by callout_wait_lock:

Same here.

> @@ -241,6 +276,21 @@
>  				if (!(c_flags & CALLOUT_MPSAFE))
>  					mtx_unlock(&Giant);
>  				mtx_lock_spin(&callout_lock);
> +				curr_callout = NULL;
> +				if (wakeup_needed) {
> +					/*
> +					 * There might be someone waiting
> +					 * for the callout to complete.
> +					 */

For this one, you can move the comment to above the "if" statement and add
a blank line before it.  It's usually best to comment on the whole block
above the if statement rather than within it.

> @@ -344,6 +394,17 @@
>  {
>
>  	mtx_lock_spin(&callout_lock);
> +
> +	if (c == curr_callout && wakeup_needed) {
> +		/*
> +		 * We're being asked to reschedule a callout which is
> +		 * currently in progress, and someone has called
> +		 * callout_drain to kill that callout.  Don't reschedule.
> +		 */
> +		mtx_unlock_spin(&callout_lock);

Same here.

> +		if (c == curr_callout && safe) {
> +			/* We need to wait until the callout is finished */
> +			wakeup_needed = 1;

Same here.

> +			mtx_lock(&callout_wait_lock);
> +			/*
> +			 * Check to make sure that softclock() didn't
> +			 * do the wakeup in between our dropping
> +			 * callout_lock and picking up callout_wait_lock
> +			 */
> +			if (wakeup_cookie - wakeup_done_ctr > 0)

Preceding blank line.

> --- src/sys/sys/callout.h:1.24	Tue Mar 19 12:18:36 2002
> +++ src/sys/sys/callout.h	Tue Apr  6 16:08:48 2004
> @@ -81,6 +81,9 @@
>  #define	callout_pending(c)	((c)->c_flags & CALLOUT_PENDING)
>  void	callout_reset(struct callout *, int, void (*)(void *), void *);
>  int	callout_stop(struct callout *);
> +#define	callout_stop(c)		_callout_stop_safe(c, 0)
> +#define	callout_drain(c)	_callout_stop_safe(c, 1)
> +int	_callout_stop_safe(struct callout *, int);
>
>  #endif

The goal here is to keep binary compatibility (multiple defines of
callout_stop)?  I'm not sure if this will work on all compilers.  Are you
going to remove that shim at some point?  Perhaps a BURN_BRIDGES or
GONE_IN_6 ifdef would be appropriate for that.

-Nate