[02/25] drm/i915/gem: Embed context/timeline name inside the GEM context
diff mbox series

Message ID 20191110185806.17413-2-chris@chris-wilson.co.uk
State New
Headers show
Series
  • [01/25] drm/i915: Protect context while grabbing its name for the request
Related show

Commit Message

Chris Wilson Nov. 10, 2019, 6:57 p.m. UTC
Use a small char buffer inside the i915_gem_context to store the user
friendly name so that ctx->name has the same lifetime as the RCU
protected GEM context. That is, e.g. when using print_request() that
prints the timeline name (ctx->name), the name will not be prematurely
freed upon the context being closed and the last reference dropped.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c    | 12 ++----------
 .../gpu/drm/i915/gem/i915_gem_context_types.h  | 18 +++++++++---------
 .../gpu/drm/i915/gem/selftests/mock_context.c  |  4 +---
 3 files changed, 12 insertions(+), 22 deletions(-)

Comments

Mika Kuoppala Nov. 11, 2019, 11:20 a.m. UTC | #1
Chris Wilson <chris@chris-wilson.co.uk> writes:

> Use a small char buffer inside the i915_gem_context to store the user
> friendly name so that ctx->name has the same lifetime as the RCU
> protected GEM context. That is, e.g. when using print_request() that
> prints the timeline name (ctx->name), the name will not be prematurely
> freed upon the context being closed and the last reference dropped.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_context.c    | 12 ++----------
>  .../gpu/drm/i915/gem/i915_gem_context_types.h  | 18 +++++++++---------
>  .../gpu/drm/i915/gem/selftests/mock_context.c  |  4 +---
>  3 files changed, 12 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 94b51b13ef17..982770e8163d 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -277,9 +277,7 @@ static void i915_gem_context_free(struct i915_gem_context *ctx)
>  	if (ctx->timeline)
>  		intel_timeline_put(ctx->timeline);
>  
> -	kfree(ctx->name);
>  	put_pid(ctx->pid);
> -
>  	mutex_destroy(&ctx->mutex);
>  
>  	kfree_rcu(ctx, rcu);
> @@ -789,12 +787,8 @@ static int gem_context_register(struct i915_gem_context *ctx,
>  	mutex_unlock(&ctx->mutex);
>  
>  	ctx->pid = get_task_pid(current, PIDTYPE_PID);
> -	ctx->name = kasprintf(GFP_KERNEL, "%s[%d]",
> -			      current->comm, pid_nr(ctx->pid));
> -	if (!ctx->name) {
> -		ret = -ENOMEM;
> -		goto err_pid;
> -	}
> +	snprintf(ctx->name, sizeof(ctx->name), "%s[%d]",
> +		 current->comm, pid_nr(ctx->pid));
>  
>  	/* And finally expose ourselves to userspace via the idr */
>  	mutex_lock(&fpriv->context_idr_lock);
> @@ -803,8 +797,6 @@ static int gem_context_register(struct i915_gem_context *ctx,
>  	if (ret >= 0)
>  		goto out;
>  
> -	kfree(fetch_and_zero(&ctx->name));
> -err_pid:
>  	put_pid(fetch_and_zero(&ctx->pid));
>  out:
>  	return ret;
> 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 861d7d92fe9f..69df5459c350 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> @@ -100,15 +100,6 @@ struct i915_gem_context {
>  	 */
>  	struct pid *pid;
>  
> -	/**
> -	 * @name: arbitrary name
> -	 *
> -	 * A name is constructed for the context from the creator's process
> -	 * name, pid and user handle in order to uniquely identify the
> -	 * context in messages.
> -	 */
> -	const char *name;
> -
>  	/** link: place with &drm_i915_private.context_list */
>  	struct list_head link;
>  	struct llist_node free_link;
> @@ -176,6 +167,15 @@ struct i915_gem_context {
>  	 * per vm, which may be one per context or shared with the global GTT)
>  	 */
>  	struct radix_tree_root handles_vma;
> +
> +	/**
> +	 * @name: arbitrary name, used for user debug
> +	 *
> +	 * A name is constructed for the context from the creator's process
> +	 * name, pid and user handle in order to uniquely identify the
> +	 * context in messages.
> +	 */
> +	char name[TASK_COMM_LEN + 8];

I consider 24 bytes as quite cheap for robustness and simplicity.
In a playground where we throw pages around like sand.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

>  };
>  
>  #endif /* __I915_GEM_CONTEXT_TYPES_H__ */
> diff --git a/drivers/gpu/drm/i915/gem/selftests/mock_context.c b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
> index cdcb006321a7..53e89efb09c0 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/mock_context.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
> @@ -37,9 +37,7 @@ mock_context(struct drm_i915_private *i915,
>  	if (name) {
>  		struct i915_ppgtt *ppgtt;
>  
> -		ctx->name = kstrdup(name, GFP_KERNEL);
> -		if (!ctx->name)
> -			goto err_put;
> +		strncpy(ctx->name, name, sizeof(ctx->name));
>  
>  		ppgtt = mock_ppgtt(i915, name);
>  		if (!ppgtt)
> -- 
> 2.24.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 94b51b13ef17..982770e8163d 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -277,9 +277,7 @@  static void i915_gem_context_free(struct i915_gem_context *ctx)
 	if (ctx->timeline)
 		intel_timeline_put(ctx->timeline);
 
-	kfree(ctx->name);
 	put_pid(ctx->pid);
-
 	mutex_destroy(&ctx->mutex);
 
 	kfree_rcu(ctx, rcu);
@@ -789,12 +787,8 @@  static int gem_context_register(struct i915_gem_context *ctx,
 	mutex_unlock(&ctx->mutex);
 
 	ctx->pid = get_task_pid(current, PIDTYPE_PID);
-	ctx->name = kasprintf(GFP_KERNEL, "%s[%d]",
-			      current->comm, pid_nr(ctx->pid));
-	if (!ctx->name) {
-		ret = -ENOMEM;
-		goto err_pid;
-	}
+	snprintf(ctx->name, sizeof(ctx->name), "%s[%d]",
+		 current->comm, pid_nr(ctx->pid));
 
 	/* And finally expose ourselves to userspace via the idr */
 	mutex_lock(&fpriv->context_idr_lock);
@@ -803,8 +797,6 @@  static int gem_context_register(struct i915_gem_context *ctx,
 	if (ret >= 0)
 		goto out;
 
-	kfree(fetch_and_zero(&ctx->name));
-err_pid:
 	put_pid(fetch_and_zero(&ctx->pid));
 out:
 	return ret;
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 861d7d92fe9f..69df5459c350 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
@@ -100,15 +100,6 @@  struct i915_gem_context {
 	 */
 	struct pid *pid;
 
-	/**
-	 * @name: arbitrary name
-	 *
-	 * A name is constructed for the context from the creator's process
-	 * name, pid and user handle in order to uniquely identify the
-	 * context in messages.
-	 */
-	const char *name;
-
 	/** link: place with &drm_i915_private.context_list */
 	struct list_head link;
 	struct llist_node free_link;
@@ -176,6 +167,15 @@  struct i915_gem_context {
 	 * per vm, which may be one per context or shared with the global GTT)
 	 */
 	struct radix_tree_root handles_vma;
+
+	/**
+	 * @name: arbitrary name, used for user debug
+	 *
+	 * A name is constructed for the context from the creator's process
+	 * name, pid and user handle in order to uniquely identify the
+	 * context in messages.
+	 */
+	char name[TASK_COMM_LEN + 8];
 };
 
 #endif /* __I915_GEM_CONTEXT_TYPES_H__ */
diff --git a/drivers/gpu/drm/i915/gem/selftests/mock_context.c b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
index cdcb006321a7..53e89efb09c0 100644
--- a/drivers/gpu/drm/i915/gem/selftests/mock_context.c
+++ b/drivers/gpu/drm/i915/gem/selftests/mock_context.c
@@ -37,9 +37,7 @@  mock_context(struct drm_i915_private *i915,
 	if (name) {
 		struct i915_ppgtt *ppgtt;
 
-		ctx->name = kstrdup(name, GFP_KERNEL);
-		if (!ctx->name)
-			goto err_put;
+		strncpy(ctx->name, name, sizeof(ctx->name));
 
 		ppgtt = mock_ppgtt(i915, name);
 		if (!ppgtt)