Date: Wed, 26 Aug 2015 18:31:59 +0200 From: Julien Charbon <jch@freebsd.org> To: Hans Petter Selasky <hps@selasky.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: <55DDE9FF.3020705@freebsd.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
[-- Attachment #1 --] Hi Hans, On 26/08/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 > > 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; Hum, interesting let me double check that. -- Julien [-- Attachment #2 --] -----BEGIN PGP SIGNATURE----- Comment: GPGTools - https://gpgtools.org iQEcBAEBCgAGBQJV3eoEAAoJEKVlQ5Je6dhxrnkH/3U43xxFECX8nd6l8PHlUhAF DwBvlKviCZ7fxnmUEEN8o1U/TC52E2hQYPVlONTmGkbEkOI7bppXOPeT8tcIJ38F u1uqwx1ArnG8T8Vne5puvB28Ij1SwyIarhkR4XUEAmYdp7IUrE5dIOhVeuVOTzSZ P3xoxrjMKfxaNtY+c7bDPuGbhzGFZiGK+ljsSltjxVKgPyXiKERyz3t51AYWJkMX npfn6iNLHkr3SePj03MzIL8e7aXtzCEiJSzENQDEdl+QlhuqYBv6TO1FGfMr/gP/ pAL2qoHpn84zuKsp9D+D4g7giW44Gd19RLx+DQxFLNSy3q9JMv1wr5cnc8Dq8ls= =cd2B -----END PGP SIGNATURE-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?55DDE9FF.3020705>