Re: [PATCH v5] dma-buf/sync-file: Defer creation of sync_file->name

From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	Daniel Vetter <daniel.vetter@ffwll.ch>
Subject: Re: [PATCH v5] dma-buf/sync-file: Defer creation of sync_file->name
Date: Mon, 22 May 2017 21:31:56 +0200	[thread overview]
Message-ID: <20170522193156.jp4ab7gi6pehjerp@phenom.ffwll.local> (raw)
In-Reply-To: <20170516111042.24719-1-chris@chris-wilson.co.uk>

On Tue, May 16, 2017 at 12:10:42PM +0100, Chris Wilson wrote:
> Constructing the name takes the majority of the time for allocating a
> sync_file to wrap a fence, and the name is very rarely used (only via
> the sync_file status user interface). To reduce the impact on the common
> path (that of creating sync_file to pass around), defer the construction
> of the name until it is first used.
> 
> v2: Update kerneldoc (kbuild test robot)
> v3: sync_debug.c was peeking at the name
> v4: Comment upon the potential race between two users of
> sync_file_get_name() and claim that such a race is below the level of
> notice. However, to prevent any future nuisance, use a global spinlock
> to serialize the assignment of the name.
> v5: Completely avoid the read/write race by only storing the name passed
> in from the user inside sync_file->user_name and passing in a buffer to
> dynamically construct the name otherwise.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> ---
>  drivers/dma-buf/sync_debug.c |  4 +++-
>  drivers/dma-buf/sync_file.c  | 39 ++++++++++++++++++++++++++++++++-------
>  include/linux/sync_file.h    |  5 +++--
>  3 files changed, 38 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c
> index 4b1731ee7458..59a3b2f8ee91 100644
> --- a/drivers/dma-buf/sync_debug.c
> +++ b/drivers/dma-buf/sync_debug.c
> @@ -132,9 +132,11 @@ static void sync_print_obj(struct seq_file *s, struct sync_timeline *obj)
>  static void sync_print_sync_file(struct seq_file *s,
>  				  struct sync_file *sync_file)
>  {
> +	char buf[128];
>  	int i;
>  
> -	seq_printf(s, "[%p] %s: %s\n", sync_file, sync_file->name,
> +	seq_printf(s, "[%p] %s: %s\n", sync_file,
> +		   sync_file_get_name(sync_file, buf, sizeof(buf)),
>  		   sync_status_str(dma_fence_get_status(sync_file->fence)));
>  
>  	if (dma_fence_is_array(sync_file->fence)) {
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index c9eb4997cfcc..d7e219d2669d 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -80,11 +80,6 @@ struct sync_file *sync_file_create(struct dma_fence *fence)
>  
>  	sync_file->fence = dma_fence_get(fence);
>  
> -	snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
> -		 fence->ops->get_driver_name(fence),
> -		 fence->ops->get_timeline_name(fence), fence->context,
> -		 fence->seqno);
> -
>  	return sync_file;
>  }
>  EXPORT_SYMBOL(sync_file_create);
> @@ -129,6 +124,36 @@ struct dma_fence *sync_file_get_fence(int fd)
>  }
>  EXPORT_SYMBOL(sync_file_get_fence);
>  
> +/**
> + * sync_file_get_name - get the name of the sync_file
> + * @sync_file:		sync_file to get the fence from
> + * @buf:		destination buffer to copy sync_file name into
> + * @len:		available size of destination buffer.
> + *
> + * Each sync_file may have a name assigned either by the user (when merging
> + * sync_files together) or created from the fence it contains. In the latter
> + * case construction of the name is deferred until use, and so requires
> + * sync_file_get_name().
> + *
> + * Returns: a string representing the name.
> + */
> +char *sync_file_get_name(struct sync_file *sync_file, char *buf, int len)
> +{
> +	if (sync_file->user_name[0]) {
> +		strlcpy(buf, sync_file->user_name, len);
> +	} else {
> +		struct dma_fence *fence = sync_file->fence;
> +
> +		snprintf(buf, len, "%s-%s%llu-%d",
> +			 fence->ops->get_driver_name(fence),
> +			 fence->ops->get_timeline_name(fence),
> +			 fence->context,
> +			 fence->seqno);
> +	}
> +
> +	return buf;
> +}
> +
>  static int sync_file_set_fence(struct sync_file *sync_file,
>  			       struct dma_fence **fences, int num_fences)
>  {
> @@ -266,7 +291,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
>  		goto err;
>  	}
>  
> -	strlcpy(sync_file->name, name, sizeof(sync_file->name));
> +	strlcpy(sync_file->user_name, name, sizeof(sync_file->user_name));
>  	return sync_file;
>  
>  err:
> @@ -419,7 +444,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
>  	}
>  
>  no_fences:
> -	strlcpy(info.name, sync_file->name, sizeof(info.name));
> +	sync_file_get_name(sync_file, info.name, sizeof(info.name));
>  	info.status = dma_fence_is_signaled(sync_file->fence);
>  	info.num_fences = num_fences;
>  
> diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
> index d37beefdfbd5..5226d62ae91b 100644
> --- a/include/linux/sync_file.h
> +++ b/include/linux/sync_file.h
> @@ -23,7 +23,7 @@
>  /**
>   * struct sync_file - sync file to export to the userspace
>   * @file:		file representing this fence
> - * @name:		name of sync_file.  Useful for debugging
> + * @user_name:		name of sync_file.  Useful for debugging
>   * @sync_file_list:	membership in global file list
>   * @wq:			wait queue for fence signaling
>   * @fence:		fence with the fences in the sync_file
> @@ -31,7 +31,7 @@
>   */
>  struct sync_file {
>  	struct file		*file;
> -	char			name[32];

Maybe inline kerneldoc an explain a bit better what's going on:

	/**
	 * @user_name:
	 *
	 * Name of the sync file provided by userspace, for merged fences.
	 * Otherwise generated through driver callbacks (in which case the
	 * entire array is 0).
	 */

Might be overkill, so not going to insist at all. Either way:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Pls poke Gustavo or Sumits to also check it and pick it up.

Thanks, Daniel

> +	char			user_name[32];
>  #ifdef CONFIG_DEBUG_FS
>  	struct list_head	sync_file_list;
>  #endif
> @@ -46,5 +46,6 @@ struct sync_file {
>  
>  struct sync_file *sync_file_create(struct dma_fence *fence);
>  struct dma_fence *sync_file_get_fence(int fd);
> +char *sync_file_get_name(struct sync_file *sync_file, char *buf, int len);
>  
>  #endif /* _LINUX_SYNC_H */
> -- 
> 2.11.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel