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

[Tvrtko Ursulin]

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.

Regards,

Tvrtko