[RFC,02/17] dma-fence: basic lockdep annotations

Commit Message

Daniel Vetter May 12, 2020, 8:59 a.m. UTC

Design is similar to the lockdep annotations for workers, but with
some twists:

- We use a read-lock for the execution/worker/completion side, so that
  this explicit annotation can be more liberally sprinkled around.
  With read locks lockdep isn't going to complain if the read-side
  isn't nested the same way under all circumstances, so ABBA deadlocks
  are ok. Which they are, since this is an annotation only.

- We're using non-recursive lockdep read lock mode, since in recursive
  read lock mode lockdep does not catch read side hazards. And we
  _very_ much want read side hazards to be caught. For full details of
  this limitation see

  commit e91498589746065e3ae95d9a00b068e525eec34f
  Author: Peter Zijlstra <[e-mail]>
  Date:   Wed Aug 23 13:13:11 2017 +0200

      locking/lockdep/selftests: Add mixed read-write ABBA tests

- To allow nesting of the read-side explicit annotations we explicitly
  keep track of the nesting. lock_is_held() allows us to do that.

- The wait-side annotation is a write lock, and entirely done within
  dma_fence_wait() for everyone by default.

- To be able to freely annotate helper functions I want to make it ok
  to call dma_fence_begin/end_signalling from soft/hardirq context.
  First attempt was using the hardirq locking context for the write
  side in lockdep, but this forces all normal spinlocks nested within
  dma_fence_begin/end_signalling to be spinlocks. That bollocks.

  The approach now is to simple check in_atomic(), and for these cases
  entirely rely on the might_sleep() check in dma_fence_wait(). That
  will catch any wrong nesting against spinlocks from soft/hardirq
  contexts.

The idea here is that every code path that's critical for eventually
signalling a dma_fence should be annotated with
dma_fence_begin/end_signalling. The annotation ideally starts right
after a dma_fence is published (added to a dma_resv, exposed as a
sync_file fd, attached to a drm_syncobj fd, or anything else that
makes the dma_fence visible to other kernel threads), up to and
including the dma_fence_wait(). Examples are irq handlers, the
scheduler rt threads, the tail of execbuf (after the corresponding
fences are visible), any workers that end up signalling dma_fences and
really anything else. Not annotated should be code paths that only
complete fences opportunistically as the gpu progresses, like e.g.
shrinker/eviction code.

The main class of deadlocks this is supposed to catch are:

Thread A:

	mutex_lock(A);
	mutex_unlock(A);

	dma_fence_signal();

Thread B:

	mutex_lock(A);
	dma_fence_wait();
	mutex_unlock(A);

Thread B is blocked on A signalling the fence, but A never gets around
to that because it cannot acquire the lock A.

Note that dma_fence_wait() is allowed to be nested within
dma_fence_begin/end_signalling sections. To allow this to happen the
read lock needs to be upgraded to a write lock, which means that any
other lock is acquired between the dma_fence_begin_signalling() call and
the call to dma_fence_wait(), and still held, this will result in an
immediate lockdep complaint. The only other option would be to not
annotate such calls, defeating the point. Therefore these annotations
cannot be sprinkled over the code entirely mindless to avoid false
positives.

v2: handle soft/hardirq ctx better against write side and dont forget
EXPORT_SYMBOL, drivers can't use this otherwise.

Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: Chris Wilson <[e-mail]>
Cc: Maarten Lankhorst <[e-mail]>
Cc: Christian König <[e-mail]>
Signed-off-by: Daniel Vetter <[e-mail]>
---
 drivers/dma-buf/dma-fence.c | 53 +++++++++++++++++++++++++++++++++++++
 include/linux/dma-fence.h   | 12 +++++++++
 2 files changed, 65 insertions(+)

Comments

Daniel Vetter May 12, 2020, 9:08 a.m. UTC | #1

On Tue, May 12, 2020 at 10:04:22AM +0100, Chris Wilson wrote:
> Quoting Daniel Vetter (2020-05-12 09:59:29)
> > Design is similar to the lockdep annotations for workers, but with
> > some twists:
> > 
> > - We use a read-lock for the execution/worker/completion side, so that
> >   this explicit annotation can be more liberally sprinkled around.
> >   With read locks lockdep isn't going to complain if the read-side
> >   isn't nested the same way under all circumstances, so ABBA deadlocks
> >   are ok. Which they are, since this is an annotation only.
> > 
> > - We're using non-recursive lockdep read lock mode, since in recursive
> >   read lock mode lockdep does not catch read side hazards. And we
> >   _very_ much want read side hazards to be caught. For full details of
> >   this limitation see
> > 
> >   commit e91498589746065e3ae95d9a00b068e525eec34f
> >   Author: Peter Zijlstra <[e-mail]>
> >   Date:   Wed Aug 23 13:13:11 2017 +0200
> > 
> >       locking/lockdep/selftests: Add mixed read-write ABBA tests
> > 
> > - To allow nesting of the read-side explicit annotations we explicitly
> >   keep track of the nesting. lock_is_held() allows us to do that.
> > 
> > - The wait-side annotation is a write lock, and entirely done within
> >   dma_fence_wait() for everyone by default.
> > 
> > - To be able to freely annotate helper functions I want to make it ok
> >   to call dma_fence_begin/end_signalling from soft/hardirq context.
> >   First attempt was using the hardirq locking context for the write
> >   side in lockdep, but this forces all normal spinlocks nested within
> >   dma_fence_begin/end_signalling to be spinlocks. That bollocks.
> > 
> >   The approach now is to simple check in_atomic(), and for these cases
> >   entirely rely on the might_sleep() check in dma_fence_wait(). That
> >   will catch any wrong nesting against spinlocks from soft/hardirq
> >   contexts.
> > 
> > The idea here is that every code path that's critical for eventually
> > signalling a dma_fence should be annotated with
> > dma_fence_begin/end_signalling. The annotation ideally starts right
> > after a dma_fence is published (added to a dma_resv, exposed as a
> > sync_file fd, attached to a drm_syncobj fd, or anything else that
> > makes the dma_fence visible to other kernel threads), up to and
> > including the dma_fence_wait(). Examples are irq handlers, the
> > scheduler rt threads, the tail of execbuf (after the corresponding
> > fences are visible), any workers that end up signalling dma_fences and
> > really anything else. Not annotated should be code paths that only
> > complete fences opportunistically as the gpu progresses, like e.g.
> > shrinker/eviction code.
> > 
> > The main class of deadlocks this is supposed to catch are:
> > 
> > Thread A:
> > 
> >         mutex_lock(A);
> >         mutex_unlock(A);
> > 
> >         dma_fence_signal();
> > 
> > Thread B:
> > 
> >         mutex_lock(A);
> >         dma_fence_wait();
> >         mutex_unlock(A);
> > 
> > Thread B is blocked on A signalling the fence, but A never gets around
> > to that because it cannot acquire the lock A.
> > 
> > Note that dma_fence_wait() is allowed to be nested within
> > dma_fence_begin/end_signalling sections. To allow this to happen the
> > read lock needs to be upgraded to a write lock, which means that any
> > other lock is acquired between the dma_fence_begin_signalling() call and
> > the call to dma_fence_wait(), and still held, this will result in an
> > immediate lockdep complaint. The only other option would be to not
> > annotate such calls, defeating the point. Therefore these annotations
> > cannot be sprinkled over the code entirely mindless to avoid false
> > positives.
> > 
> > v2: handle soft/hardirq ctx better against write side and dont forget
> > EXPORT_SYMBOL, drivers can't use this otherwise.
> > 
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: Chris Wilson <[e-mail]>
> > Cc: Maarten Lankhorst <[e-mail]>
> > Cc: Christian König <[e-mail]>
> > Signed-off-by: Daniel Vetter <[e-mail]>
> > ---
> >  drivers/dma-buf/dma-fence.c | 53 +++++++++++++++++++++++++++++++++++++
> >  include/linux/dma-fence.h   | 12 +++++++++
> >  2 files changed, 65 insertions(+)
> > 
> > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > index 6802125349fb..d5c0fd2efc70 100644
> > --- a/drivers/dma-buf/dma-fence.c
> > +++ b/drivers/dma-buf/dma-fence.c
> > @@ -110,6 +110,52 @@ u64 dma_fence_context_alloc(unsigned num)
> >  }
> >  EXPORT_SYMBOL(dma_fence_context_alloc);
> >  
> > +#ifdef CONFIG_LOCKDEP
> > +struct lockdep_map     dma_fence_lockdep_map = {
> > +       .name = "dma_fence_map"
> > +};
> 
> Not another false global sharing lockmap.

