Quoting Tvrtko Ursulin (2020-01-07 11:15:39)
>
> On 22/12/2019 23:35, Chris Wilson wrote:
> > The only protection for intel_context.gem_cotext is granted by RCU, so
> > annotate it as a rcu protected pointer and carefully dereference it in
> > the few occasions we need to use it.
> >
> > Fixes: 9f3ccd40acf4 ("drm/i915: Drop GEM context as a direct link from i915_request")
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Andi Shyti <andi.shyti@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/gem/i915_gem_context.c | 5 ++-
> > drivers/gpu/drm/i915/gt/intel_context_types.h | 2 +-
> > drivers/gpu/drm/i915/gt/intel_reset.c | 26 +++++++++---
> > .../gpu/drm/i915/gt/intel_ring_submission.c | 2 +-
> > drivers/gpu/drm/i915/i915_gpu_error.c | 40 ++++++++++++-------
> > drivers/gpu/drm/i915/i915_request.c | 6 +--
> > drivers/gpu/drm/i915/i915_request.h | 8 ++++
> > 7 files changed, 62 insertions(+), 27 deletions(-)
> >
>
> [snip]
>
> >
> > static void engine_record_requests(struct intel_engine_cs *engine,
> > @@ -1298,28 +1304,34 @@ static void error_record_engine_execlists(const struct intel_engine_cs *engine,
> > static bool record_context(struct drm_i915_error_context *e,
> > const struct i915_request *rq)
> > {
> > - const struct i915_gem_context *ctx = rq->context->gem_context;
> > + struct i915_gem_context *ctx;
> > + struct task_struct *task;
> > + bool capture;
> >
> > + rcu_read_lock();
> > + ctx = rcu_dereference(rq->context->gem_context);
> > + if (ctx && !kref_get_unless_zero(&ctx->ref))
> > + ctx = NULL;
> > + rcu_read_unlock();
> > if (!ctx)
> > return false;
> >
> > - if (ctx->pid) {
> > - struct task_struct *task;
> > -
> > - rcu_read_lock();
> > - task = pid_task(ctx->pid, PIDTYPE_PID);
> > - if (task) {
> > - strcpy(e->comm, task->comm);
> > - e->pid = task->pid;
> > - }
> > - rcu_read_unlock();
> > + rcu_read_lock();
> > + task = pid_task(ctx->pid, PIDTYPE_PID);
> > + if (task) {
> > + strcpy(e->comm, task->comm);
> > + e->pid = task->pid;
> > }
> > + rcu_read_unlock();
>
> Why is this rcu_read_lock section needed? The first one obtained the
> reference to the context so should be good.
The task returned by ctx->pid is not protected by that reference, and
pid_task() doesn't increment the reference on the task. That's what I
remember of the pid_task() interface, that requires rcu to be held while
you look inside, where get_pid_task() does not.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
