Re: [Intel-gfx] [PATCH] drm/i915: Mark the GEM context link as RCU protected
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
Cc: Andi Shyti
---
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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Mark the GEM context link as RCU protected
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
> > Cc: Andi Shyti
> > ---
> > 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Mark the GEM context link as RCU protected
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
Cc: Andi Shyti
---
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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