It's a global contract, it needs a global lockdep map. And yes a big
reason for the motivation here is that i915-gem has a tremendous urge to
just redefine all these global locks to fit to some local interpretation
of what's going on.

That doesn't make the resulting real&existing deadlocks go away.
-Daniel

Jason Gunthorpe May 12, 2020, 12:09 p.m. UTC | #2

On Tue, May 12, 2020 at 10:59:29AM +0200, Daniel Vetter wrote:
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 6802125349fb..d5c0fd2efc70 100644
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -110,6 +110,52 @@ u64 dma_fence_context_alloc(unsigned num)
>  }
>  EXPORT_SYMBOL(dma_fence_context_alloc);
>  
> +#ifdef CONFIG_LOCKDEP
> +struct lockdep_map	dma_fence_lockdep_map = {
> +	.name = "dma_fence_map"
> +};
> +
> +bool dma_fence_begin_signalling(void)
> +{

Why is this global? I would have expected it to be connected to a
single fence?

It would also be alot nicer if this was some general lockdep feature,
not tied to dmabuf. This exact problem also strikes anyone using
completions, for instance, and the same solution should be
applicable??

Jason

Daniel Vetter May 12, 2020, 12:57 p.m. UTC | #3

On Tue, May 12, 2020 at 09:09:52AM -0300, Jason Gunthorpe wrote:
> On Tue, May 12, 2020 at 10:59:29AM +0200, Daniel Vetter wrote:
> > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > index 6802125349fb..d5c0fd2efc70 100644
> > +++ b/drivers/dma-buf/dma-fence.c
> > @@ -110,6 +110,52 @@ u64 dma_fence_context_alloc(unsigned num)
> >  }
> >  EXPORT_SYMBOL(dma_fence_context_alloc);
> >  
> > +#ifdef CONFIG_LOCKDEP
> > +struct lockdep_map	dma_fence_lockdep_map = {
> > +	.name = "dma_fence_map"
> > +};
> > +
> > +bool dma_fence_begin_signalling(void)
> > +{
> 
> Why is this global? I would have expected it to be connected to a
> single fence?

It's the same rules for all fences, since they can be shared across
drivers in various ways. Lockdep usually achieves that with a static
variable hidden in the macro, but that doesn't work if you have lots of
different ways from different drivers to create a dma_fence. Hence the
unique global one that we explicitly allocate.

We have similar stuff for the global dma_resv_lock ww_mutex class, just
there it's a bit more standardized and hidden behind a neat macro. But
really lockdep needs global lockdep_maps or it doesn't work.

> It would also be alot nicer if this was some general lockdep feature,
> not tied to dmabuf. This exact problem also strikes anyone using
> completions, for instance, and the same solution should be
> applicable??

There was:

https://lwn.net/Articles/709849/

It even got merged, and seems to have worked. Unfortunately (and I'm not
entirely clear on the reasons) it was thrown out again, so we can't use
it. That means wait_event/wake_up dependencies need to be manually
annotated, like e.g. flush_work() already is. flush_work is more or less
where I've stolen this idea from, with some adjustements and tricks on top
to make it work for dma_fence users.

Cheers, Daniel

Daniel Vetter May 13, 2020, 8:30 a.m. UTC | #4

On Tue, May 12, 2020 at 11:19 AM Chris Wilson <[e-mail]> wrote:
> Quoting Daniel Vetter (2020-05-12 10:08:47)
> > On Tue, May 12, 2020 at 10:04:22AM +0100, Chris Wilson wrote:
> > > Quoting Daniel Vetter (2020-05-12 09:59:29)
> > > > Design is similar to the lockdep annotations for workers, but with
> > > > some twists:
> > > >
> > > > - We use a read-lock for the execution/worker/completion side, so that
> > > >   this explicit annotation can be more liberally sprinkled around.
> > > >   With read locks lockdep isn't going to complain if the read-side
> > > >   isn't nested the same way under all circumstances, so ABBA deadlocks
> > > >   are ok. Which they are, since this is an annotation only.
> > > >
> > > > - We're using non-recursive lockdep read lock mode, since in recursive
> > > >   read lock mode lockdep does not catch read side hazards. And we
> > > >   _very_ much want read side hazards to be caught. For full details of
> > > >   this limitation see
> > > >
> > > >   commit e91498589746065e3ae95d9a00b068e525eec34f
> > > >   Author: Peter Zijlstra <[e-mail]>
> > > >   Date:   Wed Aug 23 13:13:11 2017 +0200
> > > >
> > > >       locking/lockdep/selftests: Add mixed read-write ABBA tests
> > > >
> > > > - To allow nesting of the read-side explicit annotations we explicitly
> > > >   keep track of the nesting. lock_is_held() allows us to do that.
> > > >
> > > > - The wait-side annotation is a write lock, and entirely done within
> > > >   dma_fence_wait() for everyone by default.
> > > >
> > > > - To be able to freely annotate helper functions I want to make it ok
> > > >   to call dma_fence_begin/end_signalling from soft/hardirq context.
> > > >   First attempt was using the hardirq locking context for the write
> > > >   side in lockdep, but this forces all normal spinlocks nested within
> > > >   dma_fence_begin/end_signalling to be spinlocks. That bollocks.
> > > >
> > > >   The approach now is to simple check in_atomic(), and for these cases
> > > >   entirely rely on the might_sleep() check in dma_fence_wait(). That
> > > >   will catch any wrong nesting against spinlocks from soft/hardirq
> > > >   contexts.
> > > >
> > > > The idea here is that every code path that's critical for eventually
> > > > signalling a dma_fence should be annotated with
> > > > dma_fence_begin/end_signalling. The annotation ideally starts right
> > > > after a dma_fence is published (added to a dma_resv, exposed as a
> > > > sync_file fd, attached to a drm_syncobj fd, or anything else that
> > > > makes the dma_fence visible to other kernel threads), up to and
> > > > including the dma_fence_wait(). Examples are irq handlers, the
> > > > scheduler rt threads, the tail of execbuf (after the corresponding
> > > > fences are visible), any workers that end up signalling dma_fences and
> > > > really anything else. Not annotated should be code paths that only
> > > > complete fences opportunistically as the gpu progresses, like e.g.
> > > > shrinker/eviction code.
> > > >
> > > > The main class of deadlocks this is supposed to catch are:
> > > >
> > > > Thread A:
> > > >
> > > >         mutex_lock(A);
> > > >         mutex_unlock(A);
> > > >
> > > >         dma_fence_signal();
> > > >
> > > > Thread B:
> > > >
> > > >         mutex_lock(A);
> > > >         dma_fence_wait();
> > > >         mutex_unlock(A);
> > > >
> > > > Thread B is blocked on A signalling the fence, but A never gets around
> > > > to that because it cannot acquire the lock A.
> > > >
> > > > Note that dma_fence_wait() is allowed to be nested within
> > > > dma_fence_begin/end_signalling sections. To allow this to happen the
> > > > read lock needs to be upgraded to a write lock, which means that any
> > > > other lock is acquired between the dma_fence_begin_signalling() call and
> > > > the call to dma_fence_wait(), and still held, this will result in an
> > > > immediate lockdep complaint. The only other option would be to not
> > > > annotate such calls, defeating the point. Therefore these annotations
> > > > cannot be sprinkled over the code entirely mindless to avoid false
> > > > positives.
> > > >
> > > > v2: handle soft/hardirq ctx better against write side and dont forget
> > > > EXPORT_SYMBOL, drivers can't use this otherwise.
> > > >
> > > > Cc: [e-mail]
> > > > Cc: [e-mail]
> > > > Cc: [e-mail]
> > > > Cc: [e-mail]
> > > > Cc: [e-mail]
> > > > Cc: Chris Wilson <[e-mail]>
> > > > Cc: Maarten Lankhorst <[e-mail]>
> > > > Cc: Christian König <[e-mail]>
> > > > Signed-off-by: Daniel Vetter <[e-mail]>
> > > > ---
> > > >  drivers/dma-buf/dma-fence.c | 53 +++++++++++++++++++++++++++++++++++++
> > > >  include/linux/dma-fence.h   | 12 +++++++++
> > > >  2 files changed, 65 insertions(+)
> > > >
> > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > > > index 6802125349fb..d5c0fd2efc70 100644
> > > > --- a/drivers/dma-buf/dma-fence.c
> > > > +++ b/drivers/dma-buf/dma-fence.c
> > > > @@ -110,6 +110,52 @@ u64 dma_fence_context_alloc(unsigned num)
> > > >  }
> > > >  EXPORT_SYMBOL(dma_fence_context_alloc);
> > > >
> > > > +#ifdef CONFIG_LOCKDEP
> > > > +struct lockdep_map     dma_fence_lockdep_map = {
> > > > +       .name = "dma_fence_map"
> > > > +};
> > >
> > > Not another false global sharing lockmap.
> >
> > It's a global contract, it needs a global lockdep map. And yes a big
> > reason for the motivation here is that i915-gem has a tremendous urge to
> > just redefine all these global locks to fit to some local interpretation
> > of what's going on.
>
> No, you can build the global contract out of the actual contracts
> between fence drivers. If you introduce a struct lockdep_map *map into
> the fence_ops (so the fence_ops can remain const), you gain correctness
> at the cost of having to run through all possible interactions once.
> You can also then do if ops->lockmap ?: &global_fence_lockmap for
> piecemeal conversion of drivers that do not already use lockmaps for
> contract enforcement of their fence waits.

