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

[Thomas Hellström]

On Wed, 2024-09-18 at 15:18 +0200, Christian König wrote:
> Sorry, somehow completely missed that. Feel free to push it to 
> drm-misc-next.
> 
> Christian.

Pushed, thanks.
/Thomas


> 
> 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 at linux.intel.com>
> > > > > 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 at ffwll.ch>
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > > 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
>