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

[Tvrtko Ursulin]

On 07/01/2020 11:15, Tvrtko Ursulin wrote:
> 
> 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.

Hmm pid_task() does:

...
                 first = 
rcu_dereference_check(hlist_first_rcu(&pid->tasks[type]),
 
lockdep_tasklist_lock_is_held());

...

Note, tasklist_lock_is_held.

Regards,

Tvrtko