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

[Chris Wilson]

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.
-Chris