[Intel-gfx] [PATCH v3] dma-buf/sync-file: Defer creation of sync_file->name

[Daniel Vetter]

On Fri, May 12, 2017 at 07:55: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
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Sumit Semwal <sumit.semwal at linaro.org>
> Cc: Gustavo Padovan <gustavo at padovan.org>
> ---
>  drivers/dma-buf/sync_debug.c |  3 ++-
>  drivers/dma-buf/sync_file.c  | 34 +++++++++++++++++++++++++++-------
>  include/linux/sync_file.h    |  5 +++--
>  3 files changed, 32 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/dma-buf/sync_debug.c b/drivers/dma-buf/sync_debug.c
> index 4b1731ee7458..9a93f1085c63 100644
> --- a/drivers/dma-buf/sync_debug.c
> +++ b/drivers/dma-buf/sync_debug.c
> @@ -134,7 +134,8 @@ static void sync_print_sync_file(struct seq_file *s,
>  {
>  	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),
>  		   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..d105079ec45c 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,31 @@ 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
> + *
> + * Each sync_file may have a name assigned either by the user (when merging
> + * sync_files together) or created from the fence it contains. However,
> + * construction of the name is deferred until first use.
> + *
> + * Returns: a string representing the name
> + */
> +char *sync_file_get_name(struct sync_file *sync_file)
> +{
> +	if (!sync_file->user_name[0]) {
> +		scnprintf(sync_file->user_name,
> +			  sizeof(sync_file->user_name),
> +			  "%s-%s%llu-%d",
> +			  sync_file->fence->ops->get_driver_name(sync_file->fence),
> +			  sync_file->fence->ops->get_timeline_name(sync_file->fence),
> +			  sync_file->fence->context,
> +			  sync_file->fence->seqno);
> +	}

This is mildly race. Do we care? Deserves at least a comment that we don't
care, with that ack: me.
-Daniel

> +
> +	return sync_file->user_name;
> +}
> +
>  static int sync_file_set_fence(struct sync_file *sync_file,
>  			       struct dma_fence **fences, int num_fences)
>  {
> @@ -266,7 +286,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 +439,7 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file,
>  	}
>  
>  no_fences:
> -	strlcpy(info.name, sync_file->name, sizeof(info.name));
> +	strlcpy(info.name, sync_file_get_name(sync_file), 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..0f3f05325c84 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];
> +	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);
>  
>  #endif /* _LINUX_SYNC_H */
> -- 
> 2.11.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch