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@xxxxxxxxxxxxxxxxxx>
> Cc: Sumit Semwal <sumit.semwal@xxxxxxxxxx>
> Cc: Gustavo Padovan <gustavo@xxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: David Herrmann <dh.herrmann@xxxxxxxxx>
> ---
> 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@xxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