I'm not quite sure whether you're actually proposing to have locking
contracts per drivers, since that seems rather out of ... I dunno. But
if that's what you want, that just doesn't make any sense at all:

- Locking is rather core to kernel programming, aside from a few other
things like hard/softirq/preempt/... disabled sections and how
recursion works for these, or where and what you're allowed to
allocate memory. Lockdep, might_sleep and a bunch of other such debug
checks help us enforce that. If you instead go with every driver does
what they please yolo, then you don't have an abstraction, all you
have is smashing a rose and rose and Rose into one thing because they
have the same 4 letter name. It's just an interface that can be used
only when understanding every single implementation in detail - really
not something that's an abstraction. Yes I've seen some of these
dubious abstractions in i915, merged fairly recently, that doesn't
make them a good idea.

- You need to test the full NxN matrix (yes you need to test the
driver against itself in this world, since testing against something
fake like vgem doesn't cut it). That's nuts. Strike that, that's
impossible.

- Review is impossible, because the documentation can be summed up as
"yolo". Without clear rules all review can do is check every code
against every other piece of code, on every change. That's impossible,
because we humans are mere mortals, and we're left with push&pray
engineering, which really isn't.

The other issue with this approach is that it's full on platform
problem in extremis. Instead of extending the shared abstraction or
adding new useful functionality, i915-gem has resorted to reinpreting
rules to fix local problems. That leads to stuff like roughly

if (mutex_lock_timeout(HZ*10) == -ETIME) {
    /* I guess we deadlocked, try to bail out */
}

except it's for fences. That's neither solid engineering - we don't
generally let the kernel deadlock on itself to test whether maybe it
was a deadlock or not, nor is this solid upstreaming in a open source
project - we fix the problems where they are, not work around them
just in our own driver.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
[phone] - http://blog.ffwll.ch

Daniel Vetter May 25, 2020, 3:41 p.m. UTC | #5

On Tue, May 12, 2020 at 11:04 AM Chris Wilson <[e-mail]> wrote:
>
> Quoting Daniel Vetter (2020-05-12 09:59:29)
> > Design is similar to the lockdep annotations for workers, but with
> > some twists:
> >
> > - We use a read-lock for the execution/worker/completion side, so that
> >   this explicit annotation can be more liberally sprinkled around.
> >   With read locks lockdep isn't going to complain if the read-side
> >   isn't nested the same way under all circumstances, so ABBA deadlocks
> >   are ok. Which they are, since this is an annotation only.
> >
> > - We're using non-recursive lockdep read lock mode, since in recursive
> >   read lock mode lockdep does not catch read side hazards. And we
> >   _very_ much want read side hazards to be caught. For full details of
> >   this limitation see
> >
> >   commit e91498589746065e3ae95d9a00b068e525eec34f
> >   Author: Peter Zijlstra <[e-mail]>
> >   Date:   Wed Aug 23 13:13:11 2017 +0200
> >
> >       locking/lockdep/selftests: Add mixed read-write ABBA tests
> >
> > - To allow nesting of the read-side explicit annotations we explicitly
> >   keep track of the nesting. lock_is_held() allows us to do that.
> >
> > - The wait-side annotation is a write lock, and entirely done within
> >   dma_fence_wait() for everyone by default.
> >
> > - To be able to freely annotate helper functions I want to make it ok
> >   to call dma_fence_begin/end_signalling from soft/hardirq context.
> >   First attempt was using the hardirq locking context for the write
> >   side in lockdep, but this forces all normal spinlocks nested within
> >   dma_fence_begin/end_signalling to be spinlocks. That bollocks.
> >
> >   The approach now is to simple check in_atomic(), and for these cases
> >   entirely rely on the might_sleep() check in dma_fence_wait(). That
> >   will catch any wrong nesting against spinlocks from soft/hardirq
> >   contexts.
> >
> > The idea here is that every code path that's critical for eventually
> > signalling a dma_fence should be annotated with
> > dma_fence_begin/end_signalling. The annotation ideally starts right
> > after a dma_fence is published (added to a dma_resv, exposed as a
> > sync_file fd, attached to a drm_syncobj fd, or anything else that
> > makes the dma_fence visible to other kernel threads), up to and
> > including the dma_fence_wait(). Examples are irq handlers, the
> > scheduler rt threads, the tail of execbuf (after the corresponding
> > fences are visible), any workers that end up signalling dma_fences and
> > really anything else. Not annotated should be code paths that only
> > complete fences opportunistically as the gpu progresses, like e.g.
> > shrinker/eviction code.
> >
> > The main class of deadlocks this is supposed to catch are:
> >
> > Thread A:
> >
> >         mutex_lock(A);
> >         mutex_unlock(A);
> >
> >         dma_fence_signal();
> >
> > Thread B:
> >
> >         mutex_lock(A);
> >         dma_fence_wait();
> >         mutex_unlock(A);
> >
> > Thread B is blocked on A signalling the fence, but A never gets around
> > to that because it cannot acquire the lock A.
> >
> > Note that dma_fence_wait() is allowed to be nested within
> > dma_fence_begin/end_signalling sections. To allow this to happen the
> > read lock needs to be upgraded to a write lock, which means that any
> > other lock is acquired between the dma_fence_begin_signalling() call and
> > the call to dma_fence_wait(), and still held, this will result in an
> > immediate lockdep complaint. The only other option would be to not
> > annotate such calls, defeating the point. Therefore these annotations
> > cannot be sprinkled over the code entirely mindless to avoid false
> > positives.
> >
> > v2: handle soft/hardirq ctx better against write side and dont forget
> > EXPORT_SYMBOL, drivers can't use this otherwise.
> >
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: Chris Wilson <[e-mail]>
> > Cc: Maarten Lankhorst <[e-mail]>
> > Cc: Christian König <[e-mail]>
> > Signed-off-by: Daniel Vetter <[e-mail]>
> > ---
> >  drivers/dma-buf/dma-fence.c | 53 +++++++++++++++++++++++++++++++++++++
> >  include/linux/dma-fence.h   | 12 +++++++++
> >  2 files changed, 65 insertions(+)
> >
> > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > index 6802125349fb..d5c0fd2efc70 100644
> > --- a/drivers/dma-buf/dma-fence.c
> > +++ b/drivers/dma-buf/dma-fence.c
> > @@ -110,6 +110,52 @@ u64 dma_fence_context_alloc(unsigned num)
> >  }
> >  EXPORT_SYMBOL(dma_fence_context_alloc);
> >
> > +#ifdef CONFIG_LOCKDEP
> > +struct lockdep_map     dma_fence_lockdep_map = {
> > +       .name = "dma_fence_map"
> > +};
>
> Not another false global sharing lockmap.

