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

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

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