Re: [PATCH v2 4/4] dma-buf: Check status of enable-signaling bit on debug
From: Christian König
Date: Tue Sep 06 2022 - 07:36:06 EST
Am 06.09.22 um 13:21 schrieb Tvrtko Ursulin:
On 06/09/2022 11:43, Christian König wrote:
Am 06.09.22 um 12:20 schrieb Tvrtko Ursulin:
On 06/09/2022 09:39, Christian König wrote:
Am 05.09.22 um 18:35 schrieb Arvind Yadav:
The core DMA-buf framework needs to enable signaling
before the fence is signaled. The core DMA-buf framework
can forget to enable signaling before the fence is signaled.
This sentence is a bit confusing. I'm not a native speaker of
English either, but I suggest something like:
"Fence signaling must be enable to make sure that the
dma_fence_is_signaled() function ever returns true."
To avoid this scenario on the debug kernel, check the
DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT status bit before checking
the signaling bit status to confirm that enable_signaling
is enabled.
This describes the implementation, but we should rather describe
the background of the change. The implementation should be obvious.
Something like this maybe:
"
Since drivers and implementations sometimes mess this up enforce
correct behavior when DEBUG_WW_MUTEX_SLOWPATH is used during
debugging.
This should make any implementations bugs resulting in not signaled
fences much more obvious.
"
I think I follow the idea but am not sure coupling (well
"coupling".. not really, but cross-contaminating in a way)
dma-fence.c with a foreign and effectively unrelated concept of a ww
mutex is the best way.
Instead, how about a dma-buf specific debug kconfig option?
Yeah, I was thinking about that as well.
Cool, lets see about the specifics below and then decide.
The slowpath config option was just at hand because when you want to
test the slowpath you want to test this here as well.
Condition would then be, according to my understanding of the rules
and expectations, along the lines of:
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 775cdc0b4f24..147a9df2c9d0 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -428,6 +428,17 @@ dma_fence_is_signaled_locked(struct dma_fence
*fence)
static inline bool
dma_fence_is_signaled(struct dma_fence *fence)
{
+#ifdef CONFIG_DEBUG_DMAFENCE
+ /*
+ * Implementations not providing the enable_signaling
callback are
+ * required to always have signaling enabled or fences are not
+ * guaranteed to ever signal.
+ */
Well that comment is a bit misleading. The intention of the extra
check is to find bugs in the frontend and not the backend.
By backend you mean drivers/dma-buf/dma-fence.c and by front end
driver specific implementations? Or simply users for dma-fence?
Backend are the driver specific implementation of the dma_fence_ops.
Middleware is the framework in drivers/dma-buf.
Frontend is the users of the dma_fence interface, e.g. either drivers or
components (drm_syncobj, drm_scheduler etc...).
Could be that I got confused.. I was reading these two:
* This callback is optional. If this callback is not present,
then the
* driver must always have signaling enabled.
*/
bool (*enable_signaling)(struct dma_fence *fence);
And dma_fence_is_signaled:
* Returns true if the fence was already signaled, false if not. Since
this
* function doesn't enable signaling, it is not guaranteed to ever return
* true if dma_fence_add_callback(), dma_fence_wait() or
* dma_fence_enable_sw_signaling() haven't been called before.
Right, I think I did get confused, apologies. What I was thinking was
probably two separate conditions:
static inline bool
dma_fence_is_signaled(struct dma_fence *fence)
{
+#ifdef CONFIG_DEBUG_DMAFENCE
+ if (WARN_ON_ONCE(!fence->ops->enable_signaling &&
+ !test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags)))
+ return false;
+
+ if (!test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags))
+ return false;
+#endif
+
if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
return true;
Not sure "is signaled" is the best place for the first one or that it
should definitely be added.
I was thinking about adding this WARN_ON() as well, but then decided
against it.
There are a couple of cases where calling dma_fence_is_signaled()
without previously calling dma_fence_enable_sw_signaling() is valid
because it is done just for optimization purposes and we guarantee that
dma_fence_enable_sw_signaling() is called just a little bit later.
But yes, in general it's the same idea I already had as well.
Regards,
Christian.
Regards,
Tvrtko
In other words somewhere in the drm_syncobj code we have a
dma_fence_is_signaled() call without matching
dma_fence_enable_sw_signaling().
Regards,
Christian.
+ if (!fence->ops->enable_signaling &&
+ !test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags))
+ return false;
+#endif
+
if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
return true;
Thoughts?
Regards,
Tvrtko
Signed-off-by: Arvind Yadav <Arvind.Yadav@xxxxxxx>
With the improved commit message this patch is Reviewed-by:
Christian König <christian.koenig@xxxxxxx>
Regards,
Christian.
---
Changes in v1 :
1- Addressing Christian's comment to replace
CONFIG_DEBUG_WW_MUTEX_SLOWPATH instead of CONFIG_DEBUG_FS.
2- As per Christian's comment moving this patch at last so
The version of this patch is also changed and previously
it was [PATCH 1/4]
---
include/linux/dma-fence.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 775cdc0b4f24..ba1ddc14c5d4 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -428,6 +428,11 @@ dma_fence_is_signaled_locked(struct dma_fence
*fence)
static inline bool
dma_fence_is_signaled(struct dma_fence *fence)
{
+#ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH
+ if (!test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags))
+ return false;
+#endif
+
if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
return true;