Re: [Intel-wired-lan] [PATCH v2] igb: Fix watchdog_task race with shutdown

[Jacob Keller <jacob.e.keller@intel.com>]

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