Message-ID: <83916c62-6069-4ed1-9856-b6c21363b137@linux.alibaba.com>
Date: Mon, 16 Jun 2025 09:54:36 +0800
From: Shuai Xue <xueshuai@...ux.alibaba.com>
To: Dave Jiang <dave.jiang@...el.com>,
 Vinicius Costa Gomes <vinicius.gomes@...el.com>, fenghuay@...dia.com,
 vkoul@...nel.org
Cc: dmaengine@...r.kernel.org, colin.i.king@...il.com,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/2] dmaengine: idxd: Fix race condition between WQ
 enable and reset paths



在 2025/6/6 22:32, Dave Jiang 写道:
> 
> 
> On 6/5/25 12:40 AM, Shuai Xue wrote:
>>
>>
>> 在 2025/6/4 22:19, Dave Jiang 写道:
>>>
>>>
>>> On 6/4/25 1:55 AM, Shuai Xue wrote:
>>>>
>>>>
>>>> 在 2025/6/3 22:32, Dave Jiang 写道:
>>>>>
>>>>>
>>>>> On 5/27/25 7:21 PM, Vinicius Costa Gomes wrote:
>>>>>> Shuai Xue <xueshuai@...ux.alibaba.com> writes:
>>>>>>
>>>>>>> 在 2025/5/23 22:54, Dave Jiang 写道:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 5/22/25 10:20 PM, Shuai Xue wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 在 2025/5/22 22:55, Dave Jiang 写道:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 5/21/25 11:33 PM, Shuai Xue wrote:
>>>>>>>>>>> A device reset command disables all WQs in hardware. If issued while a WQ
>>>>>>>>>>> is being enabled, it can cause a mismatch between the software and hardware
>>>>>>>>>>> states.
>>>>>>>>>>>
>>>>>>>>>>> When a hardware error occurs, the IDXD driver calls idxd_device_reset() to
>>>>>>>>>>> send a reset command and clear the state (wq->state) of all WQs. It then
>>>>>>>>>>> uses wq_enable_map (a bitmask tracking enabled WQs) to re-enable them and
>>>>>>>>>>> ensure consistency between the software and hardware states.
>>>>>>>>>>>
>>>>>>>>>>> However, a race condition exists between the WQ enable path and the
>>>>>>>>>>> reset/recovery path. For example:
>>>>>>>>>>>
>>>>>>>>>>> A: WQ enable path                   B: Reset and recovery path
>>>>>>>>>>> ------------------                 ------------------------
>>>>>>>>>>> a1. issue IDXD_CMD_ENABLE_WQ
>>>>>>>>>>>                                         b1. issue IDXD_CMD_RESET_DEVICE
>>>>>>>>>>>                                         b2. clear wq->state
>>>>>>>>>>>                                         b3. check wq_enable_map bit, not set
>>>>>>>>>>> a2. set wq->state = IDXD_WQ_ENABLED
>>>>>>>>>>> a3. set wq_enable_map
>>>>>>>>>>>
>>>>>>>>>>> In this case, b1 issues a reset command that disables all WQs in hardware.
>>>>>>>>>>> Since b3 checks wq_enable_map before a2, it doesn't re-enable the WQ,
>>>>>>>>>>> leading to an inconsistency between wq->state (software) and the actual
>>>>>>>>>>> hardware state (IDXD_WQ_DISABLED).
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Would it lessen the complication to just have wq enable path grab the device lock before proceeding?
>>>>>>>>>>
>>>>>>>>>> DJ
>>>>>>>>>
>>>>>>>>> Yep, how about add a spin lock to enable wq and reset device path.
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
>>>>>>>>> index 38633ec5b60e..c0dc904b2a94 100644
>>>>>>>>> --- a/drivers/dma/idxd/device.c
>>>>>>>>> +++ b/drivers/dma/idxd/device.c
>>>>>>>>> @@ -203,6 +203,29 @@ int idxd_wq_enable(struct idxd_wq *wq)
>>>>>>>>>      }
>>>>>>>>>      EXPORT_SYMBOL_GPL(idxd_wq_enable);
>>>>>>>>>      +/*
>>>>>>>>> + * This function enables a WQ in hareware and updates the driver maintained
>>>>>>>>> + * wq->state to IDXD_WQ_ENABLED. It should be called with the dev_lock held
>>>>>>>>> + * to prevent race conditions with IDXD_CMD_RESET_DEVICE, which could
>>>>>>>>> + * otherwise disable the WQ without the driver's state being properly
>>>>>>>>> + * updated.
>>>>>>>>> + *
>>>>>>>>> + * For IDXD_CMD_DISABLE_DEVICE, this function is safe because it is only
>>>>>>>>> + * called after the WQ has been explicitly disabled, so no concurrency
>>>>>>>>> + * issues arise.
>>>>>>>>> + */
>>>>>>>>> +int idxd_wq_enable_locked(struct idxd_wq *wq)
>>>>>>>>> +{
>>>>>>>>> +       struct idxd_device *idxd = wq->idxd;
>>>>>>>>> +       int ret;
>>>>>>>>> +
>>>>>>>>> +       spin_lock(&idxd->dev_lock);
>>>>>>>>
>>>>>>>> Let's start using the new cleanup macro going forward:
>>>>>>>> guard(spinlock)(&idxd->dev_lock);
>>>>>>>>
>>>>>>>> On a side note, there's been a cleanup on my mind WRT this driver's locking. I think we can replace idxd->dev_lock with idxd_confdev(idxd) device lock. You can end up just do:
>>>>>>>> guard(device)(idxd_confdev(idxd));
>>>>>>>
>>>>>>> Then we need to replace the lock from spinlock to mutex lock?
>>>>>>
>>>>>> We still need a (spin) lock that we could hold in interrupt contexts.
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> And also drop the wq->wq_lock and replace with wq_confdev(wq) device lock:
>>>>>>>> guard(device)(wq_confdev(wq));
>>>>>>>>
>>>>>>>> If you are up for it that is.
>>>>>>>
>>>>>>> We creates a hierarchy: pdev -> idxd device -> wq device.
>>>>>>> idxd_confdev(idxd) is the parent of wq_confdev(wq) because:
>>>>>>>
>>>>>>>         (wq_confdev(wq))->parent = idxd_confdev(idxd);
>>>>>>>
>>>>>>> Is it safe to grap lock of idxd_confdev(idxd) under hold
>>>>>>> lock of wq_confdev(wq)?
>>>>>>>
>>>>>>> We have mounts of code use spinlock of idxd->dev_lock under
>>>>>>> hold of wq->wq_lock.
>>>>>>>
>>>>>>
>>>>>> I agree with Dave that the locking could be simplified, but I don't
>>>>>> think that we should hold this series because of that. That
>>>>>> simplification can be done later.
>>>>>
>>>>> I agree. Just passing musing on the current code.
>>>>
>>>> Got it, do I need to send a separate patch for Patch 2?
>>>
>>> Not sure what you mean. Do you mean if you need to send patch 2 again?
>>
>> Yep, the locking issue is more complicate and can be done later.
>> (I could split patch 2 from this patch set if you prefer a new patch.)
> 
> Yes split it out and send it ahead if it can go independently.

Hi, Dave,

I split patch 2 out.

As for this race issue between WQ enable and reset paths, do you have any plan
or idea?


> 
>>
>>>>
>>>> Thanks.
>>>> Shuai
>>
>>

Powered by blists - more mailing lists