Date: Thu, 3 Mar 2016 11:59:36 +0100 From: Hans Petter Selasky <hps@selasky.org> To: Konstantin Belousov <kib@FreeBSD.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r296320 - in head/sys: kern sys Message-ID: <56D81918.6020403@selasky.org> In-Reply-To: <201603021846.u22IkHWM010861@repo.freebsd.org> References: <201603021846.u22IkHWM010861@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 03/02/16 19:46, Konstantin Belousov wrote: > Author: kib > Date: Wed Mar 2 18:46:17 2016 > New Revision: 296320 > URL: https://svnweb.freebsd.org/changeset/base/296320 > > Log: > If callout_stop_safe() noted that the callout is currently executing, > but next invocation is cancelled while migrating, > sleepq_check_timeout() needs to be informed that the callout is > stopped. Otherwise the thread switches off CPU and never become > runnable, since running callout could have already raced with us, > while the migrating and cancelled callout could be one which is > expected to set TDP_TIMOFAIL flag for us. This contradicts with the > expected behaviour of callout_stop() for other callers, which > e.g. decrement references from the callout callbacks. > > Add a new flag CS_MIGRBLOCK requesting report of the situation as > 'successfully stopped'. > > Reviewed by: jhb (previous version) > Tested by: cognet, pho > PR: 200992 > Sponsored by: The FreeBSD Foundation > MFC after: 2 weeks > Differential revision: https://reviews.freebsd.org/D5221 > > Modified: > head/sys/kern/kern_timeout.c > head/sys/kern/subr_sleepqueue.c > head/sys/sys/callout.h > > Modified: head/sys/kern/kern_timeout.c > ============================================================================== > --- head/sys/kern/kern_timeout.c Wed Mar 2 16:36:24 2016 (r296319) > +++ head/sys/kern/kern_timeout.c Wed Mar 2 18:46:17 2016 (r296320) > @@ -1155,14 +1155,14 @@ callout_schedule(struct callout *c, int > } > > int > -_callout_stop_safe(struct callout *c, int safe, void (*drain)(void *)) > +_callout_stop_safe(struct callout *c, int flags, void (*drain)(void *)) > { > struct callout_cpu *cc, *old_cc; > struct lock_class *class; > int direct, sq_locked, use_lock; > int not_on_a_list; > > - if (safe) > + if ((flags & CS_DRAIN) != 0) > WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, c->c_lock, > "calling %s", __func__); > > @@ -1170,7 +1170,7 @@ _callout_stop_safe(struct callout *c, in > * Some old subsystems don't hold Giant while running a callout_stop(), > * so just discard this check for the moment. > */ > - if (!safe && c->c_lock != NULL) { > + if ((flags & CS_DRAIN) == 0 && c->c_lock != NULL) { > if (c->c_lock == &Giant.lock_object) > use_lock = mtx_owned(&Giant); > else { > @@ -1253,7 +1253,7 @@ again: > return (-1); > } > > - if (safe) { > + if ((flags & CS_DRAIN) != 0) { > /* > * The current callout is running (or just > * about to run) and blocking is allowed, so > @@ -1370,7 +1370,7 @@ again: > cc_exec_drain(cc, direct) = drain; > } > CC_UNLOCK(cc); > - return (0); > + return ((flags & CS_MIGRBLOCK) != 0); > } > CTR3(KTR_CALLOUT, "failed to stop %p func %p arg %p", > c, c->c_func, c->c_arg); > > Modified: head/sys/kern/subr_sleepqueue.c > ============================================================================== > --- head/sys/kern/subr_sleepqueue.c Wed Mar 2 16:36:24 2016 (r296319) > +++ head/sys/kern/subr_sleepqueue.c Wed Mar 2 18:46:17 2016 (r296320) > @@ -586,7 +586,8 @@ sleepq_check_timeout(void) > * another CPU, so synchronize with it to avoid having it > * accidentally wake up a subsequent sleep. > */ > - else if (callout_stop(&td->td_slpcallout) == 0) { > + else if (_callout_stop_safe(&td->td_slpcallout, CS_MIGRBLOCK, NULL) > + == 0) { > td->td_flags |= TDF_TIMEOUT; > TD_SET_SLEEPING(td); > mi_switch(SW_INVOL | SWT_SLEEPQTIMO, NULL); > > Modified: head/sys/sys/callout.h > ============================================================================== > --- head/sys/sys/callout.h Wed Mar 2 16:36:24 2016 (r296319) > +++ head/sys/sys/callout.h Wed Mar 2 18:46:17 2016 (r296320) > @@ -62,6 +62,12 @@ struct callout_handle { > struct callout *callout; > }; > > +/* Flags for callout_stop_safe() */ > +#define CS_DRAIN 0x0001 /* callout_drain(), wait allowed */ > +#define CS_MIGRBLOCK 0x0002 /* Block migration, return value > + indicates that the callout was > + executing */ > + > #ifdef _KERNEL > /* > * Note the flags field is actually *two* fields. The c_flags > @@ -81,7 +87,7 @@ struct callout_handle { > */ > #define callout_active(c) ((c)->c_flags & CALLOUT_ACTIVE) > #define callout_deactivate(c) ((c)->c_flags &= ~CALLOUT_ACTIVE) > -#define callout_drain(c) _callout_stop_safe(c, 1, NULL) > +#define callout_drain(c) _callout_stop_safe(c, CS_DRAIN, NULL) > void callout_init(struct callout *, int); > void _callout_init_lock(struct callout *, struct lock_object *, int); > #define callout_init_mtx(c, mtx, flags) \ > > Hi, I believe all of these callout quirks can be avoided by using a spinlock to proctect the thread callout like done in projects/hps_head. Has anyone tried to reproduce the issue with projects/hps_head, before making this patch? BTW: The FreeBSD kernel version should have been bumped because _callout_stop_safe() is a public callout API, used by all external kernel modules, which now will have undefined behaviour until recompiled. --HPS
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?56D81918.6020403>