On 6/10/2025 5:44 AM, Ian Ray wrote:
> On Mon, Jun 09, 2025 at 04:10:39PM -0700, Jakub Kicinski wrote:
>> On Mon, 9 Jun 2025 09:32:58 +0300 Ian Ray wrote:
>>> On Thu, Jun 05, 2025 at 06:43:39PM -0700, Jakub Kicinski wrote:
>>>> On Tue, 3 Jun 2025 11:09:49 +0300 Ian Ray wrote:
>>>>> set_bit(__IGB_DOWN, &adapter->state);
>>>>> + timer_delete_sync(&adapter->watchdog_timer);
>>>>> + timer_delete_sync(&adapter->phy_info_timer);
>>>>> +
>>>>> + cancel_work_sync(&adapter->watchdog_task);
>>>>
>>>> This doesn't look very race-proof as watchdog_task
>>>> can schedule the timer as its last operation?
>>>
>>> Thanks for the reply. __IGB_DOWN is the key to this design.
>>>
>>> If watchdog_task runs *before* __IGB_DOWN is set, then the
>>> timer is stopped (by this patch) as required.
>>>
>>> However, if watchdog_task runs *after* __IGB_DOWN is set,
>>> then the timer will not even be started (by watchdog_task).
>>
>> Well, yes, but what if the two functions run *simultaneously*
>> There is no mutual exclusion between these two pieces of code AFAICT
>
> Thank you for clarifying.
>
> IIUC set_bit() is an atomic operation (via bitops.h), and so
> my previous comment still stands.
>
> (Sorry if I have misunderstood your question.)
>
> Either watchdog_task runs just before __IGB_DOWN is set (and
> the timer is stopped by this patch) -- or watchdog_task runs
> just after __IGB_DOWN is set (and thus the timer will not be
> restarted).
>
> In both cases, the final cancel_work_sync ensures that the
> watchdog_task completes before igb_down() continues.
>
> Regards,
> Ian
Hmm. Well set_bit is atomic, but I don't think it has ordering
guarantees on its own. Wouldn't we need to be using a barrier here to
guarantee ordering here?
Perhaps cancel_work_sync has barriers implied and that makes this work
properly?
> ORDERING
> --------
>
> Like with atomic_t, the rule of thumb is:
>
> - non-RMW operations are unordered;
>
> - RMW operations that have no return value are unordered;
>
> - RMW operations that have a return value are fully ordered.
>
> - RMW operations that are conditional are fully ordered.
>
> Except for a successful test_and_set_bit_lock() which has ACQUIRE semantics,
> clear_bit_unlock() which has RELEASE semantics and test_bit_acquire which has
> ACQUIRE semantics.
>
set_bit is listed as a RMW without a return value, so its unordered.
That makes me think we'd want clear_bit_unlock() if the cancel_work_sync
itself doesn't provide the barriers we need.
Thanks,
Jake
