svn commit: r286880 - head/sys/kern

[Julien Charbon]

 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


-------------- 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-head/attachments/20150826/7dbce573/attachment.bin>