dri-devel Archive on lore.kernel.org
 help / color / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: linux-rdma <linux-rdma@vger.kernel.org>,
	intel-gfx <intel-gfx@lists.freedesktop.org>,
	LKML <linux-kernel@vger.kernel.org>,
	"amd-gfx list" <amd-gfx@lists.freedesktop.org>,
	"moderated list:DMA BUFFER SHARING FRAMEWORK"
	<linaro-mm-sig@lists.linaro.org>,
	"DRI Development" <dri-devel@lists.freedesktop.org>,
	"Dave Airlie" <airlied@redhat.com>,
	"Daniel Vetter" <daniel.vetter@intel.com>,
	"Christian König" <christian.koenig@amd.com>,
	"open list:DMA BUFFER SHARING FRAMEWORK"
	<linux-media@vger.kernel.org>
Subject: Re: [RFC 02/17] dma-fence: basic lockdep annotations
Date: Wed, 13 May 2020 10:30:19 +0200
Message-ID: <CAKMK7uGnFhbpuurRsnZ4dvRV9gQ_3-rmSJaoqSFY=+Kvepz_CA@mail.gmail.com> (raw)
In-Reply-To: <158927519651.15653.17392305363363808831@build.alporthouse.com>

On Tue, May 12, 2020 at 11:19 AM Chris Wilson <chris@chris-wilson.co.uk> 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 <peterz@infradead.org>
> > > >   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: linux-media@vger.kernel.org
> > > > Cc: linaro-mm-sig@lists.linaro.org
> > > > Cc: linux-rdma@vger.kernel.org
> > > > Cc: amd-gfx@lists.freedesktop.org
> > > > Cc: intel-gfx@lists.freedesktop.org
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > Cc: Christian König <christian.koenig@amd.com>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > ---
> > > >  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
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

  reply index

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-12  8:59 [RFC 00/17] dma-fence " Daniel Vetter
2020-05-12  8:59 ` [RFC 01/17] dma-fence: add might_sleep annotation to _wait() Daniel Vetter
2020-05-12  9:03   ` Chris Wilson
2020-05-12  9:08   ` Christian König
2020-06-02  9:45     ` Maarten Lankhorst
2020-05-12  8:59 ` [RFC 02/17] dma-fence: basic lockdep annotations Daniel Vetter
2020-05-12  9:04   ` Chris Wilson
2020-05-12  9:08     ` Daniel Vetter
2020-05-12  9:19       ` Chris Wilson
2020-05-13  8:30         ` Daniel Vetter [this message]
2020-05-25 15:41     ` Daniel Vetter
2020-05-12 12:09   ` Jason Gunthorpe
2020-05-12 12:57     ` Daniel Vetter
2020-05-26 10:00   ` Maarten Lankhorst
2020-05-28 13:36   ` Thomas Hellström (Intel)
2020-05-28 14:22     ` Daniel Vetter
2020-05-28 21:54   ` Luben Tuikov
2020-05-29  5:49     ` Daniel Vetter
2020-05-12  8:59 ` [RFC 03/17] dma-fence: prime " Daniel Vetter
2020-05-12  8:59 ` [RFC 04/17] drm/vkms: Annotate vblank timer Daniel Vetter
2020-05-12  8:59 ` [RFC 05/17] drm/vblank: Annotate with dma-fence signalling section Daniel Vetter
2020-05-12  8:59 ` [RFC 06/17] drm/atomic-helper: Add dma-fence annotations Daniel Vetter
2020-05-12  8:59 ` [RFC 07/17] drm/amdgpu: add dma-fence annotations to atomic commit path Daniel Vetter
2020-05-12  8:59 ` [RFC 08/17] drm/scheduler: use dma-fence annotations in main thread Daniel Vetter
2020-05-25 15:30   ` Daniel Vetter
2020-05-12  8:59 ` [RFC 09/17] drm/amdgpu: use dma-fence annotations in cs_submit() Daniel Vetter
2020-05-13  7:02   ` Christian König
2020-05-13  7:07     ` Daniel Vetter
2020-05-12  8:59 ` [RFC 10/17] drm/amdgpu: s/GFP_KERNEL/GFP_ATOMIC in scheduler code Daniel Vetter
2020-05-12 15:56   ` Christian König
2020-05-12 16:20     ` Daniel Vetter
2020-05-12 16:27       ` Daniel Vetter
2020-05-12 17:31         ` Christian König
2020-05-12 18:34           ` Daniel Vetter
2020-05-12  8:59 ` [RFC 11/17] drm/amdgpu: DC also loves to allocate stuff where it shouldn't Daniel Vetter
2020-05-12  8:59 ` [RFC 12/17] drm/amdgpu/dc: Stop dma_resv_lock inversion in commit_tail Daniel Vetter
2020-05-12  8:59 ` [RFC 13/17] drm/scheduler: use dma-fence annotations in tdr work Daniel Vetter
2020-05-12  8:59 ` [RFC 14/17] drm/amdgpu: use dma-fence annotations for gpu reset code Daniel Vetter
2020-05-12  8:59 ` [RFC 15/17] Revert "drm/amdgpu: add fbdev suspend/resume on gpu reset" Daniel Vetter
2020-05-12  8:59 ` [RFC 16/17] drm/amdgpu: gpu recovery does full modesets Daniel Vetter
2020-05-12 12:54   ` Alex Deucher
2020-05-12 12:58     ` Daniel Vetter
2020-05-12 13:12       ` Alex Deucher
2020-05-12 13:17         ` Daniel Vetter
2020-05-12 13:29           ` Alex Deucher
2020-05-12 13:45             ` Daniel Vetter
2020-05-12 14:24               ` Alex Deucher
2020-05-12 16:12                 ` Daniel Vetter
2020-05-12 20:10                   ` Kazlauskas, Nicholas
2020-05-13  6:02                     ` Daniel Vetter
2020-05-12  8:59 ` [RFC 17/17] drm/i915: Annotate dma_fence_work Daniel Vetter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAKMK7uGnFhbpuurRsnZ4dvRV9gQ_3-rmSJaoqSFY=+Kvepz_CA@mail.gmail.com' \
    --to=daniel@ffwll.ch \
    --cc=airlied@redhat.com \
    --cc=amd-gfx@lists.freedesktop.org \
    --cc=chris@chris-wilson.co.uk \
    --cc=christian.koenig@amd.com \
    --cc=daniel.vetter@intel.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=linaro-mm-sig@lists.linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-rdma@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

dri-devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dri-devel/0 dri-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dri-devel dri-devel/ https://lore.kernel.org/dri-devel \
		dri-devel@lists.freedesktop.org
	public-inbox-index dri-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.freedesktop.lists.dri-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git