Hi Hans, On 26/08/15 18:31, Julien Charbon wrote: > 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.
After checking the non-mpsafe callout cases, you are completely right, this test makes sense only in mpsafe callout case. Fixed in r287196: https://svnweb.freebsd.org/base?view=revision&revision=287196 Thanks a lot for your time. -- Julien
signature.asc
Description: OpenPGP digital signature