svn commit: r286880 - head/sys/kern
Hans Petter Selasky
hps at selasky.org
Wed Aug 26 07:24:00 UTC 2015
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
More information about the svn-src-all
mailing list