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

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