| Message ID | 20200110133049.2705-6-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Per client engine busyness | expand |
Quoting Tvrtko Ursulin (2020-01-10 13:30:46) > From: Tvrtko Ursulin <tvrtko.ursulin@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.ursulin@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? 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. -Chris
On 10/01/2020 13:42, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2020-01-10 13:30:46) >> From: Tvrtko Ursulin <tvrtko.ursulin@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.ursulin@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
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; 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;