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

[Daniel Vetter <daniel.vetter@ffwll.ch>]

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@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@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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch