Date: Wed, 26 Aug 2015 10:12:27 +0200 From: Hans Petter Selasky <hp@selasky.org> To: Julien Charbon <jch@FreeBSD.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org, John Baldwin <jhb@freebsd.org> Subject: Re: svn commit: r286880 - head/sys/kern Message-ID: <55DD74EB.30601@selasky.org> In-Reply-To: <55DD69E5.4090904@selasky.org> References: <201508181015.t7IAFAex055889@repo.freebsd.org> <55DD69E5.4090904@selasky.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 08/26/15 09:25, Hans Petter Selasky wrote: > On 08/18/15 12:15, Julien Charbon wrote: >> Author: jch >> Date: Tue Aug 18 10:15:09 2015 >> New Revision: 286880 >> URL: https://svnweb.freebsd.org/changeset/base/286880 >> >> Log: >> callout_stop() should return 0 (fail) when the callout is currently >> being serviced and indeed unstoppable. >> >> A scenario to reproduce this case is: >> >> - the callout is being serviced and at same time, >> - callout_reset() is called on this callout that sets >> the CALLOUT_PENDING flag and at same time, >> - callout_stop() is called on this callout and returns 1 (success) >> even if the callout is indeed currently running and unstoppable. >> >> This issue was caught up while making r284245 (D2763) workaround, and >> was discussed at BSDCan 2015. Once applied the r284245 workaround >> is not needed anymore and will be reverted. >> >> Differential Revision: https://reviews.freebsd.org/D3078 >> Reviewed by: jhb >> Sponsored by: Verisign, Inc. >> >> Modified: >> head/sys/kern/kern_timeout.c >> >> Modified: head/sys/kern/kern_timeout.c >> ============================================================================== >> >> --- head/sys/kern/kern_timeout.c Tue Aug 18 10:07:03 2015 (r286879) >> +++ head/sys/kern/kern_timeout.c Tue Aug 18 10:15:09 2015 (r286880) >> @@ -1150,7 +1150,7 @@ _callout_stop_safe(struct callout *c, in >> struct callout_cpu *cc, *old_cc; >> struct lock_class *class; >> int direct, sq_locked, use_lock; >> - int not_on_a_list; >> + int not_on_a_list, not_running; >> >> if (safe) >> WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, c->c_lock, >> @@ -1378,8 +1378,15 @@ again: >> } >> } >> callout_cc_del(c, cc); >> + >> + /* >> + * If we are asked to stop a callout which is currently in progress >> + * and indeed impossible to stop then return 0. >> + */ >> + not_running = !(cc_exec_curr(cc, direct) == c); >> + >> CC_UNLOCK(cc); >> - return (1); >> + return (not_running); >> } >> >> void >> >> > > Hi, > > I think this patch is incomplete and can break the return value for > non-MPSAFE callouts. I think the correct statement should check the > value of "use_lock" too: > > not_running = !(cc_exec_curr(cc, direct) == c && use_lock == 0); > > Because if the callback process waits for lock "c_lock" in the callback > process then we have above "cc_exec_curr(cc, direct) == c" is satisfied > too, and non-MPSAFE callouts are always cancelable, via > cc_exec_cancel(cc, direct) = true; > >> class->lc_lock(c_lock, lock_status); >> /* >> * The callout may have been cancelled >> * while we switched locks. >> */ >> if (cc_exec_cancel(cc, direct)) { >> class->lc_unlock(c_lock); >> goto skip; >> } >> /* The callout cannot be stopped now. */ >> cc_exec_cancel(cc, direct) = true; > > --HPS > Hi, I'm sorry to say, but I think this change should be backed out as it changes the behaviour of the callout API. The old code was correct. The issues should be fixed in the TCP stack instead, like suggested by D1563 and also r284245. This patch introduces new behaviour to the callout_stop() function, which is contradicting "man 9 callout" and _not_ documented anywhere! Reading "man 9 callout" in 11-current: > The callout_reset() and callout_schedule() function families schedule a > future function invocation for callout c. If c already has a pending > callout, it is cancelled before the new invocation is scheduled. callout_reset() causes a _new_ callout invocation and it is logical that the return value of callout_stop() reflect operations on the _new_ callout invocation and not the previous one. > The function callout_stop() cancels a callout c if it is currently pend- > ing. If the callout is pending, then callout_stop() returns a non-zero > value. If the callout is not set, has already been serviced, or is cur- > rently being serviced, then zero will be returned. If the callout has an > associated lock, then that lock must be held when this function is > called. If there are two callout invocations that callout_stop() needs to handle, it should be called twice. The correct way is: callout_reset(); callout_stop(); callout_drain(); For the TCP stack's matter, it should be: callout_reset(); callout_stop(); /* cancel _new_ callout invocation */ callout_stop(); /* check for any pending callout invokation */ Remember there are other OS'es out there using our TCP/IP stack which do not have an identical callout subsystem. --HPS
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?55DD74EB.30601>