Date: Tue, 6 Apr 2004 16:32:16 -0700 (PDT) From: Nate Lawson <nate@root.org> To: Colin Percival <cperciva@FreeBSD.org> Cc: cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/kern kern_timeout.c src/sys/sys callout.h src/share/man/man9 timeout.9 Message-ID: <20040406162703.H30263@root.org> In-Reply-To: <20040406230958.C01C616A545@hub.freebsd.org> References: <20040406230958.C01C616A545@hub.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040406162703.H30263>