Re: RESEND Re: [RFC PATCH] dma-buf/dma-fence: Use a successful read_trylock() annotation for dma_fence_begin_signalling()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Sorry, somehow completely missed that. Feel free to push it to drm-misc-next.

Christian.

Am 18.09.24 um 14:34 schrieb Thomas Hellström:
Christian,

Ping?


On Wed, 2024-08-14 at 10:37 +0200, Thomas Hellström wrote:
Christian,

Ack to merge this through drm-misc-next, or do you want to pick it up
for dma-buf?

Thanks,
Thomas


On Wed, 2024-08-14 at 09:10 +0200, Daniel Vetter wrote:
On Fri, May 26, 2023 at 01:11:28PM +0200, Thomas Hellström wrote:
Daniel,

On 4/28/23 14:52, Thomas Hellström wrote:
Condsider the following call sequence:

/* Upper layer */
dma_fence_begin_signalling();
lock(tainted_shared_lock);
/* Driver callback */
dma_fence_begin_signalling();
...

The driver might here use a utility that is annotated as
intended
for the
dma-fence signalling critical path. Now if the upper layer
isn't
correctly
annotated yet for whatever reason, resulting in

/* Upper layer */
lock(tainted_shared_lock);
/* Driver callback */
dma_fence_begin_signalling();

We will receive a false lockdep locking order violation
notification from
dma_fence_begin_signalling(). However entering a dma-fence
signalling
critical section itself doesn't block and could not cause a
deadlock.

So use a successful read_trylock() annotation instead for
dma_fence_begin_signalling(). That will make sure that the
locking order
is correctly registered in the first case, and doesn't register
any
locking order in the second case.

The alternative is of course to make sure that the "Upper
layer"
is always
correctly annotated. But experience shows that's not easily
achievable
in all cases.

Signed-off-by: Thomas Hellström
<thomas.hellstrom@xxxxxxxxxxxxxxx>
Resurrecting the discussion on this one. I can't see a situation
where we
would miss *relevant* locking
order violation warnings with this patch. Ofc if we have a
scheduler
annotation patch that would work fine as well, but the lack of
annotation in
the scheduler callbacks is really starting to hurt us.
Yeah this is just a bit too brain-melting to review, but I concur
now.

Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>









I think what would help is some lockdep selftests to check that we
both
catch the stuff we want to, and don't incur false positives. Maybe
with a
plea that lockdep should have some native form of cross-release
annotations ...

But definitely seperate patch set, since it might take a few rounds
of
review by lockdep folks.
-Sima

Thanks,

Thomas



---
   drivers/dma-buf/dma-fence.c | 6 +++---
   1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-
fence.c
index f177c56269bb..17f632768ef9 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -308,8 +308,8 @@ bool dma_fence_begin_signalling(void)
   	if (in_atomic())
   		return true;
-	/* ... and non-recursive readlock */
-	lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, NULL,
_RET_IP_);
+	/* ... and non-recursive successful read_trylock */
+	lock_acquire(&dma_fence_lockdep_map, 0, 1, 1, 1, NULL,
_RET_IP_);
   	return false;
   }
@@ -340,7 +340,7 @@ void __dma_fence_might_wait(void)
   	lock_map_acquire(&dma_fence_lockdep_map);
   	lock_map_release(&dma_fence_lockdep_map);
   	if (tmp)
-		lock_acquire(&dma_fence_lockdep_map, 0, 0, 1,
1,
NULL, _THIS_IP_);
+		lock_acquire(&dma_fence_lockdep_map, 0, 1, 1,
1,
NULL, _THIS_IP_);
   }
   #endif




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux