Re: [Intel-gfx] [RFC 5/8] drm/i915: Contexts can use struct pid stored in the client

Thu, 30 Jan 2020 08:12:47 -0800 


On 10/01/2020 13:42, Chris Wilson wrote:

Quoting Tvrtko Ursulin (2020-01-10 13:30:46)

From: Tvrtko Ursulin <tvrtko.ursu...@intel.com>

Now that contexts keep their parent client reference counted, we can
remove separate struct pid reference owned by contexts in favour of the
one already held by the client.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
---
  drivers/gpu/drm/i915/gem/i915_gem_context.c       | 13 ++++---------
  drivers/gpu/drm/i915/gem/i915_gem_context_types.h | 10 ----------
  drivers/gpu/drm/i915/i915_debugfs.c               |  7 ++++---
  drivers/gpu/drm/i915/i915_gpu_error.c             |  8 ++++----
  4 files changed, 12 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index ba8ccc754f20..758cebb99ba4 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -323,7 +323,6 @@ static void i915_gem_context_free(struct i915_gem_context 
*ctx)
         if (ctx->timeline)
                 intel_timeline_put(ctx->timeline);

  
-       put_pid(ctx->pid);

         mutex_destroy(&ctx->mutex);

  
         kfree_rcu(ctx, rcu);

@@ -794,24 +793,20 @@ static int gem_context_register(struct i915_gem_context 
*ctx,
                 WRITE_ONCE(vm->file, fpriv); /* XXX */
         mutex_unlock(&ctx->mutex);

  
-       ctx->pid = get_task_pid(current, PIDTYPE_PID);

         snprintf(ctx->name, sizeof(ctx->name), "%s[%d]",
-                current->comm, pid_nr(ctx->pid));
+                current->comm, pid_nr(client->pid));

  
         /* And finally expose ourselves to userspace via the idr */

         ret = xa_alloc(&fpriv->context_xa, id, ctx, xa_limit_32b, GFP_KERNEL);
-       if (ret) {
-               put_pid(fetch_and_zero(&ctx->pid));
-               goto out;
-       }
+       if (ret)
+               return ret;

  
         ctx->client = client = i915_drm_client_get(fpriv->client);

         spin_lock(&client->ctx_lock);
         list_add_tail_rcu(&ctx->client_link, &client->ctx_list);
         spin_unlock(&client->ctx_lock);

  
-out:

-       return ret;
+       return 0;
  }

  
  int i915_gem_context_open(struct drm_i915_private *i915,

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h 
b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
index 879824159646..23421377a43d 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
@@ -90,16 +90,6 @@ struct i915_gem_context {
          */
         struct i915_address_space __rcu *vm;

  
-       /**

-        * @pid: process id of creator
-        *
-        * Note that who created the context may not be the principle user,
-        * as the context may be shared across a local socket. However,
-        * that should only affect the default context, all contexts created
-        * explicitly by the client are expected to be isolated.
-        */
-       struct pid *pid;
-
         /** link: place with &drm_i915_private.context_list */
         struct list_head link;
         struct llist_node free_link;
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index 8f01c2bc7355..bc533501b4e0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -346,7 +346,8 @@ static void print_context_stats(struct seq_file *m,
                         rcu_read_unlock();

  
                         rcu_read_lock();

-                       task = pid_task(ctx->pid ?: file->pid, PIDTYPE_PID);
+                       task = pid_task(ctx->client->pid ?: file->pid,
+                                       PIDTYPE_PID);
                         snprintf(name, sizeof(name), "%s",
                                  task ? task->comm : "<unknown>");
                         rcu_read_unlock();
@@ -1492,10 +1493,10 @@ static int i915_context_status(struct seq_file *m, void 
*unused)
                 spin_unlock(&i915->gem.contexts.lock);

  
                 seq_puts(m, "HW context ");

-               if (ctx->pid) {
+               if (ctx->client->pid) {
                         struct task_struct *task;

  
-                       task = get_pid_task(ctx->pid, PIDTYPE_PID);

+                       task = get_pid_task(ctx->client->pid, PIDTYPE_PID);
                         if (task) {
                                 seq_printf(m, "(%s [%d]) ",
                                            task->comm, task->pid);
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c 
b/drivers/gpu/drm/i915/i915_gpu_error.c
index fda0977d2059..9240327bdb7d 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1235,8 +1235,8 @@ static void record_request(const struct i915_request 
*request,
         erq->pid = 0;
         rcu_read_lock();
         ctx = rcu_dereference(request->context->gem_context);
-       if (ctx)
-               erq->pid = pid_nr(ctx->pid);
+       if (ctx && ctx->client->pid)
+               erq->pid = pid_nr(ctx->client->pid);
         rcu_read_unlock();
  }

  
@@ -1313,11 +1313,11 @@ static bool record_context(struct drm_i915_error_context *e,

         if (ctx && !kref_get_unless_zero(&ctx->ref))
                 ctx = NULL;
         rcu_read_unlock();
-       if (!ctx)
+       if (!ctx || !ctx->client->pid)
                 return false;


Can we have a client without a pid?



Yes, when fbdev registers I ignore it as a client because sysfs is not 
there yet. I contemplated stashing it somewhere and registering later 
but did not yet decide to actually do it. This was actually because 
without sysfs I had no use for pid and name. Perhaps now that I have 
moved this patch earlier I can change this to always have those two.. 
okay I'll see.




Holding a reference to the ctx is sufficent to protect the
i915_drm_client, ok.


         rcu_read_lock();
-       task = pid_task(ctx->pid, PIDTYPE_PID);
+       task = pid_task(ctx->client->pid, PIDTYPE_PID);
         if (task) {
                 strcpy(e->comm, task->comm);
                 e->pid = task->pid;


Ok, if you move this ahead to patch 3, we can use this as justification
for i915_drm_client all by itself.



Yes mostly done but will see the above, plus a small regression in 
busyness tracking crept in after rebase, somehow. Will post the series 
when I resolve that.


Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx






















					Reply via email to