So in some meetings you also mentioned nesting is going to be a
problem here. I see about three different kinds of nesting here, but
none should be a fundamental problem:

- nesting of fence drivers, specifically the syncobj timeline fences
but there's others around dma_fence->lock. This series is about
blocking deadlocks, it doesn't care about irqsave spinlocks at all. So
all the nesting going on there is entirely unchanged. Validation
against atomic section relies on the might_sleep annotation in the
first patch.

- nesting of callers, for better code composability. The annotations
are recursive, I've tested it with amdgpu, works.

- nesting of timelines, where e.g. you have some scheduler completion
events that drive the scheduler logic, which eventually will also
result in userspace visible fences on some context getting completed.
Works for amdgpu, that's why I annotated the scheduler. Also, not a
problem for two reasons:

1. uapi relevant fences are the relevant fences for the cross-driver
contract. Building something outside of them few fewer constraints
doesn't make sense, that would just mean we make the dma_fence
cross-driver contract less strict (but then for everyone, not just for
one driver, cause that asymmetric doesn't really work)

2. fences entirely hidden in drivers, which driver something
underneath the uapi visible fences (like scheduler or whatever). Those
can be more constraint, but as long as they're driving the public
fences, can't be less constrained. So cross-driver annotations don't
give you any limitations, you still can do your own driver-internal
annotations to track this more strict constraints.

So really not seeing the fence nesting issue here, either it's a
totally different one, or I'm misunderstood something.

I guess the other issue is that there's a ton of code that's broken
all around in various drivers, but that's why the RFC part. I
specifically highlighted that the priming patch needs some serious
discussion, but "nope I don't want a cross driver contract" really
isn't that.

Cheers, Daniel

Maarten Lankhorst May 26, 2020, 10 a.m. UTC | #6

Op 12-05-2020 om 10:59 schreef Daniel Vetter:
> Design is similar to the lockdep annotations for workers, but with
> some twists:
>
> - We use a read-lock for the execution/worker/completion side, so that
>   this explicit annotation can be more liberally sprinkled around.
>   With read locks lockdep isn't going to complain if the read-side
>   isn't nested the same way under all circumstances, so ABBA deadlocks
>   are ok. Which they are, since this is an annotation only.
>
> - We're using non-recursive lockdep read lock mode, since in recursive
>   read lock mode lockdep does not catch read side hazards. And we
>   _very_ much want read side hazards to be caught. For full details of
>   this limitation see
>
>   commit e91498589746065e3ae95d9a00b068e525eec34f
>   Author: Peter Zijlstra <[e-mail]>
>   Date:   Wed Aug 23 13:13:11 2017 +0200
>
>       locking/lockdep/selftests: Add mixed read-write ABBA tests
>
> - To allow nesting of the read-side explicit annotations we explicitly
>   keep track of the nesting. lock_is_held() allows us to do that.
>
> - The wait-side annotation is a write lock, and entirely done within
>   dma_fence_wait() for everyone by default.
>
> - To be able to freely annotate helper functions I want to make it ok
>   to call dma_fence_begin/end_signalling from soft/hardirq context.
>   First attempt was using the hardirq locking context for the write
>   side in lockdep, but this forces all normal spinlocks nested within
>   dma_fence_begin/end_signalling to be spinlocks. That bollocks.
>
>   The approach now is to simple check in_atomic(), and for these cases
>   entirely rely on the might_sleep() check in dma_fence_wait(). That
>   will catch any wrong nesting against spinlocks from soft/hardirq
>   contexts.
>
> The idea here is that every code path that's critical for eventually
> signalling a dma_fence should be annotated with
> dma_fence_begin/end_signalling. The annotation ideally starts right
> after a dma_fence is published (added to a dma_resv, exposed as a
> sync_file fd, attached to a drm_syncobj fd, or anything else that
> makes the dma_fence visible to other kernel threads), up to and
> including the dma_fence_wait(). Examples are irq handlers, the
> scheduler rt threads, the tail of execbuf (after the corresponding
> fences are visible), any workers that end up signalling dma_fences and
> really anything else. Not annotated should be code paths that only
> complete fences opportunistically as the gpu progresses, like e.g.
> shrinker/eviction code.
>
> The main class of deadlocks this is supposed to catch are:
>
> Thread A:
>
> 	mutex_lock(A);
> 	mutex_unlock(A);
>
> 	dma_fence_signal();
>
> Thread B:
>
> 	mutex_lock(A);
> 	dma_fence_wait();
> 	mutex_unlock(A);
>
> Thread B is blocked on A signalling the fence, but A never gets around
> to that because it cannot acquire the lock A.
>
> Note that dma_fence_wait() is allowed to be nested within
> dma_fence_begin/end_signalling sections. To allow this to happen the
> read lock needs to be upgraded to a write lock, which means that any
> other lock is acquired between the dma_fence_begin_signalling() call and
> the call to dma_fence_wait(), and still held, this will result in an
> immediate lockdep complaint. The only other option would be to not
> annotate such calls, defeating the point. Therefore these annotations
> cannot be sprinkled over the code entirely mindless to avoid false
> positives.
>
> v2: handle soft/hardirq ctx better against write side and dont forget
> EXPORT_SYMBOL, drivers can't use this otherwise.
>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: Chris Wilson <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Christian König <[e-mail]>
> Signed-off-by: Daniel Vetter <[e-mail]>
> ---

This is something we definitely need, all drivers need to follow the same rules, in order to put some light in the darkness. :)

Reviewed-by: Maarten Lankhorst <[e-mail]>

