[Intel-gfx] [PATCH v2 8/9] drm/i915: Stop rcu support for i915_address_space
Daniel Vetter
daniel.vetter at ffwll.ch
Wed Aug 4 14:25:21 UTC 2021
The full audit is quite a bit of work:
- i915_dpt has very simple lifetime (somehow we create a display pagetable vm
per object, so its _very_ simple, there's only ever a single vma in there),
and uses i915_vm_close(), which internally does a i915_vm_put(). No rcu.
Aside: wtf is i915_dpt doing in the intel_display.c garbage collector as a new
feature, instead of added as a separate file with some clean-ish interface.
Also, i915_dpt unfortunately re-introduces some coding patterns from
pre-dma_resv_lock conversion times.
- i915_gem_proto_ctx is fully refcounted and no rcu, all protected by
fpriv->proto_context_lock.
- i915_gem_context is itself rcu protected, and that might leak to anything it
points at. Before
commit cf977e18610e66e48c31619e7e0cfa871be9eada
Author: Chris Wilson <chris at chris-wilson.co.uk>
Date: Wed Dec 2 11:21:40 2020 +0000
drm/i915/gem: Spring clean debugfs
and
commit db80a1294c231b6ac725085f046bb2931e00c9db
Author: Chris Wilson <chris at chris-wilson.co.uk>
Date: Mon Jan 18 11:08:54 2021 +0000
drm/i915/gem: Remove per-client stats from debugfs/i915_gem_objects
we had a bunch of debugfs files that relied on rcu protecting everything, but
those are gone now. The main one was removed even earlier with
There doesn't seem to be anything left that's actually protecting
stuff now that the ctx->vm itself is invariant. See
commit ccbc1b97948ab671335e950271e39766729736c3
Author: Jason Ekstrand <jason at jlekstrand.net>
Date: Thu Jul 8 10:48:30 2021 -0500
drm/i915/gem: Don't allow changing the VM on running contexts (v4)
Note that we drop the vm refcount before the final release of the gem context
refcount, so this is all very dangerous even without rcu. Note that aside from
later on creating new engines (a defunct feature) and debug output we're never
looked at gem_ctx->vm for anything functional, hence why this is ok.
Fingers crossed.
Preceeding patches removed all vestiges of rcu use from gem_ctx->vm
derferencing to make it clear it's really not used.
The gem_ctx->rcu protection was introduced in
commit a4e7ccdac38ec8335d9e4e2656c1a041c77feae1
Author: Chris Wilson <chris at chris-wilson.co.uk>
Date: Fri Oct 4 14:40:09 2019 +0100
drm/i915: Move context management under GEM
The commit message is somewhat entertaining because it fails to
mention this fact completely, and compensates that by an in-commit
changelog entry that claims that ctx->vm is protected by ctx->mutex.
Which was the case _before_ this commit, but no longer after it.
- intel_context holds a full reference. Unfortunately intel_context is also rcu
protected and the reference to the ->vm is dropped before the
rcu barrier - only the kfree is delayed. So again we need to check
whether that leaks anywhere on the intel_context->vm. RCU is only
used to protect intel_context sitting on the breadcrumb lists, which
don't look at the vm anywhere, so we are fine.
Nothing else relies on rcu protection of intel_context and hence is
fully protected by the kref refcount alone, which protects
intel_context->vm in turn.
The breadcrumbs rcu usage was added in
commit c744d50363b714783bbc88d986cc16def13710f7
Author: Chris Wilson <chris at chris-wilson.co.uk>
Date: Thu Nov 26 14:04:06 2020 +0000
drm/i915/gt: Split the breadcrumb spinlock between global and contexts
its parent commit added the intel_context rcu protection:
commit 14d1eaf08845c534963c83f754afe0cb14cb2512
Author: Chris Wilson <chris at chris-wilson.co.uk>
Date: Thu Nov 26 14:04:05 2020 +0000
drm/i915/gt: Protect context lifetime with RCU
given some credence to my claim that I've actually caught them all.
- drm_i915_gem_object's shares_resv_from pointer has a full refcount to the
dma_resv, which is a sub-refcount that's released after the final
i915_vm_put() has been called. Safe.
Aside: Maybe we should have a struct dma_resv_shared which is just dma_resv +
kref as a stand-alone thing. It's a pretty useful pattern which other drivers
might want to copy.
For a bit more context see
commit 4d8151ae5329cf50781a02fd2298a909589a5bab
Author: Thomas Hellström <thomas.hellstrom at linux.intel.com>
Date: Tue Jun 1 09:46:41 2021 +0200
drm/i915: Don't free shared locks while shared
- the fpriv->vm_xa was relying on rcu_read_lock for lookup, but that
was updated in a prep patch too to just be a spinlock-protected
lookup.
- intel_gt->vm is set at driver load in intel_gt_init() and released
in intel_gt_driver_release(). There seems to be some issue that
in some error paths this is called twice, but otherwise no rcu to be
found anywhere. This was added in the below commit, which
unfortunately doesn't explain why this complication exists.
commit e6ba76480299a0d77c51d846f7467b1673aad25b
Author: Chris Wilson <chris at chris-wilson.co.uk>
Date: Sat Dec 21 16:03:24 2019 +0000
drm/i915: Remove i915->kernel_context
The proper fix most likely for this is to start using drmm_ at large
scale, but that's also huge amounts of work.
- i915_vma->vm is some real pain, because rcu is rcu protected, at
least in the vma lookup in the context lookup cache in
eb_lookup_vma(). This was added in
commit 4ff4b44cbb70c269259958cbcc48d7b8a2cb9ec8
Author: Chris Wilson <chris at chris-wilson.co.uk>
Date: Fri Jun 16 15:05:16 2017 +0100
drm/i915: Store a direct lookup from object handle to vma
This was changed to a radix tree from the hashtable in, but with the
locking unchanged, in
commit d1b48c1e7184d9bc4ae6d7f9fe2eed9efed11ffc
Author: Chris Wilson <chris at chris-wilson.co.uk>
Date: Wed Aug 16 09:52:08 2017 +0100
drm/i915: Replace execbuf vma ht with an idr
In
commit 93159e12353c2a47e5576d642845a91fa00530bf
Author: Chris Wilson <chris at chris-wilson.co.uk>
Date: Mon Mar 23 09:28:41 2020 +0000
drm/i915/gem: Avoid gem_context->mutex for simple vma lookup
the locking was changed from dev->struct_mutex to rcu, which added
the requirement to rcu protect i915_vma. Somehow this was missed in
review (or I'm completely blind).
Irrespective of all that the vma lookup cache rcu_read_lock grabs a
full reference of the vma and the rcu doesn't leak further. So no
impact on i915_address_space from that.
I have not found any other rcu use for i915_vma, but given that it
seems broken I also didn't bother to do a careful in-depth audit.
Alltogether there's nothing left in-tree anymore which requires that a
pointer deref to an i915_address_space is safe undre rcu_read_lock
only.
rcu protection of i915_address_space was introduced in
commit b32fa811156328aea5a3c2ff05cc096490382456
Author: Chris Wilson <chris at chris-wilson.co.uk>
Date: Thu Jun 20 19:37:05 2019 +0100
drm/i915/gtt: Defer address space cleanup to an RCU worker
by mixing up a bugfixing (i915_address_space needs to be released from
a worker) with enabling rcu support. The commit message also seems
somewhat confused, because it talks about cleanup of WC pages
requiring sleep, while the code and linked bugzilla are about a
requirement to take dev->struct_mutex (which yes sleeps but it's a
much more specific problem). Since final kref_put can be called from
pretty much anywhere (including hardirq context through the
scheduler's i915_active cleanup) we need a worker here. Hence that
part must be kept.
Ideally all these reclaim workers should have some kind of integration
with our shrinkers, but for some of these it's rather tricky. Anyway,
that's a preexisting condition in the codeebase that we wont fix in
this patch here.
We also remove the rcu_barrier in ggtt_cleanup_hw added in
commit 60a4233a4952729089e4df152e730f8f4d0e82ce
Author: Chris Wilson <chris at chris-wilson.co.uk>
Date: Mon Jul 29 14:24:12 2019 +0100
drm/i915: Flush the i915_vm_release before ggtt shutdown
Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
Cc: Jon Bloomfield <jon.bloomfield at intel.com>
Cc: Chris Wilson <chris at chris-wilson.co.uk>
Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
Cc: "Thomas Hellström" <thomas.hellstrom at linux.intel.com>
Cc: Matthew Auld <matthew.auld at intel.com>
Cc: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
Cc: Dave Airlie <airlied at redhat.com>
Cc: Jason Ekstrand <jason at jlekstrand.net>
---
drivers/gpu/drm/i915/gt/intel_ggtt.c | 1 -
drivers/gpu/drm/i915/gt/intel_gtt.c | 6 +++---
drivers/gpu/drm/i915/gt/intel_gtt.h | 2 +-
3 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
index de3ac58fceec..8d71f67926f1 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
@@ -727,7 +727,6 @@ static void ggtt_cleanup_hw(struct i915_ggtt *ggtt)
atomic_set(&ggtt->vm.open, 0);
- rcu_barrier(); /* flush the RCU'ed__i915_vm_release */
flush_workqueue(ggtt->vm.i915->wq);
mutex_lock(&ggtt->vm.mutex);
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c b/drivers/gpu/drm/i915/gt/intel_gtt.c
index e137dd32b5b8..a0c2b952aa57 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
@@ -155,7 +155,7 @@ void i915_vm_resv_release(struct kref *kref)
static void __i915_vm_release(struct work_struct *work)
{
struct i915_address_space *vm =
- container_of(work, struct i915_address_space, rcu.work);
+ container_of(work, struct i915_address_space, release_work);
vm->cleanup(vm);
i915_address_space_fini(vm);
@@ -171,7 +171,7 @@ void i915_vm_release(struct kref *kref)
GEM_BUG_ON(i915_is_ggtt(vm));
trace_i915_ppgtt_release(vm);
- queue_rcu_work(vm->i915->wq, &vm->rcu);
+ queue_work(vm->i915->wq, &vm->release_work);
}
void i915_address_space_init(struct i915_address_space *vm, int subclass)
@@ -185,7 +185,7 @@ void i915_address_space_init(struct i915_address_space *vm, int subclass)
if (!kref_read(&vm->resv_ref))
kref_init(&vm->resv_ref);
- INIT_RCU_WORK(&vm->rcu, __i915_vm_release);
+ INIT_WORK(&vm->release_work, __i915_vm_release);
atomic_set(&vm->open, 1);
/*
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h b/drivers/gpu/drm/i915/gt/intel_gtt.h
index bc7153018ebd..5b539bd7645d 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.h
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
@@ -213,7 +213,7 @@ struct i915_vma_ops {
struct i915_address_space {
struct kref ref;
- struct rcu_work rcu;
+ struct work_struct release_work;
struct drm_mm mm;
struct intel_gt *gt;
--
2.32.0
More information about the Intel-gfx
mailing list