[Intel-gfx] [CI] drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Fri Nov 22 09:24:56 UTC 2019
On 22/11/2019 07:02, Chris Wilson wrote:
> 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>
Now that you made the commit message so pretty I have not choice but to:
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
Seriously, I think it looks fine. Am I missing something? Could be I
guess. Anything more could be done for instance in sw_fence_reinit
checking the wq head?
Regards,
Tvrtko
> ---
> 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);
> +}
> +
> 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
>
More information about the Intel-gfx
mailing list