>  drivers/dma-buf/dma-fence.c | 53 +++++++++++++++++++++++++++++++++++++
>  include/linux/dma-fence.h   | 12 +++++++++
>  2 files changed, 65 insertions(+)
>
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 6802125349fb..d5c0fd2efc70 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -110,6 +110,52 @@ u64 dma_fence_context_alloc(unsigned num)
>  }
>  EXPORT_SYMBOL(dma_fence_context_alloc);
>  
> +#ifdef CONFIG_LOCKDEP
> +struct lockdep_map	dma_fence_lockdep_map = {
> +	.name = "dma_fence_map"
> +};
> +
> +bool dma_fence_begin_signalling(void)
> +{
> +	/* explicitly nesting ... */
> +	if (lock_is_held_type(&dma_fence_lockdep_map, 1))
> +		return true;
> +
> +	/* rely on might_sleep check for soft/hardirq locks */
> +	if (in_atomic())
> +		return true;
> +
> +	/* ... and non-recursive readlock */
> +	lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, NULL, _RET_IP_);
> +
> +	return false;
> +}
> +EXPORT_SYMBOL(dma_fence_begin_signalling);
> +
> +void dma_fence_end_signalling(bool cookie)
> +{
> +	if (cookie)
> +		return;
> +
> +	lock_release(&dma_fence_lockdep_map, _RET_IP_);
> +}
> +EXPORT_SYMBOL(dma_fence_end_signalling);
> +
> +void __dma_fence_might_wait(void)
> +{
> +	bool tmp;
> +
> +	tmp = lock_is_held_type(&dma_fence_lockdep_map, 1);
> +	if (tmp)
> +		lock_release(&dma_fence_lockdep_map, _THIS_IP_);
> +	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_);
> +}
> +#endif
> +
> +
>  /**
>   * dma_fence_signal_locked - signal completion of a fence
>   * @fence: the fence to signal
> @@ -170,14 +216,19 @@ int dma_fence_signal(struct dma_fence *fence)
>  {
>  	unsigned long flags;
>  	int ret;
> +	bool tmp;
>  
>  	if (!fence)
>  		return -EINVAL;
>  
> +	tmp = dma_fence_begin_signalling();
> +
>  	spin_lock_irqsave(fence->lock, flags);
>  	ret = dma_fence_signal_locked(fence);
>  	spin_unlock_irqrestore(fence->lock, flags);
>  
> +	dma_fence_end_signalling(tmp);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(dma_fence_signal);
> @@ -211,6 +262,8 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
>  	if (timeout > 0)
>  		might_sleep();
>  
> +	__dma_fence_might_wait();
> +
>  	trace_dma_fence_wait_start(fence);
>  	if (fence->ops->wait)
>  		ret = fence->ops->wait(fence, intr, timeout);
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index 3347c54f3a87..3f288f7db2ef 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -357,6 +357,18 @@ dma_fence_get_rcu_safe(struct dma_fence __rcu **fencep)
>  	} while (1);
>  }
>  
> +#ifdef CONFIG_LOCKDEP
> +bool dma_fence_begin_signalling(void);
> +void dma_fence_end_signalling(bool cookie);
> +#else
> +static inline bool dma_fence_begin_signalling(void)
> +{
> +	return true;
> +}
> +static inline void dma_fence_end_signalling(bool cookie) {}
> +static inline void __dma_fence_might_wait(void) {}
> +#endif
> +
>  int dma_fence_signal(struct dma_fence *fence);
>  int dma_fence_signal_locked(struct dma_fence *fence);
>  signed long dma_fence_default_wait(struct dma_fence *fence,

Thomas Hellström (Intel) May 28, 2020, 1:36 p.m. UTC | #7

On 2020-05-12 10:59, Daniel Vetter wrote:
> Design is similar to the lockdep annotations for workers, but with
> some twists:
>
> - We use a read-lock for the execution/worker/completion side, so that
>    this explicit annotation can be more liberally sprinkled around.
>    With read locks lockdep isn't going to complain if the read-side
>    isn't nested the same way under all circumstances, so ABBA deadlocks
>    are ok. Which they are, since this is an annotation only.
>
> - We're using non-recursive lockdep read lock mode, since in recursive
>    read lock mode lockdep does not catch read side hazards. And we
>    _very_ much want read side hazards to be caught. For full details of
>    this limitation see
>
>    commit e91498589746065e3ae95d9a00b068e525eec34f
>    Author: Peter Zijlstra <[e-mail]>
>    Date:   Wed Aug 23 13:13:11 2017 +0200
>
>        locking/lockdep/selftests: Add mixed read-write ABBA tests
>
> - To allow nesting of the read-side explicit annotations we explicitly
>    keep track of the nesting. lock_is_held() allows us to do that.
>
> - The wait-side annotation is a write lock, and entirely done within
>    dma_fence_wait() for everyone by default.
>
> - To be able to freely annotate helper functions I want to make it ok
>    to call dma_fence_begin/end_signalling from soft/hardirq context.
>    First attempt was using the hardirq locking context for the write
>    side in lockdep, but this forces all normal spinlocks nested within
>    dma_fence_begin/end_signalling to be spinlocks. That bollocks.
>
>    The approach now is to simple check in_atomic(), and for these cases
>    entirely rely on the might_sleep() check in dma_fence_wait(). That
>    will catch any wrong nesting against spinlocks from soft/hardirq
>    contexts.
>
> The idea here is that every code path that's critical for eventually
> signalling a dma_fence should be annotated with
> dma_fence_begin/end_signalling. The annotation ideally starts right
> after a dma_fence is published (added to a dma_resv, exposed as a
> sync_file fd, attached to a drm_syncobj fd, or anything else that
> makes the dma_fence visible to other kernel threads), up to and
> including the dma_fence_wait(). Examples are irq handlers, the
> scheduler rt threads, the tail of execbuf (after the corresponding
> fences are visible), any workers that end up signalling dma_fences and
> really anything else. Not annotated should be code paths that only
> complete fences opportunistically as the gpu progresses, like e.g.
> shrinker/eviction code.
>
> The main class of deadlocks this is supposed to catch are:
>
> Thread A:
>
> 	mutex_lock(A);
> 	mutex_unlock(A);
>
> 	dma_fence_signal();
>
> Thread B:
>
> 	mutex_lock(A);
> 	dma_fence_wait();
> 	mutex_unlock(A);
>
> Thread B is blocked on A signalling the fence, but A never gets around
> to that because it cannot acquire the lock A.
>
> Note that dma_fence_wait() is allowed to be nested within
> dma_fence_begin/end_signalling sections. To allow this to happen the
> read lock needs to be upgraded to a write lock, which means that any
> other lock is acquired between the dma_fence_begin_signalling() call and
> the call to dma_fence_wait(), and still held, this will result in an
> immediate lockdep complaint. The only other option would be to not
> annotate such calls, defeating the point. Therefore these annotations
> cannot be sprinkled over the code entirely mindless to avoid false
> positives.
>
> v2: handle soft/hardirq ctx better against write side and dont forget
> EXPORT_SYMBOL, drivers can't use this otherwise.
>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: Chris Wilson <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Christian König <[e-mail]>
> Signed-off-by: Daniel Vetter <[e-mail]>

LGTM. Perhaps some in-code documentation on how to use the new functions 
are called.

Otherwise for patch 2 and 3,

Reviewed-by: Thomas Hellstrom <[e-mail]>

Daniel Vetter May 28, 2020, 2:22 p.m. UTC | #8

On Thu, May 28, 2020 at 3:37 PM Thomas Hellström (Intel)
<[e-mail]> wrote:
>
> On 2020-05-12 10:59, Daniel Vetter wrote:
> > Design is similar to the lockdep annotations for workers, but with
> > some twists:
> >
> > - We use a read-lock for the execution/worker/completion side, so that
> >    this explicit annotation can be more liberally sprinkled around.
> >    With read locks lockdep isn't going to complain if the read-side
> >    isn't nested the same way under all circumstances, so ABBA deadlocks
> >    are ok. Which they are, since this is an annotation only.
> >
> > - We're using non-recursive lockdep read lock mode, since in recursive
> >    read lock mode lockdep does not catch read side hazards. And we
> >    _very_ much want read side hazards to be caught. For full details of
> >    this limitation see
> >
> >    commit e91498589746065e3ae95d9a00b068e525eec34f
> >    Author: Peter Zijlstra <[e-mail]>
> >    Date:   Wed Aug 23 13:13:11 2017 +0200
> >
> >        locking/lockdep/selftests: Add mixed read-write ABBA tests
> >
> > - To allow nesting of the read-side explicit annotations we explicitly
> >    keep track of the nesting. lock_is_held() allows us to do that.
> >
> > - The wait-side annotation is a write lock, and entirely done within
> >    dma_fence_wait() for everyone by default.
> >
> > - To be able to freely annotate helper functions I want to make it ok
> >    to call dma_fence_begin/end_signalling from soft/hardirq context.
> >    First attempt was using the hardirq locking context for the write
> >    side in lockdep, but this forces all normal spinlocks nested within
> >    dma_fence_begin/end_signalling to be spinlocks. That bollocks.
> >
> >    The approach now is to simple check in_atomic(), and for these cases
> >    entirely rely on the might_sleep() check in dma_fence_wait(). That
> >    will catch any wrong nesting against spinlocks from soft/hardirq
> >    contexts.
> >
> > The idea here is that every code path that's critical for eventually
> > signalling a dma_fence should be annotated with
> > dma_fence_begin/end_signalling. The annotation ideally starts right
> > after a dma_fence is published (added to a dma_resv, exposed as a
> > sync_file fd, attached to a drm_syncobj fd, or anything else that
> > makes the dma_fence visible to other kernel threads), up to and
> > including the dma_fence_wait(). Examples are irq handlers, the
> > scheduler rt threads, the tail of execbuf (after the corresponding
> > fences are visible), any workers that end up signalling dma_fences and
> > really anything else. Not annotated should be code paths that only
> > complete fences opportunistically as the gpu progresses, like e.g.
> > shrinker/eviction code.
> >
> > The main class of deadlocks this is supposed to catch are:
> >
> > Thread A:
> >
> >       mutex_lock(A);
> >       mutex_unlock(A);
> >
> >       dma_fence_signal();
> >
> > Thread B:
> >
> >       mutex_lock(A);
> >       dma_fence_wait();
> >       mutex_unlock(A);
> >
> > Thread B is blocked on A signalling the fence, but A never gets around
> > to that because it cannot acquire the lock A.
> >
> > Note that dma_fence_wait() is allowed to be nested within
> > dma_fence_begin/end_signalling sections. To allow this to happen the
> > read lock needs to be upgraded to a write lock, which means that any
> > other lock is acquired between the dma_fence_begin_signalling() call and
> > the call to dma_fence_wait(), and still held, this will result in an
> > immediate lockdep complaint. The only other option would be to not
> > annotate such calls, defeating the point. Therefore these annotations
> > cannot be sprinkled over the code entirely mindless to avoid false
> > positives.
> >
> > v2: handle soft/hardirq ctx better against write side and dont forget
> > EXPORT_SYMBOL, drivers can't use this otherwise.
> >
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: Chris Wilson <[e-mail]>
> > Cc: Maarten Lankhorst <[e-mail]>
> > Cc: Christian König <[e-mail]>
> > Signed-off-by: Daniel Vetter <[e-mail]>
>
> LGTM. Perhaps some in-code documentation on how to use the new functions
> are called.

See cover letter, that's going to be done for next round. For this one
here I just wanted to showcase a bit how it's used in a few different
places, mostly selected to get as much feedback from across different
drivers. Hence e.g. annotating drm/scheduler.

> Otherwise for patch 2 and 3,
>
> Reviewed-by: Thomas Hellstrom <[e-mail]>

I think I'll just cc you for the next round with docs, so you can make
sure it looks ok :-)
-Daniel

Luben Tuikov May 28, 2020, 9:54 p.m. UTC | #9

On 2020-05-12 4:59 a.m., Daniel Vetter wrote:
> Design is similar to the lockdep annotations for workers, but with
> some twists:
> 
> - We use a read-lock for the execution/worker/completion side, so that
>   this explicit annotation can be more liberally sprinkled around.
>   With read locks lockdep isn't going to complain if the read-side
>   isn't nested the same way under all circumstances, so ABBA deadlocks
>   are ok. Which they are, since this is an annotation only.
> 
> - We're using non-recursive lockdep read lock mode, since in recursive
>   read lock mode lockdep does not catch read side hazards. And we
>   _very_ much want read side hazards to be caught. For full details of
>   this limitation see
> 
>   commit e91498589746065e3ae95d9a00b068e525eec34f
>   Author: Peter Zijlstra <[e-mail]>
>   Date:   Wed Aug 23 13:13:11 2017 +0200
> 
>       locking/lockdep/selftests: Add mixed read-write ABBA tests
> 
> - To allow nesting of the read-side explicit annotations we explicitly
>   keep track of the nesting. lock_is_held() allows us to do that.
> 
> - The wait-side annotation is a write lock, and entirely done within
>   dma_fence_wait() for everyone by default.
> 
> - To be able to freely annotate helper functions I want to make it ok
>   to call dma_fence_begin/end_signalling from soft/hardirq context.
>   First attempt was using the hardirq locking context for the write
>   side in lockdep, but this forces all normal spinlocks nested within
>   dma_fence_begin/end_signalling to be spinlocks. That bollocks.
> 
>   The approach now is to simple check in_atomic(), and for these cases
>   entirely rely on the might_sleep() check in dma_fence_wait(). That
>   will catch any wrong nesting against spinlocks from soft/hardirq
>   contexts.
> 
> The idea here is that every code path that's critical for eventually
> signalling a dma_fence should be annotated with
> dma_fence_begin/end_signalling. The annotation ideally starts right
> after a dma_fence is published (added to a dma_resv, exposed as a
> sync_file fd, attached to a drm_syncobj fd, or anything else that
> makes the dma_fence visible to other kernel threads), up to and
> including the dma_fence_wait(). Examples are irq handlers, the
> scheduler rt threads, the tail of execbuf (after the corresponding
> fences are visible), any workers that end up signalling dma_fences and
> really anything else. Not annotated should be code paths that only
> complete fences opportunistically as the gpu progresses, like e.g.
> shrinker/eviction code.
> 
> The main class of deadlocks this is supposed to catch are:
> 
> Thread A:
> 
> 	mutex_lock(A);
> 	mutex_unlock(A);
> 
> 	dma_fence_signal();
> 
> Thread B:
> 
> 	mutex_lock(A);
> 	dma_fence_wait();
> 	mutex_unlock(A);
> 
> Thread B is blocked on A signalling the fence, but A never gets around
> to that because it cannot acquire the lock A.
> 
> Note that dma_fence_wait() is allowed to be nested within
> dma_fence_begin/end_signalling sections. To allow this to happen the
> read lock needs to be upgraded to a write lock, which means that any
> other lock is acquired between the dma_fence_begin_signalling() call and
> the call to dma_fence_wait(), and still held, this will result in an
> immediate lockdep complaint. The only other option would be to not
> annotate such calls, defeating the point. Therefore these annotations
> cannot be sprinkled over the code entirely mindless to avoid false
> positives.
> 
> v2: handle soft/hardirq ctx better against write side and dont forget
> EXPORT_SYMBOL, drivers can't use this otherwise.
> 
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: Chris Wilson <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Christian König <[e-mail]>
> Signed-off-by: Daniel Vetter <[e-mail]>
> ---
>  drivers/dma-buf/dma-fence.c | 53 +++++++++++++++++++++++++++++++++++++
>  include/linux/dma-fence.h   | 12 +++++++++
>  2 files changed, 65 insertions(+)
> 
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 6802125349fb..d5c0fd2efc70 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -110,6 +110,52 @@ u64 dma_fence_context_alloc(unsigned num)
>  }
>  EXPORT_SYMBOL(dma_fence_context_alloc);
>  
> +#ifdef CONFIG_LOCKDEP
> +struct lockdep_map	dma_fence_lockdep_map = {
> +	.name = "dma_fence_map"
> +};
> +
> +bool dma_fence_begin_signalling(void)
> +{
> +	/* explicitly nesting ... */
> +	if (lock_is_held_type(&dma_fence_lockdep_map, 1))
> +		return true;
> +
> +	/* rely on might_sleep check for soft/hardirq locks */
> +	if (in_atomic())
> +		return true;
> +
> +	/* ... and non-recursive readlock */
> +	lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, NULL, _RET_IP_);
> +
> +	return false;
> +}
> +EXPORT_SYMBOL(dma_fence_begin_signalling);

