| Message ID | 20170516111042.24719-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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> Any thoughts on this one? It should completely avoid the race, with the presumption that unless the user sets a name when merging the best response is to show the fence name. It may be interesting to actually show both; the encoded sync_file->fence name and the user's identifier. -Chris > --- > 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]; > + 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 >
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 >
2017-05-22 Daniel Vetter <daniel@ffwll.ch>: > 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> I fixed it. Applied with the modification proposed by Daniel. Thanks for the patch. Gustavo
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]; + 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 */
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(-)