[Intel-gfx] [PATCH] drm/i915: Mark the GEM context link as RCU protected

[Chris Wilson]

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 at chris-wilson.co.uk>
> > Cc: Andi Shyti <andi.shyti at intel.com>
> > ---
> >   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