Hi Daniel,

This is great work and could help a lot.

If you invert the result of dma_fence_begin_signalling()
then it would naturally mean "locked", i.e. whether we need to
later release "dma_fence_lockedep_map". Then,
in dma_fence_end_signalling(), you can call the "cookie"
argument "locked" and simply do:

void dma_fence_end_signalling(bool locked)
{
	if (locked)
		lock_release(&dma_fence_lockdep_map, _RET_IP_);
}
EXPORT_SYMBOL(dma_fence_end_signalling);

It'll be more natural to understand as well.

Regards,
Luben

> +
> +void dma_fence_end_signalling(bool cookie)
> +{
> +	if (cookie)
> +		return;
> +
> +	lock_release(&dma_fence_lockdep_map, _RET_IP_);
> +}
> +EXPORT_SYMBOL(dma_fence_end_signalling);
> +
> +void __dma_fence_might_wait(void)
> +{
> +	bool tmp;
> +
> +	tmp = lock_is_held_type(&dma_fence_lockdep_map, 1);
> +	if (tmp)
> +		lock_release(&dma_fence_lockdep_map, _THIS_IP_);
> +	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_);
> +}
> +#endif
> +
> +
>  /**
>   * dma_fence_signal_locked - signal completion of a fence
>   * @fence: the fence to signal
> @@ -170,14 +216,19 @@ int dma_fence_signal(struct dma_fence *fence)
>  {
>  	unsigned long flags;
>  	int ret;
> +	bool tmp;
>  
>  	if (!fence)
>  		return -EINVAL;
>  
> +	tmp = dma_fence_begin_signalling();
> +
>  	spin_lock_irqsave(fence->lock, flags);
>  	ret = dma_fence_signal_locked(fence);
>  	spin_unlock_irqrestore(fence->lock, flags);
>  
> +	dma_fence_end_signalling(tmp);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(dma_fence_signal);
> @@ -211,6 +262,8 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
>  	if (timeout > 0)
>  		might_sleep();
>  
> +	__dma_fence_might_wait();
> +
>  	trace_dma_fence_wait_start(fence);
>  	if (fence->ops->wait)
>  		ret = fence->ops->wait(fence, intr, timeout);
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index 3347c54f3a87..3f288f7db2ef 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -357,6 +357,18 @@ dma_fence_get_rcu_safe(struct dma_fence __rcu **fencep)
>  	} while (1);
>  }
>  
> +#ifdef CONFIG_LOCKDEP
> +bool dma_fence_begin_signalling(void);
> +void dma_fence_end_signalling(bool cookie);
> +#else
> +static inline bool dma_fence_begin_signalling(void)
> +{
> +	return true;
> +}
> +static inline void dma_fence_end_signalling(bool cookie) {}
> +static inline void __dma_fence_might_wait(void) {}
> +#endif
> +
>  int dma_fence_signal(struct dma_fence *fence);
>  int dma_fence_signal_locked(struct dma_fence *fence);
>  signed long dma_fence_default_wait(struct dma_fence *fence,
>

Daniel Vetter May 29, 2020, 5:49 a.m. UTC | #10

On Thu, May 28, 2020 at 11:54 PM Luben Tuikov <[e-mail]> wrote:
>
> On 2020-05-12 4:59 a.m., Daniel Vetter wrote:
> > Design is similar to the lockdep annotations for workers, but with
> > some twists:
> >
> > - We use a read-lock for the execution/worker/completion side, so that
> >   this explicit annotation can be more liberally sprinkled around.
> >   With read locks lockdep isn't going to complain if the read-side
> >   isn't nested the same way under all circumstances, so ABBA deadlocks
> >   are ok. Which they are, since this is an annotation only.
> >
> > - We're using non-recursive lockdep read lock mode, since in recursive
> >   read lock mode lockdep does not catch read side hazards. And we
> >   _very_ much want read side hazards to be caught. For full details of
> >   this limitation see
> >
> >   commit e91498589746065e3ae95d9a00b068e525eec34f
> >   Author: Peter Zijlstra <[e-mail]>
> >   Date:   Wed Aug 23 13:13:11 2017 +0200
> >
> >       locking/lockdep/selftests: Add mixed read-write ABBA tests
> >
> > - To allow nesting of the read-side explicit annotations we explicitly
> >   keep track of the nesting. lock_is_held() allows us to do that.
> >
> > - The wait-side annotation is a write lock, and entirely done within
> >   dma_fence_wait() for everyone by default.
> >
> > - To be able to freely annotate helper functions I want to make it ok
> >   to call dma_fence_begin/end_signalling from soft/hardirq context.
> >   First attempt was using the hardirq locking context for the write
> >   side in lockdep, but this forces all normal spinlocks nested within
> >   dma_fence_begin/end_signalling to be spinlocks. That bollocks.
> >
> >   The approach now is to simple check in_atomic(), and for these cases
> >   entirely rely on the might_sleep() check in dma_fence_wait(). That
> >   will catch any wrong nesting against spinlocks from soft/hardirq
> >   contexts.
> >
> > The idea here is that every code path that's critical for eventually
> > signalling a dma_fence should be annotated with
> > dma_fence_begin/end_signalling. The annotation ideally starts right
> > after a dma_fence is published (added to a dma_resv, exposed as a
> > sync_file fd, attached to a drm_syncobj fd, or anything else that
> > makes the dma_fence visible to other kernel threads), up to and
> > including the dma_fence_wait(). Examples are irq handlers, the
> > scheduler rt threads, the tail of execbuf (after the corresponding
> > fences are visible), any workers that end up signalling dma_fences and
> > really anything else. Not annotated should be code paths that only
> > complete fences opportunistically as the gpu progresses, like e.g.
> > shrinker/eviction code.
> >
> > The main class of deadlocks this is supposed to catch are:
> >
> > Thread A:
> >
> >       mutex_lock(A);
> >       mutex_unlock(A);
> >
> >       dma_fence_signal();
> >
> > Thread B:
> >
> >       mutex_lock(A);
> >       dma_fence_wait();
> >       mutex_unlock(A);
> >
> > Thread B is blocked on A signalling the fence, but A never gets around
> > to that because it cannot acquire the lock A.
> >
> > Note that dma_fence_wait() is allowed to be nested within
> > dma_fence_begin/end_signalling sections. To allow this to happen the
> > read lock needs to be upgraded to a write lock, which means that any
> > other lock is acquired between the dma_fence_begin_signalling() call and
> > the call to dma_fence_wait(), and still held, this will result in an
> > immediate lockdep complaint. The only other option would be to not
> > annotate such calls, defeating the point. Therefore these annotations
> > cannot be sprinkled over the code entirely mindless to avoid false
> > positives.
> >
> > v2: handle soft/hardirq ctx better against write side and dont forget
> > EXPORT_SYMBOL, drivers can't use this otherwise.
> >
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: Chris Wilson <[e-mail]>
> > Cc: Maarten Lankhorst <[e-mail]>
> > Cc: Christian König <[e-mail]>
> > Signed-off-by: Daniel Vetter <[e-mail]>
> > ---
> >  drivers/dma-buf/dma-fence.c | 53 +++++++++++++++++++++++++++++++++++++
> >  include/linux/dma-fence.h   | 12 +++++++++
> >  2 files changed, 65 insertions(+)
> >
> > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > index 6802125349fb..d5c0fd2efc70 100644
> > --- a/drivers/dma-buf/dma-fence.c
> > +++ b/drivers/dma-buf/dma-fence.c
> > @@ -110,6 +110,52 @@ u64 dma_fence_context_alloc(unsigned num)
> >  }
> >  EXPORT_SYMBOL(dma_fence_context_alloc);
> >
> > +#ifdef CONFIG_LOCKDEP
> > +struct lockdep_map   dma_fence_lockdep_map = {
> > +     .name = "dma_fence_map"
> > +};
> > +
> > +bool dma_fence_begin_signalling(void)
> > +{
> > +     /* explicitly nesting ... */
> > +     if (lock_is_held_type(&dma_fence_lockdep_map, 1))
> > +             return true;
> > +
> > +     /* rely on might_sleep check for soft/hardirq locks */
> > +     if (in_atomic())
> > +             return true;
> > +
> > +     /* ... and non-recursive readlock */
> > +     lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, NULL, _RET_IP_);
> > +
> > +     return false;
> > +}
> > +EXPORT_SYMBOL(dma_fence_begin_signalling);
>
> Hi Daniel,
>
> This is great work and could help a lot.
>
> If you invert the result of dma_fence_begin_signalling()
> then it would naturally mean "locked", i.e. whether we need to
> later release "dma_fence_lockedep_map". Then,
> in dma_fence_end_signalling(), you can call the "cookie"
> argument "locked" and simply do:
>
> void dma_fence_end_signalling(bool locked)
> {
>         if (locked)
>                 lock_release(&dma_fence_lockdep_map, _RET_IP_);
> }
> EXPORT_SYMBOL(dma_fence_end_signalling);
>
> It'll be more natural to understand as well.

