[Intel-gfx] [PATCH v3] dma-fence: Serialise signal enabling (dma_fence_enable_sw_signaling)

[Tvrtko Ursulin]

On 03/10/2019 15:18, Chris Wilson wrote:
> Quoting Ruhl, Michael J (2019-10-03 15:12:38)
>>> -----Original Message-----
>>> From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of
>>> Chris Wilson
>>> Sent: Thursday, October 3, 2019 9:24 AM
>>> To: intel-gfx at lists.freedesktop.org
>>> Cc: dri-devel at lists.freedesktop.org
>>> Subject: [Intel-gfx] [PATCH v3] dma-fence: Serialise signal enabling
>>> (dma_fence_enable_sw_signaling)
>>>
>>> Make dma_fence_enable_sw_signaling() behave like its
>>> dma_fence_add_callback() and dma_fence_default_wait() counterparts and
>>> perform the test to enable signaling under the fence->lock, along with
>>> the action to do so. This ensure that should an implementation be trying
>>> to flush the cb_list (by signaling) on retirement before freeing the
>>> fence, it can do so in a race-free manner.
>>>
>>> See also 0fc89b6802ba ("dma-fence: Simply wrap dma_fence_signal_locked
>>> with dma_fence_signal").
>>>
>>> v2: Refactor all 3 enable_signaling paths to use a common function.
>>>
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> ---
>>> Return false for "could not _enable_ signaling as it was already
>>> signaled"
>>> ---
>>> drivers/dma-buf/dma-fence.c | 78 +++++++++++++++++--------------------
>>> 1 file changed, 35 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>>> index 2c136aee3e79..b58528c1cc9d 100644
>>> --- a/drivers/dma-buf/dma-fence.c
>>> +++ b/drivers/dma-buf/dma-fence.c
>>> @@ -273,6 +273,30 @@ void dma_fence_free(struct dma_fence *fence)
>>> }
>>> EXPORT_SYMBOL(dma_fence_free);
>>>
>>> +static bool __dma_fence_enable_signaling(struct dma_fence *fence)
>>> +{
>>> +      bool was_set;
>>> +
>>> +      lockdep_assert_held(fence->lock);
>>
>> With this held...
>>
>>> +      was_set =
>>> test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
>>> +                                 &fence->flags);
>>> +
>>> +      if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>>> +              return false;
>>
>> Would making these the non-atomic versions be useful (and/or reasonable)?
> 
> That depends on all modifications to the dword (not just the bit) being
> serialised by the same lock (or otherwise guaranteed to be serial and
> coherent), which as Tvrtko rediscovered is not the case. dma_fence.flags
> is also home to a number of user flags.

Michael, for completeness of the reference - I fell into the same trap - 
https://patchwork.freedesktop.org/series/67532/. Check the results. :)

Regards,

Tvrtko