From owner-cvs-src@FreeBSD.ORG Tue Apr 6 16:40:12 2004 Return-Path: Delivered-To: cvs-src@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id C36AB16A4CF for ; Tue, 6 Apr 2004 16:40:12 -0700 (PDT) Received: from root.org (root.org [67.118.192.226]) by mx1.FreeBSD.org (Postfix) with SMTP id A4B6143D41 for ; Tue, 6 Apr 2004 16:40:12 -0700 (PDT) (envelope-from nate@root.org) Received: (qmail 30270 invoked by uid 1000); 6 Apr 2004 23:32:16 -0000 Date: Tue, 6 Apr 2004 16:32:16 -0700 (PDT) From: Nate Lawson To: Colin Percival In-Reply-To: <20040406230958.C01C616A545@hub.freebsd.org> Message-ID: <20040406162703.H30263@root.org> References: <20040406230958.C01C616A545@hub.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII cc: cvs-src@FreeBSD.org cc: src-committers@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 X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 06 Apr 2004 23:40:13 -0000 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