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

[Ruhl, Michael J]

>-----Original Message-----
>From: Chris Wilson [mailto:chris at chris-wilson.co.uk]
>Sent: Thursday, October 3, 2019 10:19 AM
>To: Ruhl, Michael J <michael.j.ruhl at intel.com>; intel-
>gfx at lists.freedesktop.org
>Cc: dri-devel at lists.freedesktop.org
>Subject: RE: [Intel-gfx] [PATCH v3] dma-fence: Serialise signal enabling
>(dma_fence_enable_sw_signaling)
>
>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.

Ah got it.

In the future, I  will think a bit outside the patch.  (sorry for the pun...)

Thanks,

Mike

>-Chris