It's intentionally called cookie so callers don't start doing funny
stuff with it. The thing is, after begin_signalling you are _always_
in the locked state. It's just that because of limitations with
lockdep we need to play a few tricks, and in some cases we do not take
the lockdep map. There's 2 cases:
- lockdep map already taken - we want recursive readlock semantics for
this, but lockdep does not correctly check recursive read locks. Hence
we only use readlock, and make sure we do not actually nest upon
ourselves with this explicit check.
- when we're in atomic sections - lockdep gets pissed at us if we take
the read lock in hard/softirq sections because of hard/softirq ctx
mismatch (lockdep thinks it's a real lock, but we don't treat it as
one). Simplest fix was to rely on the might_sleep check in patch 1
(already merged)

The commit message mentions this already a bit, but I'll try to
explain this implementation detail tersely in the kerneldoc too in the
next round.

Thanks, Daniel

>
> Regards,
> Luben
>
> > +
> > +void dma_fence_end_signalling(bool cookie)
> > +{
> > +     if (cookie)
> > +             return;
> > +
> > +     lock_release(&dma_fence_lockdep_map, _RET_IP_);
> > +}
> > +EXPORT_SYMBOL(dma_fence_end_signalling);
> > +
> > +void __dma_fence_might_wait(void)
> > +{
> > +     bool tmp;
> > +
> > +     tmp = lock_is_held_type(&dma_fence_lockdep_map, 1);
> > +     if (tmp)
> > +             lock_release(&dma_fence_lockdep_map, _THIS_IP_);
> > +     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_);
> > +}
> > +#endif
> > +
> > +
> >  /**
> >   * dma_fence_signal_locked - signal completion of a fence
> >   * @fence: the fence to signal
> > @@ -170,14 +216,19 @@ int dma_fence_signal(struct dma_fence *fence)
> >  {
> >       unsigned long flags;
> >       int ret;
> > +     bool tmp;
> >
> >       if (!fence)
> >               return -EINVAL;
> >
> > +     tmp = dma_fence_begin_signalling();
> > +
> >       spin_lock_irqsave(fence->lock, flags);
> >       ret = dma_fence_signal_locked(fence);
> >       spin_unlock_irqrestore(fence->lock, flags);
> >
> > +     dma_fence_end_signalling(tmp);
> > +
> >       return ret;
> >  }
> >  EXPORT_SYMBOL(dma_fence_signal);
> > @@ -211,6 +262,8 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
> >       if (timeout > 0)
> >               might_sleep();
> >
> > +     __dma_fence_might_wait();
> > +
> >       trace_dma_fence_wait_start(fence);
> >       if (fence->ops->wait)
> >               ret = fence->ops->wait(fence, intr, timeout);
> > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> > index 3347c54f3a87..3f288f7db2ef 100644
> > --- a/include/linux/dma-fence.h
> > +++ b/include/linux/dma-fence.h
> > @@ -357,6 +357,18 @@ dma_fence_get_rcu_safe(struct dma_fence __rcu **fencep)
> >       } while (1);
> >  }
> >
> > +#ifdef CONFIG_LOCKDEP
> > +bool dma_fence_begin_signalling(void);
> > +void dma_fence_end_signalling(bool cookie);
> > +#else
> > +static inline bool dma_fence_begin_signalling(void)
> > +{
> > +     return true;
> > +}
> > +static inline void dma_fence_end_signalling(bool cookie) {}
> > +static inline void __dma_fence_might_wait(void) {}
> > +#endif
> > +
> >  int dma_fence_signal(struct dma_fence *fence);
> >  int dma_fence_signal_locked(struct dma_fence *fence);
> >  signed long dma_fence_default_wait(struct dma_fence *fence,
> >
>

Patch

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 6802125349fb..d5c0fd2efc70 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -110,6 +110,52 @@  u64 dma_fence_context_alloc(unsigned num)
 }
 EXPORT_SYMBOL(dma_fence_context_alloc);
 
+#ifdef CONFIG_LOCKDEP
+struct lockdep_map	dma_fence_lockdep_map = {
+	.name = "dma_fence_map"
+};
+
+bool dma_fence_begin_signalling(void)
+{
+	/* explicitly nesting ... */
+	if (lock_is_held_type(&dma_fence_lockdep_map, 1))
+		return true;
+
+	/* rely on might_sleep check for soft/hardirq locks */
+	if (in_atomic())
+		return true;
+
+	/* ... and non-recursive readlock */
+	lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, NULL, _RET_IP_);
+
+	return false;
+}
+EXPORT_SYMBOL(dma_fence_begin_signalling);
+
+void dma_fence_end_signalling(bool cookie)
+{
+	if (cookie)
+		return;
+
+	lock_release(&dma_fence_lockdep_map, _RET_IP_);
+}
+EXPORT_SYMBOL(dma_fence_end_signalling);
+
+void __dma_fence_might_wait(void)
+{
+	bool tmp;
+
+	tmp = lock_is_held_type(&dma_fence_lockdep_map, 1);
+	if (tmp)
+		lock_release(&dma_fence_lockdep_map, _THIS_IP_);
+	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_);
+}
+#endif
+
+
 /**
  * dma_fence_signal_locked - signal completion of a fence
  * @fence: the fence to signal
@@ -170,14 +216,19 @@  int dma_fence_signal(struct dma_fence *fence)
 {
 	unsigned long flags;
 	int ret;
+	bool tmp;
 
 	if (!fence)
 		return -EINVAL;
 
+	tmp = dma_fence_begin_signalling();
+
 	spin_lock_irqsave(fence->lock, flags);
 	ret = dma_fence_signal_locked(fence);
 	spin_unlock_irqrestore(fence->lock, flags);
 
+	dma_fence_end_signalling(tmp);
+
 	return ret;
 }
 EXPORT_SYMBOL(dma_fence_signal);
@@ -211,6 +262,8 @@  dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
 	if (timeout > 0)
 		might_sleep();
 
+	__dma_fence_might_wait();
+
 	trace_dma_fence_wait_start(fence);
 	if (fence->ops->wait)
 		ret = fence->ops->wait(fence, intr, timeout);
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 3347c54f3a87..3f288f7db2ef 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -357,6 +357,18 @@  dma_fence_get_rcu_safe(struct dma_fence __rcu **fencep)
 	} while (1);
 }
 
+#ifdef CONFIG_LOCKDEP
+bool dma_fence_begin_signalling(void);
+void dma_fence_end_signalling(bool cookie);
+#else
+static inline bool dma_fence_begin_signalling(void)
+{
+	return true;
+}
+static inline void dma_fence_end_signalling(bool cookie) {}
+static inline void __dma_fence_might_wait(void) {}
+#endif
+
 int dma_fence_signal(struct dma_fence *fence);
 int dma_fence_signal_locked(struct dma_fence *fence);
 signed long dma_fence_default_wait(struct dma_fence *fence,