svn commit: r286880 - head/sys/kern

[Julien Charbon]

 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 496 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freebsd.org/pipermail/svn-src-all/attachments/20150827/d84984bc/attachment.bin>