[Intel-gfx] [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request
Ruhl, Michael J
michael.j.ruhl at intel.com
Fri Nov 22 20:37:59 UTC 2019
>-----Original Message-----
>From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of Chris
>Wilson
>Sent: Friday, November 22, 2019 2:03 AM
>To: intel-gfx at lists.freedesktop.org
>Subject: [Intel-gfx] [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU
>i915_request
>
>As we start peeking into requests for longer and longer, e.g.
>incorporating use of spinlocks when only protected by an
>rcu_read_lock(), we need to be careful in how we reset the request when
>recycling and need to preserve any barriers that may still be in use as
>the request is reset for reuse.
>
>Quoting Linus Torvalds:
>
>> If there is refcounting going on then why use SLAB_TYPESAFE_BY_RCU?
>
> .. because the object can be accessed (by RCU) after the refcount has
> gone down to zero, and the thing has been released.
>
> That's the whole and only point of SLAB_TYPESAFE_BY_RCU.
>
> That flag basically says:
>
> "I may end up accessing this object *after* it has been free'd,
> because there may be RCU lookups in flight"
>
> This has nothing to do with constructors. It's ok if the object gets
> reused as an object of the same type and does *not* get
> re-initialized, because we're perfectly fine seeing old stale data.
>
> What it guarantees is that the slab isn't shared with any other kind
> of object, _and_ that the underlying pages are free'd after an RCU
> quiescent period (so the pages aren't shared with another kind of
> object either during an RCU walk).
>
> And it doesn't necessarily have to have a constructor, because the
> thing that a RCU walk will care about is
>
> (a) guaranteed to be an object that *has* been on some RCU list (so
> it's not a "new" object)
>
> (b) the RCU walk needs to have logic to verify that it's still the
> *same* object and hasn't been re-used as something else.
>
> In contrast, a SLAB_TYPESAFE_BY_RCU memory gets free'd and re-used
> immediately, but because it gets reused as the same kind of object,
> the RCU walker can "know" what parts have meaning for re-use, in a way
> it couidn't if the re-use was random.
>
> That said, it *is* subtle, and people should be careful.
>
>> So the re-use might initialize the fields lazily, not necessarily using a ctor.
>
> If you have a well-defined refcount, and use "atomic_inc_not_zero()"
> to guard the speculative RCU access section, and use
> "atomic_dec_and_test()" in the freeing section, then you should be
> safe wrt new allocations.
>
> If you have a completely new allocation that has "random stale
> content", you know that it cannot be on the RCU list, so there is no
> speculative access that can ever see that random content.
>
> So the only case you need to worry about is a re-use allocation, and
> you know that the refcount will start out as zero even if you don't
> have a constructor.
>
> So you can think of the refcount itself as always having a zero
> constructor, *BUT* you need to be careful with ordering.
>
> In particular, whoever does the allocation needs to then set the
> refcount to a non-zero value *after* it has initialized all the other
> fields. And in particular, it needs to make sure that it uses the
> proper memory ordering to do so.
>
> NOTE! One thing to be very worried about is that re-initializing
> whatever RCU lists means that now the RCU walker may be walking on the
> wrong list so the walker may do the right thing for this particular
> entry, but it may miss walking *other* entries. So then you can get
> spurious lookup failures, because the RCU walker never walked all the
> way to the end of the right list. That ends up being a much more
> subtle bug.
>
>Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>---
> drivers/gpu/drm/i915/i915_request.c | 49 ++++++++++++++++++---------
> drivers/gpu/drm/i915/i915_scheduler.c | 6 ++++
> drivers/gpu/drm/i915/i915_scheduler.h | 1 +
> drivers/gpu/drm/i915/i915_sw_fence.c | 8 +++++
> drivers/gpu/drm/i915/i915_sw_fence.h | 2 ++
> 5 files changed, 50 insertions(+), 16 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_request.c
>b/drivers/gpu/drm/i915/i915_request.c
>index 00011f9533b6..a558f64186fa 100644
>--- a/drivers/gpu/drm/i915/i915_request.c
>+++ b/drivers/gpu/drm/i915/i915_request.c
>@@ -188,7 +188,7 @@ static void free_capture_list(struct i915_request
>*request)
> {
> struct i915_capture_list *capture;
>
>- capture = request->capture_list;
>+ capture = fetch_and_zero(&request->capture_list);
> while (capture) {
> struct i915_capture_list *next = capture->next;
>
>@@ -214,7 +214,7 @@ static void remove_from_engine(struct i915_request
>*rq)
> spin_lock(&engine->active.lock);
> locked = engine;
> }
>- list_del(&rq->sched.link);
>+ list_del_init(&rq->sched.link);
> spin_unlock_irq(&locked->active.lock);
> }
>
>@@ -586,6 +586,21 @@ request_alloc_slow(struct intel_timeline *tl, gfp_t
>gfp)
> return kmem_cache_alloc(global.slab_requests, gfp);
> }
>
>+static void __i915_request_ctor(void *arg)
>+{
>+ struct i915_request *rq = arg;
>+
>+ spin_lock_init(&rq->lock);
>+ i915_sched_node_init(&rq->sched);
>+ i915_sw_fence_init(&rq->submit, submit_notify);
>+ i915_sw_fence_init(&rq->semaphore, semaphore_notify);
>+
>+ rq->file_priv = NULL;
>+ rq->capture_list = NULL;
>+
>+ INIT_LIST_HEAD(&rq->execute_cb);
>+}
My understanding of the cache ctor is that it will only get invoked when
the cache is expanded. It does not get called on each kmem_cache_alloc().
I.e. if you do an _alloc() and there is nothing available in the cache, a new
slab is created, and the ctor is called.
Is that the subtly that you are referring to?
Is doing this ctor only when new cache slabs are created sufficient?
Mike
> struct i915_request *
> __i915_request_create(struct intel_context *ce, gfp_t gfp)
> {
>@@ -648,6 +663,7 @@ __i915_request_create(struct intel_context *ce, gfp_t
>gfp)
> rq->engine = ce->engine;
> rq->ring = ce->ring;
> rq->execution_mask = ce->engine->mask;
>+ rq->flags = 0;
>
> rcu_assign_pointer(rq->timeline, tl);
> rq->hwsp_seqno = tl->hwsp_seqno;
>@@ -655,23 +671,20 @@ __i915_request_create(struct intel_context *ce,
>gfp_t gfp)
>
> rq->rcustate = get_state_synchronize_rcu(); /* acts as smp_mb() */
>
>- spin_lock_init(&rq->lock);
> dma_fence_init(&rq->fence, &i915_fence_ops, &rq->lock,
> tl->fence_context, seqno);
>
> /* We bump the ref for the fence chain */
>- i915_sw_fence_init(&i915_request_get(rq)->submit, submit_notify);
>- i915_sw_fence_init(&i915_request_get(rq)->semaphore,
>semaphore_notify);
>+ i915_sw_fence_reinit(&i915_request_get(rq)->submit);
>+ i915_sw_fence_reinit(&i915_request_get(rq)->semaphore);
>
>- i915_sched_node_init(&rq->sched);
>+ i915_sched_node_reinit(&rq->sched);
>
>- /* No zalloc, must clear what we need by hand */
>- rq->file_priv = NULL;
>+ /* No zalloc, everything must be cleared after use */
> rq->batch = NULL;
>- rq->capture_list = NULL;
>- rq->flags = 0;
>-
>- INIT_LIST_HEAD(&rq->execute_cb);
>+ GEM_BUG_ON(rq->file_priv);
>+ GEM_BUG_ON(rq->capture_list);
>+ GEM_BUG_ON(!list_empty(&rq->execute_cb));
>
> /*
> * Reserve space in the ring buffer for all the commands required to
>@@ -1533,10 +1546,14 @@ static struct i915_global_request global = { {
>
> int __init i915_global_request_init(void)
> {
>- global.slab_requests = KMEM_CACHE(i915_request,
>- SLAB_HWCACHE_ALIGN |
>- SLAB_RECLAIM_ACCOUNT |
>- SLAB_TYPESAFE_BY_RCU);
>+ global.slab_requests =
>+ kmem_cache_create("i915_request",
>+ sizeof(struct i915_request),
>+ __alignof__(struct i915_request),
>+ SLAB_HWCACHE_ALIGN |
>+ SLAB_RECLAIM_ACCOUNT |
>+ SLAB_TYPESAFE_BY_RCU,
>+ __i915_request_ctor);
> if (!global.slab_requests)
> return -ENOMEM;
>
>diff --git a/drivers/gpu/drm/i915/i915_scheduler.c
>b/drivers/gpu/drm/i915/i915_scheduler.c
>index 194246548c4d..a5a6dbe6a53c 100644
>--- a/drivers/gpu/drm/i915/i915_scheduler.c
>+++ b/drivers/gpu/drm/i915/i915_scheduler.c
>@@ -387,6 +387,10 @@ void i915_sched_node_init(struct i915_sched_node
>*node)
> INIT_LIST_HEAD(&node->signalers_list);
> INIT_LIST_HEAD(&node->waiters_list);
> INIT_LIST_HEAD(&node->link);
>+}
>+
>+void i915_sched_node_reinit(struct i915_sched_node *node)
>+{
> node->attr.priority = I915_PRIORITY_INVALID;
> node->semaphores = 0;
> node->flags = 0;
>@@ -481,6 +485,7 @@ void i915_sched_node_fini(struct i915_sched_node
>*node)
> if (dep->flags & I915_DEPENDENCY_ALLOC)
> i915_dependency_free(dep);
> }
>+ INIT_LIST_HEAD(&node->signalers_list);
>
> /* Remove ourselves from everyone who depends upon us */
> list_for_each_entry_safe(dep, tmp, &node->waiters_list, wait_link) {
>@@ -491,6 +496,7 @@ void i915_sched_node_fini(struct i915_sched_node
>*node)
> if (dep->flags & I915_DEPENDENCY_ALLOC)
> i915_dependency_free(dep);
> }
>+ INIT_LIST_HEAD(&node->waiters_list);
>
> spin_unlock_irq(&schedule_lock);
> }
>diff --git a/drivers/gpu/drm/i915/i915_scheduler.h
>b/drivers/gpu/drm/i915/i915_scheduler.h
>index 07d243acf553..d1dc4efef77b 100644
>--- a/drivers/gpu/drm/i915/i915_scheduler.h
>+++ b/drivers/gpu/drm/i915/i915_scheduler.h
>@@ -26,6 +26,7 @@
> sched.link)
>
> void i915_sched_node_init(struct i915_sched_node *node);
>+void i915_sched_node_reinit(struct i915_sched_node *node);
>
> bool __i915_sched_node_add_dependency(struct i915_sched_node *node,
> struct i915_sched_node *signal,
>diff --git a/drivers/gpu/drm/i915/i915_sw_fence.c
>b/drivers/gpu/drm/i915/i915_sw_fence.c
>index 6a88db291252..eacc6c5ce0fd 100644
>--- a/drivers/gpu/drm/i915/i915_sw_fence.c
>+++ b/drivers/gpu/drm/i915/i915_sw_fence.c
>@@ -227,6 +227,14 @@ void __i915_sw_fence_init(struct i915_sw_fence
>*fence,
> fence->flags = (unsigned long)fn;
> }
>
>+void i915_sw_fence_reinit(struct i915_sw_fence *fence)
>+{
>+ debug_fence_init(fence);
>+
>+ atomic_set(&fence->pending, 1);
>+ fence->error = 0;
>+}
>+
> void i915_sw_fence_commit(struct i915_sw_fence *fence)
> {
> debug_fence_activate(fence);
>diff --git a/drivers/gpu/drm/i915/i915_sw_fence.h
>b/drivers/gpu/drm/i915/i915_sw_fence.h
>index ab7d58bd0b9d..1e90d9a51bd2 100644
>--- a/drivers/gpu/drm/i915/i915_sw_fence.h
>+++ b/drivers/gpu/drm/i915/i915_sw_fence.h
>@@ -54,6 +54,8 @@ do {
> \
> __i915_sw_fence_init((fence), (fn), NULL, NULL)
> #endif
>
>+void i915_sw_fence_reinit(struct i915_sw_fence *fence);
>+
> #ifdef CONFIG_DRM_I915_SW_FENCE_DEBUG_OBJECTS
> void i915_sw_fence_fini(struct i915_sw_fence *fence);
> #else
>--
>2.24.0
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list