diff mbox series

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

Message ID 20191003132422.32730-1-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show
Series [v3] dma-fence: Serialise signal enabling (dma_fence_enable_sw_signaling) | expand

Commit Message

Chris Wilson Oct. 3, 2019, 1:24 p.m. UTC 
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@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(-)

Comments

Michael J. Ruhl Oct. 3, 2019, 2:12 p.m. UTC | #1 
>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
>Chris Wilson
>Sent: Thursday, October 3, 2019 9:24 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: dri-devel@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@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)?

Mike

>+
>+	if (!was_set && fence->ops->enable_signaling) {
>+		if (!fence->ops->enable_signaling(fence)) {
>+			dma_fence_signal_locked(fence);
>+			return false;
>+		}
>+
>+		trace_dma_fence_enable_signal(fence);
>+	}
>+
>+	return true;
>+}
>+
> /**
>  * dma_fence_enable_sw_signaling - enable signaling on fence
>  * @fence: the fence to enable
>@@ -285,19 +309,12 @@ void dma_fence_enable_sw_signaling(struct
>dma_fence *fence)
> {
> 	unsigned long flags;
>
>-	if (!test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
>-			      &fence->flags) &&
>-	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) &&
>-	    fence->ops->enable_signaling) {
>-		trace_dma_fence_enable_signal(fence);
>-
>-		spin_lock_irqsave(fence->lock, flags);
>-
>-		if (!fence->ops->enable_signaling(fence))
>-			dma_fence_signal_locked(fence);
>+	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>+		return;
>
>-		spin_unlock_irqrestore(fence->lock, flags);
>-	}
>+	spin_lock_irqsave(fence->lock, flags);
>+	__dma_fence_enable_signaling(fence);
>+	spin_unlock_irqrestore(fence->lock, flags);
> }
> EXPORT_SYMBOL(dma_fence_enable_sw_signaling);
>
>@@ -331,7 +348,6 @@ int dma_fence_add_callback(struct dma_fence
>*fence, struct dma_fence_cb *cb,
> {
> 	unsigned long flags;
> 	int ret = 0;
>-	bool was_set;
>
> 	if (WARN_ON(!fence || !func))
> 		return -EINVAL;
>@@ -343,25 +359,14 @@ int dma_fence_add_callback(struct dma_fence
>*fence, struct dma_fence_cb *cb,
>
> 	spin_lock_irqsave(fence->lock, flags);
>
>-	was_set =
>test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
>-				   &fence->flags);
>-
>-	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>-		ret = -ENOENT;
>-	else if (!was_set && fence->ops->enable_signaling) {
>-		trace_dma_fence_enable_signal(fence);
>-
>-		if (!fence->ops->enable_signaling(fence)) {
>-			dma_fence_signal_locked(fence);
>-			ret = -ENOENT;
>-		}
>-	}
>-
>-	if (!ret) {
>+	if (__dma_fence_enable_signaling(fence)) {
> 		cb->func = func;
> 		list_add_tail(&cb->node, &fence->cb_list);
>-	} else
>+	} else {
> 		INIT_LIST_HEAD(&cb->node);
>+		ret = -ENOENT;
>+	}
>+
> 	spin_unlock_irqrestore(fence->lock, flags);
>
> 	return ret;
>@@ -461,7 +466,6 @@ dma_fence_default_wait(struct dma_fence *fence,
>bool intr, signed long timeout)
> 	struct default_wait_cb cb;
> 	unsigned long flags;
> 	signed long ret = timeout ? timeout : 1;
>-	bool was_set;
>
> 	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> 		return ret;
>@@ -473,21 +477,9 @@ dma_fence_default_wait(struct dma_fence *fence,
>bool intr, signed long timeout)
> 		goto out;
> 	}
>
>-	was_set =
>test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
>-				   &fence->flags);
>-
>-	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>+	if (!__dma_fence_enable_signaling(fence))
> 		goto out;
>
>-	if (!was_set && fence->ops->enable_signaling) {
>-		trace_dma_fence_enable_signal(fence);
>-
>-		if (!fence->ops->enable_signaling(fence)) {
>-			dma_fence_signal_locked(fence);
>-			goto out;
>-		}
>-	}
>-
> 	if (!timeout) {
> 		ret = 0;
> 		goto out;
>--
>2.23.0
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson Oct. 3, 2019, 2:18 p.m. UTC | #2 
Quoting Ruhl, Michael J (2019-10-03 15:12:38)
> >-----Original Message-----
> >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> >Chris Wilson
> >Sent: Thursday, October 3, 2019 9:24 AM
> >To: intel-gfx@lists.freedesktop.org
> >Cc: dri-devel@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@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
Michael J. Ruhl Oct. 3, 2019, 2:42 p.m. UTC | #3 
>-----Original Message-----
>From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
>Sent: Thursday, October 3, 2019 10:19 AM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel-
>gfx@lists.freedesktop.org
>Cc: dri-devel@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@lists.freedesktop.org] On Behalf
>Of
>> >Chris Wilson
>> >Sent: Thursday, October 3, 2019 9:24 AM
>> >To: intel-gfx@lists.freedesktop.org
>> >Cc: dri-devel@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@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
Tvrtko Ursulin Oct. 3, 2019, 2:43 p.m. UTC | #4 
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@lists.freedesktop.org] On Behalf Of
>>> Chris Wilson
>>> Sent: Thursday, October 3, 2019 9:24 AM
>>> To: intel-gfx@lists.freedesktop.org
>>> Cc: dri-devel@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@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
diff mbox series

Patch

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);
+
+	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;
+
+	if (!was_set && fence->ops->enable_signaling) {
+		if (!fence->ops->enable_signaling(fence)) {
+			dma_fence_signal_locked(fence);
+			return false;
+		}
+
+		trace_dma_fence_enable_signal(fence);
+	}
+
+	return true;
+}
+
 /**
  * dma_fence_enable_sw_signaling - enable signaling on fence
  * @fence: the fence to enable
@@ -285,19 +309,12 @@  void dma_fence_enable_sw_signaling(struct dma_fence *fence)
 {
 	unsigned long flags;
 
-	if (!test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
-			      &fence->flags) &&
-	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) &&
-	    fence->ops->enable_signaling) {
-		trace_dma_fence_enable_signal(fence);
-
-		spin_lock_irqsave(fence->lock, flags);
-
-		if (!fence->ops->enable_signaling(fence))
-			dma_fence_signal_locked(fence);
+	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
+		return;
 
-		spin_unlock_irqrestore(fence->lock, flags);
-	}
+	spin_lock_irqsave(fence->lock, flags);
+	__dma_fence_enable_signaling(fence);
+	spin_unlock_irqrestore(fence->lock, flags);
 }
 EXPORT_SYMBOL(dma_fence_enable_sw_signaling);
 
@@ -331,7 +348,6 @@  int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb,
 {
 	unsigned long flags;
 	int ret = 0;
-	bool was_set;
 
 	if (WARN_ON(!fence || !func))
 		return -EINVAL;
@@ -343,25 +359,14 @@  int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb,
 
 	spin_lock_irqsave(fence->lock, flags);
 
-	was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
-				   &fence->flags);
-
-	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
-		ret = -ENOENT;
-	else if (!was_set && fence->ops->enable_signaling) {
-		trace_dma_fence_enable_signal(fence);
-
-		if (!fence->ops->enable_signaling(fence)) {
-			dma_fence_signal_locked(fence);
-			ret = -ENOENT;
-		}
-	}
-
-	if (!ret) {
+	if (__dma_fence_enable_signaling(fence)) {
 		cb->func = func;
 		list_add_tail(&cb->node, &fence->cb_list);
-	} else
+	} else {
 		INIT_LIST_HEAD(&cb->node);
+		ret = -ENOENT;
+	}
+
 	spin_unlock_irqrestore(fence->lock, flags);
 
 	return ret;
@@ -461,7 +466,6 @@  dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
 	struct default_wait_cb cb;
 	unsigned long flags;
 	signed long ret = timeout ? timeout : 1;
-	bool was_set;
 
 	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
 		return ret;
@@ -473,21 +477,9 @@  dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
 		goto out;
 	}
 
-	was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
-				   &fence->flags);
-
-	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
+	if (!__dma_fence_enable_signaling(fence))
 		goto out;
 
-	if (!was_set && fence->ops->enable_signaling) {
-		trace_dma_fence_enable_signal(fence);
-
-		if (!fence->ops->enable_signaling(fence)) {
-			dma_fence_signal_locked(fence);
-			goto out;
-		}
-	}
-
 	if (!timeout) {
 		ret = 0;
 		goto out;