Quoting Tvrtko Ursulin (2019-11-29 16:25:08) > > On 29/11/2019 15:18, Chris Wilson wrote: > > Though the context is closed and so no more requests can be added to the > > timeline, retirement can still be removing requests. It can even be > > removing the very request we are inspecting and so cause us to wander > > into dead links. > > > > Serialise with the retirement by taking the timeline->mutex used for > > guarding the timeline->requests list. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112404 > > Fixes: 4a3174152147 ("drm/i915/gem: Refine occupancy test in kill_context()") > > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> > > Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx> > > Cc: Matthew Auld <matthew.auld@xxxxxxxxx> > > Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx> > > --- > > drivers/gpu/drm/i915/gem/i915_gem_context.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c > > index a179e170c936..9f1dc96b10a6 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c > > @@ -403,7 +403,7 @@ static struct intel_engine_cs *active_engine(struct intel_context *ce) > > if (!ce->timeline) > > return NULL; > > > > - rcu_read_lock(); > > + mutex_lock(&ce->timeline->mutex); > > list_for_each_entry_reverse(rq, &ce->timeline->requests, link) { > > if (i915_request_completed(rq)) > > break; > > @@ -413,7 +413,7 @@ static struct intel_engine_cs *active_engine(struct intel_context *ce) > > if (engine) > > break; > > } > > - rcu_read_unlock(); > > + mutex_unlock(&ce->timeline->mutex); > > > > return engine; > > } > > > > If retire would use list_del_rcu could we get away with no locking here? > (And list_add_tail_rcu when adding to the timeline.) It's not a > contended path I know. So this works as well. Off-the-top of my head, I think rculist still only allows forward walks, at least there are no _reverse_rcu and I think the iterator safety hinges upon the order in which the pointers are updated in list_add. Lockless and cache-friendly replacements sought; apply within. -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
