| Message ID | 20200128171614.3845825-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/execlist: Mark up racy read of execlists->pending[0] | expand |
Chris Wilson <chris@chris-wilson.co.uk> writes: > We write to execlists->pending[0] in process_csb() to acknowledge the > completion of the ESLP update, outside of the main spinlock. When we > check the current status of the previous submission in > __execlists_submission_tasklet() we should therefore use READ_ONCE() to > reflect and document the unsynchronized read. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_lrc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c > index cf6c43bd540a..058484958e87 100644 > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > @@ -2347,7 +2347,7 @@ static void process_csb(struct intel_engine_cs *engine) > static void __execlists_submission_tasklet(struct intel_engine_cs *const engine) > { > lockdep_assert_held(&engine->active.lock); > - if (!engine->execlists.pending[0]) { > + if (!READ_ONCE(engine->execlists.pending[0])) { With same token, should we also include assert_pending_invalid() read of pending with READ_ONCE? Even if the top level READ_ONCE would guard the next one, for documentation. -Mika > rcu_read_lock(); /* protect peeking at execlists->active */ > execlists_dequeue(engine); > rcu_read_unlock(); > -- > 2.25.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Mika Kuoppala (2020-01-29 09:29:43) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > We write to execlists->pending[0] in process_csb() to acknowledge the > > completion of the ESLP update, outside of the main spinlock. When we > > check the current status of the previous submission in > > __execlists_submission_tasklet() we should therefore use READ_ONCE() to > > reflect and document the unsynchronized read. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > --- > > drivers/gpu/drm/i915/gt/intel_lrc.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c > > index cf6c43bd540a..058484958e87 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c > > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > > @@ -2347,7 +2347,7 @@ static void process_csb(struct intel_engine_cs *engine) > > static void __execlists_submission_tasklet(struct intel_engine_cs *const engine) > > { > > lockdep_assert_held(&engine->active.lock); > > - if (!engine->execlists.pending[0]) { > > + if (!READ_ONCE(engine->execlists.pending[0])) { > > With same token, should we also include assert_pending_invalid() > read of pending with READ_ONCE? That happens on the control paths, so the state of pending[] at that point should be static (and the compiler can be left to its own devices). -Chris
Chris Wilson <chris@chris-wilson.co.uk> writes: > Quoting Mika Kuoppala (2020-01-29 09:29:43) >> Chris Wilson <chris@chris-wilson.co.uk> writes: >> >> > We write to execlists->pending[0] in process_csb() to acknowledge the >> > completion of the ESLP update, outside of the main spinlock. When we >> > check the current status of the previous submission in >> > __execlists_submission_tasklet() we should therefore use READ_ONCE() to >> > reflect and document the unsynchronized read. >> > >> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> > --- >> > drivers/gpu/drm/i915/gt/intel_lrc.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c >> > index cf6c43bd540a..058484958e87 100644 >> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c >> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c >> > @@ -2347,7 +2347,7 @@ static void process_csb(struct intel_engine_cs *engine) >> > static void __execlists_submission_tasklet(struct intel_engine_cs *const engine) >> > { >> > lockdep_assert_held(&engine->active.lock); >> > - if (!engine->execlists.pending[0]) { >> > + if (!READ_ONCE(engine->execlists.pending[0])) { >> >> With same token, should we also include assert_pending_invalid() >> read of pending with READ_ONCE? > > That happens on the control paths, so the state of pending[] at that > point should be static (and the compiler can be left to its own > devices). It should be static. Fair enough Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c index cf6c43bd540a..058484958e87 100644 --- a/drivers/gpu/drm/i915/gt/intel_lrc.c +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c @@ -2347,7 +2347,7 @@ static void process_csb(struct intel_engine_cs *engine) static void __execlists_submission_tasklet(struct intel_engine_cs *const engine) { lockdep_assert_held(&engine->active.lock); - if (!engine->execlists.pending[0]) { + if (!READ_ONCE(engine->execlists.pending[0])) { rcu_read_lock(); /* protect peeking at execlists->active */ execlists_dequeue(engine); rcu_read_unlock();
We write to execlists->pending[0] in process_csb() to acknowledge the completion of the ESLP update, outside of the main spinlock. When we check the current status of the previous submission in __execlists_submission_tasklet() we should therefore use READ_ONCE() to reflect and document the unsynchronized read. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/gt/intel_lrc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)