| Message ID | 20191121135131.338984-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request | expand |
On 21/11/2019 13:51, 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. How do you tell which members should be left untouched on request_create? > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/i915_request.c | 37 ++++++++++++++++++--------- > 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, 42 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c > index 00011f9533b6..5e5bd7d57134 100644 > --- a/drivers/gpu/drm/i915/i915_request.c > +++ b/drivers/gpu/drm/i915/i915_request.c > @@ -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,19 @@ 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); > + > + INIT_LIST_HEAD(&rq->execute_cb); > + rq->file_priv = NULL; > +} > + > struct i915_request * > __i915_request_create(struct intel_context *ce, gfp_t gfp) > { > @@ -655,24 +668,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; > rq->batch = NULL; > rq->capture_list = NULL; > rq->flags = 0; > > - INIT_LIST_HEAD(&rq->execute_cb); For instance this member is now untouched. How do we assert it is in the correct state before it gets used with the new request? What if it has dangling state? Same question for rq->file_priv? We have to be sure it is not read and acted upon before it is set, right? Sw_fence looks okay. Sched_node as well since it seems to have a fini which cleans it up. Regards, Tvrtko > - > /* > * Reserve space in the ring buffer for all the commands required to > * eventually emit this request. This is to guarantee that the > @@ -1533,10 +1542,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 >
Quoting Tvrtko Ursulin (2019-11-21 16:30:08) > > On 21/11/2019 13:51, 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. > > How do you tell which members should be left untouched on request_create? Chicken entrails. As I remember it, we basically have to ensure that anything stays intact (where intact can just mean we continue to get expected results, not necessarily for the same reason) until both parties have passed a strong memory barrier. Typically that is a successful kref_get for the reader, and in the request create we have an mb() in the middle of rcustate. The situations that have been popping up most recently are using rq->lock across the recycling. The reader would acquire the spinlock, but i915_request_create would then call spinlock_init and from there it goes downhill. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > --- > > drivers/gpu/drm/i915/i915_request.c | 37 ++++++++++++++++++--------- > > 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, 42 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c > > index 00011f9533b6..5e5bd7d57134 100644 > > --- a/drivers/gpu/drm/i915/i915_request.c > > +++ b/drivers/gpu/drm/i915/i915_request.c > > @@ -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,19 @@ 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); > > + > > + INIT_LIST_HEAD(&rq->execute_cb); > > + rq->file_priv = NULL; > > +} > > + > > struct i915_request * > > __i915_request_create(struct intel_context *ce, gfp_t gfp) > > { > > @@ -655,24 +668,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; > > rq->batch = NULL; > > rq->capture_list = NULL; > > rq->flags = 0; > > > > - INIT_LIST_HEAD(&rq->execute_cb); > > For instance this member is now untouched. How do we assert it is in the > correct state before it gets used with the new request? What if it has > dangling state? We always maintain it in the correct state. We can assert it that is empty here. > Same question for rq->file_priv? We have to be sure it is not read and > acted upon before it is set, right? GEM_BUG_ON(rq->file_priv == NULL); I see a pattern forming. -Chris
Quoting Patchwork (2019-11-21 17:00:02) > == Series Details == > > Series: series starting with [1/5] drm/i915: Use a ctor for TYPESAFE_BY_RCU i915_request > URL : https://patchwork.freedesktop.org/series/69834/ > State : failure > > == Summary == > > CI Bug Log - changes from CI_DRM_7400 -> Patchwork_15378 > ==================================================== > > Summary > ------- > > **FAILURE** > > Serious unknown changes coming with Patchwork_15378 absolutely need to be > verified manually. > > If you think the reported changes have nothing to do with the changes > introduced in Patchwork_15378, please notify your bug team to allow them > to document this new failure mode, which will reduce false positives in CI. > > External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15378/index.html > > Possible new issues > ------------------- > > Here are the unknown changes that may have been introduced in Patchwork_15378: > > ### IGT changes ### > > #### Possible regressions #### > > * igt@i915_selftest@live_gtt: > - fi-kbl-8809g: [PASS][1] -> [INCOMPLETE][2] > [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7400/fi-kbl-8809g/igt@i915_selftest@live_gtt.html > [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15378/fi-kbl-8809g/igt@i915_selftest@live_gtt.html Gah, <0> [427.586298] kworker/-492 1.... 418613623us : intel_timeline_exit: intel_timeline_exit:374 GEM_BUG_ON(!atomic_read(&tl->active_count)) <0> [427.586302] --------------------------------- <4> [427.586919] ------------[ cut here ]------------ <2> [427.586922] kernel BUG at drivers/gpu/drm/i915/gt/intel_timeline.c:374! <4> [427.586927] invalid opcode: 0000 [#1] PREEMPT SMP PTI <4> [427.586929] CPU: 1 PID: 492 Comm: kworker/1:2 Tainted: G U W 5.4.0-rc8-CI-Patchwork_15378+ #1 <4> [427.586931] Hardware name: Intel Corporation S1200SP/S1200SP, BIOS S1200SP.86B.03.01.0026.092720170729 09/27/2017 <4> [427.586987] Workqueue: events engine_retire [i915] <4> [427.587029] RIP: 0010:intel_timeline_exit+0xd6/0x160 [i915] <4> [427.587031] Code: 00 48 c7 c2 70 71 77 a0 48 c7 c7 fb e7 62 a0 e8 60 cb b6 e0 bf 01 00 00 00 e8 d6 9e b6 e0 31 f6 bf 09 00 00 00 e8 7a 12 a8 e0 <0f> 0b 48 81 c5 70 04 00 00 48 89 ef e8 49 b6 3b e1 f0 ff 8b 94 00 <4> [427.587033] RSP: 0018:ffffc9000068fdc0 EFLAGS: 00010297 <4> [427.587036] RAX: ffff888254740040 RBX: ffff888262d6e200 RCX: 0000000000000001 <4> [427.587037] RDX: 00000000000018c9 RSI: 0000000000000000 RDI: 0000000000000009 <4> [427.587039] RBP: ffff8882261cbc58 R08: 0000000000000000 R09: 0000000000000000 <4> [427.587040] R10: 0000000000000000 R11: 0000000000000000 R12: ffff888225352068 <4> [427.587042] R13: ffff888225352000 R14: ffff88821d43fc40 R15: ffff8882253526d8 <4> [427.587059] FS: 0000000000000000(0000) GS:ffff88826b480000(0000) knlGS:0000000000000000 <4> [427.587060] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 <4> [427.587062] CR2: 00005651d791d390 CR3: 0000000002210002 CR4: 00000000003606e0 <4> [427.587063] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 <4> [427.587064] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 <4> [427.587066] Call Trace: <4> [427.587098] intel_context_exit_engine+0xe/0x70 [i915] <4> [427.587141] i915_request_retire+0x32b/0x920 [i915] <4> [427.587234] retire_requests+0x4d/0x60 [i915] <4> [427.587371] engine_retire+0x63/0xe0 [i915] So we are now hitting timeline_exit in engine_retire before timeline_enter in engine_park. My expectation was that would be serialised by the timelines->lock... But no, we use intel_timeline_exit: GEM_BUG_ON(!atomic_read(&tl->active_count)); if (atomic_add_unless(&tl->active_count, -1, 1)) return; spin_lock(&timelines->lock); if (atomic_dec_and_test(&tl->active_count)) list_del(&tl->link); spin_unlock(&timelines->lock); Hmm. Could bias the tl->active_count as we do engine_park? -Chris
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c index 00011f9533b6..5e5bd7d57134 100644 --- a/drivers/gpu/drm/i915/i915_request.c +++ b/drivers/gpu/drm/i915/i915_request.c @@ -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,19 @@ 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); + + INIT_LIST_HEAD(&rq->execute_cb); + rq->file_priv = NULL; +} + struct i915_request * __i915_request_create(struct intel_context *ce, gfp_t gfp) { @@ -655,24 +668,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; rq->batch = NULL; rq->capture_list = NULL; rq->flags = 0; - INIT_LIST_HEAD(&rq->execute_cb); - /* * Reserve space in the ring buffer for all the commands required to * eventually emit this request. This is to guarantee that the @@ -1533,10 +1542,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
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. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/i915_request.c | 37 ++++++++++++++++++--------- 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, 42 insertions(+), 12 deletions(-)