Re: [PATCH] drm/i915: Mark the GEM context link as RCU protected

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@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.

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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux