| Message ID | 20180507092527.7359-2-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Chris Wilson <chris@chris-wilson.co.uk> writes: > Around request submission, we protect the call to schedule with a > premption disable loop to prevent set-wedge chaning function pointers s/premption/preemption s/chaning/changing Also RCU read critical sections can be preempted if CONFIG_PREEMPT_RCU so it looks like we should have had explicit preempt_disable() before the rcu_read_lock(), if it is the preempt disable we rely on? -Mika > underneath us. This also prevents the tasklet running on the local CPU, > a trick we use immediately afterwards to forcibly execute the tasklet > after submission. We can combine the two wlog, and ensure that the > tasklet is only executed (on the local CPU) after we complete our > updates to the submission queue (and not after an intermediate, > incomplete update). > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/i915_request.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c > index e4cf76ec14a6..88ee444679d4 100644 > --- a/drivers/gpu/drm/i915/i915_request.c > +++ b/drivers/gpu/drm/i915/i915_request.c > @@ -1110,12 +1110,9 @@ void __i915_request_add(struct i915_request *request, bool flush_caches) > * decide whether to preempt the entire chain so that it is ready to > * run at the earliest possible convenience. > */ > - rcu_read_lock(); > + local_bh_disable(); > if (engine->schedule) > engine->schedule(request, &request->ctx->sched); > - rcu_read_unlock(); > - > - local_bh_disable(); > i915_sw_fence_commit(&request->submit); > local_bh_enable(); /* Kick the execlists tasklet if just scheduled */ > > -- > 2.17.0
Quoting Mika Kuoppala (2018-05-07 13:09:20) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > Around request submission, we protect the call to schedule with a > > premption disable loop to prevent set-wedge chaning function pointers > > s/premption/preemption > s/chaning/changing > > Also RCU read critical sections can be preempted if CONFIG_PREEMPT_RCU > so it looks like we should have had explicit preempt_disable() > before the rcu_read_lock(), if it is the preempt disable we rely on? No, my mistake in not knowing about PREEMPT_RCU. So we need both. Sigh. -Chris
diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c index e4cf76ec14a6..88ee444679d4 100644 --- a/drivers/gpu/drm/i915/i915_request.c +++ b/drivers/gpu/drm/i915/i915_request.c @@ -1110,12 +1110,9 @@ void __i915_request_add(struct i915_request *request, bool flush_caches) * decide whether to preempt the entire chain so that it is ready to * run at the earliest possible convenience. */ - rcu_read_lock(); + local_bh_disable(); if (engine->schedule) engine->schedule(request, &request->ctx->sched); - rcu_read_unlock(); - - local_bh_disable(); i915_sw_fence_commit(&request->submit); local_bh_enable(); /* Kick the execlists tasklet if just scheduled */
Around request submission, we protect the call to schedule with a premption disable loop to prevent set-wedge chaning function pointers underneath us. This also prevents the tasklet running on the local CPU, a trick we use immediately afterwards to forcibly execute the tasklet after submission. We can combine the two wlog, and ensure that the tasklet is only executed (on the local CPU) after we complete our updates to the submission queue (and not after an intermediate, incomplete update). Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- drivers/gpu/drm/i915/i915_request.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)