| Message ID | 20170512185542.5248-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@chris-wilson.co.uk> > Cc: Sumit Semwal <sumit.semwal@linaro.org> > Cc: Gustavo Padovan <gustavo@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, May 15, 2017 at 09:01:29AM +0200, Daniel Vetter wrote: > 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@chris-wilson.co.uk> > > Cc: Sumit Semwal <sumit.semwal@linaro.org> > > Cc: Gustavo Padovan <gustavo@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. Hopefully, we don't care. It's not even a given that the fence->ops return the same names at different times. The only sore point is that this is an ioctl, so not the usual debugfs handwave. We can cut down the race somewhat if (!name[0]) { char buf[sizeof(sync_file->user_name)]; struct dma_fence *fence; int len; rcu_read_lock(); fence = sync_file->fence; len = scnprintf(buf, sizeof(buf), "%s-%s%llu-%d", fence->ops->get_driver_name(sync_file->fence), fence->ops->get_timeline_name(sync_file->fence), fence->context, fence->seqno); rcu_read_unlock(); spin_lock(&global_sync_file_name_lock); if (!sync_file->user_name[0]) memcpy(sync_file->user_name[0], buf, len + 1); spin_unlock(&global_sync_file_name_lock); } Even doing the staging to a local before doing a memcpy will be enough to trim the race to a few cycles -- and leave it under the rcu_read_unlock to keep preempt disabled. -Chris
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); + } + + 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 */
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@chris-wilson.co.uk> Cc: Sumit Semwal <sumit.semwal@linaro.org> Cc: Gustavo Padovan <gustavo@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(-)