* [PATCH] dma-buf/sync-file: Defer creation of sync_file->name @ 2017-05-12 11:30 Chris Wilson 2017-05-12 12:48 ` kbuild test robot ` (6 more replies) 0 siblings, 7 replies; 18+ messages in thread From: Chris Wilson @ 2017-05-12 11:30 UTC (permalink / raw) To: dri-devel; +Cc: intel-gfx, Sumit Semwal 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. 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_file.c | 18 +++++++++++------- include/linux/sync_file.h | 2 +- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index 2321035f6204..2cccfa834778 100644 --- a/drivers/dma-buf/sync_file.c +++ b/drivers/dma-buf/sync_file.c @@ -82,11 +82,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); @@ -268,7 +263,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: @@ -422,7 +417,16 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file, } no_fences: - strlcpy(info.name, sync_file->name, sizeof(info.name)); + 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); + } + strlcpy(info.name, sync_file->user_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 3e3ab84fc4cd..0cbeff29f510 100644 --- a/include/linux/sync_file.h +++ b/include/linux/sync_file.h @@ -34,7 +34,7 @@ struct sync_file { struct file *file; struct kref kref; - char name[32]; + char user_name[32]; #ifdef CONFIG_DEBUG_FS struct list_head sync_file_list; #endif -- 2.11.0 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] dma-buf/sync-file: Defer creation of sync_file->name 2017-05-12 11:30 [PATCH] dma-buf/sync-file: Defer creation of sync_file->name Chris Wilson @ 2017-05-12 12:48 ` kbuild test robot 2017-05-12 14:39 ` Gustavo Padovan ` (5 subsequent siblings) 6 siblings, 0 replies; 18+ messages in thread From: kbuild test robot @ 2017-05-12 12:48 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx, kbuild-all, dri-devel [-- Attachment #1: Type: text/plain, Size: 11141 bytes --] Hi Chris, [auto build test WARNING on linus/master] [also build test WARNING on v4.11 next-20170512] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Chris-Wilson/dma-buf-sync-file-Defer-creation-of-sync_file-name/20170512-193751 reproduce: make htmldocs All warnings (new ones prefixed by >>): WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org) arch/x86/include/asm/uaccess_32.h:1: warning: no structured comments found include/linux/init.h:1: warning: no structured comments found include/linux/mod_devicetable.h:686: warning: Excess struct/union/enum/typedef member 'ver_major' description in 'fsl_mc_device_id' include/linux/mod_devicetable.h:686: warning: Excess struct/union/enum/typedef member 'ver_minor' description in 'fsl_mc_device_id' kernel/sched/core.c:2088: warning: No description found for parameter 'rf' kernel/sched/core.c:2088: warning: Excess function parameter 'cookie' description in 'try_to_wake_up_local' include/linux/kthread.h:26: warning: Excess function parameter '...' description in 'kthread_create' kernel/sys.c:1: warning: no structured comments found include/linux/device.h:969: warning: No description found for parameter 'dma_ops' drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found >> include/linux/sync_file.h:47: warning: No description found for parameter 'user_name' >> include/linux/sync_file.h:47: warning: Excess struct/union/enum/typedef member 'name' description in 'sync_file' include/linux/iio/iio.h:597: warning: No description found for parameter 'trig_readonly' include/linux/iio/trigger.h:151: warning: No description found for parameter 'indio_dev' include/linux/iio/trigger.h:151: warning: No description found for parameter 'trig' include/linux/device.h:970: warning: No description found for parameter 'dma_ops' include/linux/usb/gadget.h:230: warning: No description found for parameter 'claimed' include/linux/usb/gadget.h:230: warning: No description found for parameter 'enabled' include/linux/usb/gadget.h:408: warning: No description found for parameter 'quirk_altset_not_supp' include/linux/usb/gadget.h:408: warning: No description found for parameter 'quirk_stall_not_supp' include/linux/usb/gadget.h:408: warning: No description found for parameter 'quirk_zlp_not_supp' include/drm/drm_drv.h:524: warning: No description found for parameter 'set_busid' include/drm/drm_drv.h:524: warning: No description found for parameter 'irq_handler' include/drm/drm_drv.h:524: warning: No description found for parameter 'irq_preinstall' include/drm/drm_drv.h:524: warning: No description found for parameter 'irq_postinstall' include/drm/drm_drv.h:524: warning: No description found for parameter 'irq_uninstall' include/drm/drm_drv.h:524: warning: No description found for parameter 'debugfs_init' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_open_object' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_close_object' include/drm/drm_drv.h:524: warning: No description found for parameter 'prime_handle_to_fd' include/drm/drm_drv.h:524: warning: No description found for parameter 'prime_fd_to_handle' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_export' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_import' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_pin' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_unpin' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_res_obj' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_get_sg_table' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_import_sg_table' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_vmap' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_vunmap' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_mmap' include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_vm_ops' include/drm/drm_drv.h:524: warning: No description found for parameter 'major' include/drm/drm_drv.h:524: warning: No description found for parameter 'minor' include/drm/drm_drv.h:524: warning: No description found for parameter 'patchlevel' include/drm/drm_drv.h:524: warning: No description found for parameter 'name' include/drm/drm_drv.h:524: warning: No description found for parameter 'desc' include/drm/drm_drv.h:524: warning: No description found for parameter 'date' include/drm/drm_drv.h:524: warning: No description found for parameter 'driver_features' include/drm/drm_drv.h:524: warning: No description found for parameter 'ioctls' include/drm/drm_drv.h:524: warning: No description found for parameter 'num_ioctls' include/drm/drm_drv.h:524: warning: No description found for parameter 'fops' include/drm/drm_color_mgmt.h:1: warning: no structured comments found drivers/gpu/drm/drm_plane_helper.c:403: warning: No description found for parameter 'ctx' drivers/gpu/drm/drm_plane_helper.c:404: warning: No description found for parameter 'ctx' drivers/gpu/drm/i915/intel_lpe_audio.c:350: warning: No description found for parameter 'dp_output' drivers/gpu/drm/i915/intel_lpe_audio.c:350: warning: No description found for parameter 'link_rate' drivers/gpu/drm/i915/intel_lpe_audio.c:351: warning: No description found for parameter 'dp_output' drivers/gpu/drm/i915/intel_lpe_audio.c:351: warning: No description found for parameter 'link_rate' Documentation/core-api/assoc_array.rst:13: WARNING: Enumerated list ends without a blank line; unexpected unindent. Documentation/doc-guide/sphinx.rst:126: ERROR: Unknown target name: "sphinx c domain". kernel/sched/fair.c:7650: WARNING: Inline emphasis start-string without end-string. kernel/time/timer.c:1200: ERROR: Unexpected indentation. kernel/time/timer.c:1202: ERROR: Unexpected indentation. kernel/time/timer.c:1203: WARNING: Block quote ends without a blank line; unexpected unindent. include/linux/wait.h:122: WARNING: Block quote ends without a blank line; unexpected unindent. include/linux/wait.h:125: ERROR: Unexpected indentation. include/linux/wait.h:127: WARNING: Block quote ends without a blank line; unexpected unindent. kernel/time/hrtimer.c:990: WARNING: Block quote ends without a blank line; unexpected unindent. kernel/signal.c:322: WARNING: Inline literal start-string without end-string. include/linux/iio/iio.h:219: ERROR: Unexpected indentation. include/linux/iio/iio.h:220: WARNING: Block quote ends without a blank line; unexpected unindent. include/linux/iio/iio.h:226: WARNING: Definition list ends without a blank line; unexpected unindent. drivers/iio/industrialio-core.c:638: ERROR: Unknown target name: "iio_val". drivers/iio/industrialio-core.c:645: ERROR: Unknown target name: "iio_val". drivers/message/fusion/mptbase.c:5051: WARNING: Definition list ends without a blank line; unexpected unindent. drivers/tty/serial/serial_core.c:1898: WARNING: Definition list ends without a blank line; unexpected unindent. drivers/pci/pci.c:3456: ERROR: Unexpected indentation. include/linux/regulator/driver.h:271: ERROR: Unknown target name: "regulator_regmap_x_voltage". include/linux/spi/spi.h:370: ERROR: Unexpected indentation. drivers/gpu/drm/drm_scdc_helper.c:203: ERROR: Unexpected indentation. drivers/gpu/drm/drm_scdc_helper.c:204: WARNING: Block quote ends without a blank line; unexpected unindent. drivers/gpu/drm/drm_ioctl.c:690: WARNING: Definition list ends without a blank line; unexpected unindent. Documentation/gpu/todo.rst:111: ERROR: Unknown target name: "drm_fb". sound/soc/soc-core.c:2670: ERROR: Unknown target name: "snd_soc_daifmt". sound/core/jack.c:312: ERROR: Unknown target name: "snd_jack_btn". Documentation/usb/typec.rst:116: ERROR: Error in "kernel-doc" directive: invalid option block. vim +/user_name +47 include/linux/sync_file.h a02b9dc90 include/linux/sync_file.h Gustavo Padovan 2016-08-05 31 * @fence: fence with the fences in the sync_file a02b9dc90 include/linux/sync_file.h Gustavo Padovan 2016-08-05 32 * @cb: fence callback information d4cab38e1 drivers/staging/android/sync_file.h Gustavo Padovan 2016-04-28 33 */ d4cab38e1 drivers/staging/android/sync_file.h Gustavo Padovan 2016-04-28 34 struct sync_file { d4cab38e1 drivers/staging/android/sync_file.h Gustavo Padovan 2016-04-28 35 struct file *file; d4cab38e1 drivers/staging/android/sync_file.h Gustavo Padovan 2016-04-28 36 struct kref kref; 184ed11f9 include/linux/sync_file.h Chris Wilson 2017-05-12 37 char user_name[32]; d4cab38e1 drivers/staging/android/sync_file.h Gustavo Padovan 2016-04-28 38 #ifdef CONFIG_DEBUG_FS d4cab38e1 drivers/staging/android/sync_file.h Gustavo Padovan 2016-04-28 39 struct list_head sync_file_list; d4cab38e1 drivers/staging/android/sync_file.h Gustavo Padovan 2016-04-28 40 #endif d4cab38e1 drivers/staging/android/sync_file.h Gustavo Padovan 2016-04-28 41 d4cab38e1 drivers/staging/android/sync_file.h Gustavo Padovan 2016-04-28 42 wait_queue_head_t wq; d4cab38e1 drivers/staging/android/sync_file.h Gustavo Padovan 2016-04-28 43 f54d18670 include/linux/sync_file.h Chris Wilson 2016-10-25 44 struct dma_fence *fence; f54d18670 include/linux/sync_file.h Chris Wilson 2016-10-25 45 struct dma_fence_cb cb; d4cab38e1 drivers/staging/android/sync_file.h Gustavo Padovan 2016-04-28 46 }; d4cab38e1 drivers/staging/android/sync_file.h Gustavo Padovan 2016-04-28 @47 f54d18670 include/linux/sync_file.h Chris Wilson 2016-10-25 48 #define POLL_ENABLED DMA_FENCE_FLAG_USER_BITS e24165537 include/linux/sync_file.h Gustavo Padovan 2016-08-05 49 f54d18670 include/linux/sync_file.h Chris Wilson 2016-10-25 50 struct sync_file *sync_file_create(struct dma_fence *fence); f54d18670 include/linux/sync_file.h Chris Wilson 2016-10-25 51 struct dma_fence *sync_file_get_fence(int fd); d4cab38e1 drivers/staging/android/sync_file.h Gustavo Padovan 2016-04-28 52 d4cab38e1 drivers/staging/android/sync_file.h Gustavo Padovan 2016-04-28 53 #endif /* _LINUX_SYNC_H */ :::::: The code at line 47 was first introduced by commit :::::: d4cab38e153d62ecd502645390c0289c1b8337df staging/android: prepare sync_file for de-staging :::::: TO: Gustavo Padovan <gustavo.padovan@collabora.co.uk> :::::: CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 6634 bytes --] [-- Attachment #3: Type: text/plain, Size: 160 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] dma-buf/sync-file: Defer creation of sync_file->name 2017-05-12 11:30 [PATCH] dma-buf/sync-file: Defer creation of sync_file->name Chris Wilson 2017-05-12 12:48 ` kbuild test robot @ 2017-05-12 14:39 ` Gustavo Padovan 2017-05-12 18:55 ` [PATCH v3] " Chris Wilson ` (4 subsequent siblings) 6 siblings, 0 replies; 18+ messages in thread From: Gustavo Padovan @ 2017-05-12 14:39 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx, Sumit Semwal, dri-devel Hi Chris, Thanks for the patch! 2017-05-12 Chris Wilson <chris@chris-wilson.co.uk>: > 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. > > 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_file.c | 18 +++++++++++------- > include/linux/sync_file.h | 2 +- > 2 files changed, 12 insertions(+), 8 deletions(-) > > diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c > index 2321035f6204..2cccfa834778 100644 > --- a/drivers/dma-buf/sync_file.c > +++ b/drivers/dma-buf/sync_file.c > @@ -82,11 +82,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); > @@ -268,7 +263,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: > @@ -422,7 +417,16 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file, > } > > no_fences: > - strlcpy(info.name, sync_file->name, sizeof(info.name)); > + 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); > + } > + strlcpy(info.name, sync_file->user_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 3e3ab84fc4cd..0cbeff29f510 100644 > --- a/include/linux/sync_file.h > +++ b/include/linux/sync_file.h > @@ -34,7 +34,7 @@ > struct sync_file { > struct file *file; > struct kref kref; > - char name[32]; > + char user_name[32]; Looks good to me, but please re-spin it fixing the issue reported by the test bot then I can apply it. Gustavo _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3] dma-buf/sync-file: Defer creation of sync_file->name 2017-05-12 11:30 [PATCH] dma-buf/sync-file: Defer creation of sync_file->name Chris Wilson 2017-05-12 12:48 ` kbuild test robot 2017-05-12 14:39 ` Gustavo Padovan @ 2017-05-12 18:55 ` Chris Wilson 2017-05-15 7:01 ` Daniel Vetter 2017-05-15 10:10 ` [PATCH v4] " Chris Wilson 2017-05-12 19:22 ` ✓ Fi.CI.BAT: success for dma-buf/sync-file: Defer creation of sync_file->name (rev2) Patchwork ` (3 subsequent siblings) 6 siblings, 2 replies; 18+ messages in thread From: Chris Wilson @ 2017-05-12 18:55 UTC (permalink / raw) To: dri-devel; +Cc: intel-gfx, Sumit Semwal 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); + } + + 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3] dma-buf/sync-file: Defer creation of sync_file->name 2017-05-12 18:55 ` [PATCH v3] " Chris Wilson @ 2017-05-15 7:01 ` Daniel Vetter 2017-05-15 8:53 ` [Intel-gfx] " Chris Wilson 2017-05-15 10:10 ` [PATCH v4] " Chris Wilson 1 sibling, 1 reply; 18+ messages in thread From: Daniel Vetter @ 2017-05-15 7:01 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx, Sumit Semwal, dri-devel 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Intel-gfx] [PATCH v3] dma-buf/sync-file: Defer creation of sync_file->name 2017-05-15 7:01 ` Daniel Vetter @ 2017-05-15 8:53 ` Chris Wilson 0 siblings, 0 replies; 18+ messages in thread From: Chris Wilson @ 2017-05-15 8:53 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx, dri-devel 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 -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4] dma-buf/sync-file: Defer creation of sync_file->name 2017-05-12 18:55 ` [PATCH v3] " Chris Wilson 2017-05-15 7:01 ` Daniel Vetter @ 2017-05-15 10:10 ` Chris Wilson 2017-05-15 10:50 ` David Herrmann 2017-05-16 11:10 ` [PATCH v5] " Chris Wilson 1 sibling, 2 replies; 18+ messages in thread From: Chris Wilson @ 2017-05-15 10:10 UTC (permalink / raw) To: dri-devel; +Cc: Daniel Vetter, intel-gfx 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. 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> --- drivers/dma-buf/sync_debug.c | 3 ++- drivers/dma-buf/sync_file.c | 54 ++++++++++++++++++++++++++++++++++++++------ include/linux/sync_file.h | 5 ++-- 3 files changed, 52 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..0d54de79ba76 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,51 @@ 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) +{ + static DEFINE_SPINLOCK(name_lock); + + if (!sync_file->user_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); + + /* This is mildly racy, so we stage the printf into a local + * buffer and do the copy with preempt disabled to trim the + * race down to a few cycles. To completely eliminate the + * race, we do the final assignment underneath a global + * spinlock. A global lock should be adequate here for this + * rare path (called from the middle of a user sw_sync status + * ioctl and from debugfs). + */ + spin_lock(&name_lock); + if (!sync_file->user_name[0]) + memcpy(sync_file->user_name, buf, len + 1); + spin_unlock(&name_lock); + rcu_read_unlock(); + } + + 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 +306,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 +459,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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v4] dma-buf/sync-file: Defer creation of sync_file->name 2017-05-15 10:10 ` [PATCH v4] " Chris Wilson @ 2017-05-15 10:50 ` David Herrmann 2017-05-15 11:42 ` Chris Wilson 2017-05-16 11:10 ` [PATCH v5] " Chris Wilson 1 sibling, 1 reply; 18+ messages in thread From: David Herrmann @ 2017-05-15 10:50 UTC (permalink / raw) To: Chris Wilson; +Cc: Daniel Vetter, Intel Graphics Development, dri-devel Hey On Mon, May 15, 2017 at 12:10 PM, Chris Wilson <chris@chris-wilson.co.uk> 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. > > 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> > --- > drivers/dma-buf/sync_debug.c | 3 ++- > drivers/dma-buf/sync_file.c | 54 ++++++++++++++++++++++++++++++++++++++------ > include/linux/sync_file.h | 5 ++-- > 3 files changed, 52 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..0d54de79ba76 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,51 @@ 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) > +{ > + static DEFINE_SPINLOCK(name_lock); > + > + if (!sync_file->user_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); > + > + /* This is mildly racy, so we stage the printf into a local > + * buffer and do the copy with preempt disabled to trim the > + * race down to a few cycles. To completely eliminate the > + * race, we do the final assignment underneath a global > + * spinlock. A global lock should be adequate here for this > + * rare path (called from the middle of a user sw_sync status > + * ioctl and from debugfs). > + */ > + spin_lock(&name_lock); > + if (!sync_file->user_name[0]) > + memcpy(sync_file->user_name, buf, len + 1); A racing reader might get a truncated string here. Note that a racing reader does not take the lock as soon as the first byte is visible. Not sure what the name is used for, but with that code it is not reliable. Also, please zero-terminate the string... scnprintf() does not guarantee that, if it follows posix. > + spin_unlock(&name_lock); > + rcu_read_unlock(); > + } > + > + 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 +306,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 +459,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)); Don't use strlcpy(). It requires your source to be 0-terminated. Which is not the case for sync_file->user_name. Either make sure user_name is 0-terminated, or use strcpy(). Thanks David > 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 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4] dma-buf/sync-file: Defer creation of sync_file->name 2017-05-15 10:50 ` David Herrmann @ 2017-05-15 11:42 ` Chris Wilson 2017-05-15 11:51 ` David Herrmann 0 siblings, 1 reply; 18+ messages in thread From: Chris Wilson @ 2017-05-15 11:42 UTC (permalink / raw) To: David Herrmann; +Cc: Daniel Vetter, Intel Graphics Development, dri-devel On Mon, May 15, 2017 at 12:50:04PM +0200, David Herrmann wrote: > Hey > > On Mon, May 15, 2017 at 12:10 PM, Chris Wilson <chris@chris-wilson.co.uk> 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. > > > > 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> > > --- > > drivers/dma-buf/sync_debug.c | 3 ++- > > drivers/dma-buf/sync_file.c | 54 ++++++++++++++++++++++++++++++++++++++------ > > include/linux/sync_file.h | 5 ++-- > > 3 files changed, 52 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..0d54de79ba76 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,51 @@ 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) > > +{ > > + static DEFINE_SPINLOCK(name_lock); > > + > > + if (!sync_file->user_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); > > + > > + /* This is mildly racy, so we stage the printf into a local > > + * buffer and do the copy with preempt disabled to trim the > > + * race down to a few cycles. To completely eliminate the > > + * race, we do the final assignment underneath a global > > + * spinlock. A global lock should be adequate here for this > > + * rare path (called from the middle of a user sw_sync status > > + * ioctl and from debugfs). > > + */ > > + spin_lock(&name_lock); > > + if (!sync_file->user_name[0]) > > + memcpy(sync_file->user_name, buf, len + 1); > > A racing reader might get a truncated string here. Note that a racing > reader does not take the lock as soon as the first byte is visible. > Not sure what the name is used for, but with that code it is not > reliable. The comment does explain that the use case is purely debug. We may as well forget the spinlock. It is always a valid string, if incomplete, and the race is fairly immaterial. For the value of the name, I'd be quite happy to ignore it entirely and have it only set when the user provides one. > Also, please zero-terminate the string... scnprintf() does not > guarantee that, if it follows posix. But the kernel vs[c]nprintf does ensure that the string is NUL-terminated. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4] dma-buf/sync-file: Defer creation of sync_file->name 2017-05-15 11:42 ` Chris Wilson @ 2017-05-15 11:51 ` David Herrmann 0 siblings, 0 replies; 18+ messages in thread From: David Herrmann @ 2017-05-15 11:51 UTC (permalink / raw) To: Chris Wilson, David Herrmann, dri-devel, Daniel Vetter, Intel Graphics Development Hey On Mon, May 15, 2017 at 1:42 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Mon, May 15, 2017 at 12:50:04PM +0200, David Herrmann wrote: >> Hey >> >> On Mon, May 15, 2017 at 12:10 PM, Chris Wilson <chris@chris-wilson.co.uk> 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. >> > >> > 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> >> > --- >> > drivers/dma-buf/sync_debug.c | 3 ++- >> > drivers/dma-buf/sync_file.c | 54 ++++++++++++++++++++++++++++++++++++++------ >> > include/linux/sync_file.h | 5 ++-- >> > 3 files changed, 52 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..0d54de79ba76 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,51 @@ 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) >> > +{ >> > + static DEFINE_SPINLOCK(name_lock); >> > + >> > + if (!sync_file->user_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); >> > + >> > + /* This is mildly racy, so we stage the printf into a local >> > + * buffer and do the copy with preempt disabled to trim the >> > + * race down to a few cycles. To completely eliminate the >> > + * race, we do the final assignment underneath a global >> > + * spinlock. A global lock should be adequate here for this >> > + * rare path (called from the middle of a user sw_sync status >> > + * ioctl and from debugfs). >> > + */ >> > + spin_lock(&name_lock); >> > + if (!sync_file->user_name[0]) >> > + memcpy(sync_file->user_name, buf, len + 1); >> >> A racing reader might get a truncated string here. Note that a racing >> reader does not take the lock as soon as the first byte is visible. >> Not sure what the name is used for, but with that code it is not >> reliable. > > The comment does explain that the use case is purely debug. We may as > well forget the spinlock. It is always a valid string, if incomplete, and > the race is fairly immaterial. For the value of the name, I'd be quite > happy to ignore it entirely and have it only set when the user provides > one. The comment does not make it clear to me which part is protected here. Whether it is just about racing writers, or whether it actually also protects against racing readers. The lock gives the impression that it protects only against racing writers, leaving open the issue with racing readers. If that's not the case, I don't see the point of the lock, given that racing writers would actually write the same string, anyway. >> Also, please zero-terminate the string... scnprintf() does not >> guarantee that, if it follows posix. > > But the kernel vs[c]nprintf does ensure that the string is NUL-terminated. You're right, the kernel does indeed differ from posix here. Sorry for the noise. Thanks David _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v5] dma-buf/sync-file: Defer creation of sync_file->name 2017-05-15 10:10 ` [PATCH v4] " Chris Wilson 2017-05-15 10:50 ` David Herrmann @ 2017-05-16 11:10 ` Chris Wilson 2017-05-22 9:21 ` Chris Wilson 2017-05-22 19:31 ` Daniel Vetter 1 sibling, 2 replies; 18+ messages in thread From: Chris Wilson @ 2017-05-16 11:10 UTC (permalink / raw) To: dri-devel; +Cc: Daniel Vetter, intel-gfx 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]; + 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v5] dma-buf/sync-file: Defer creation of sync_file->name 2017-05-16 11:10 ` [PATCH v5] " Chris Wilson @ 2017-05-22 9:21 ` Chris Wilson 2017-05-22 19:31 ` Daniel Vetter 1 sibling, 0 replies; 18+ messages in thread From: Chris Wilson @ 2017-05-22 9:21 UTC (permalink / raw) To: dri-devel; +Cc: intel-gfx, David Herrmann, Sumit Semwal, Daniel Vetter 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 > -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5] dma-buf/sync-file: Defer creation of sync_file->name 2017-05-16 11:10 ` [PATCH v5] " Chris Wilson 2017-05-22 9:21 ` Chris Wilson @ 2017-05-22 19:31 ` Daniel Vetter 2017-05-25 1:38 ` Gustavo Padovan 1 sibling, 1 reply; 18+ messages in thread From: Daniel Vetter @ 2017-05-22 19:31 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx, dri-devel, Daniel Vetter 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v5] dma-buf/sync-file: Defer creation of sync_file->name 2017-05-22 19:31 ` Daniel Vetter @ 2017-05-25 1:38 ` Gustavo Padovan 0 siblings, 0 replies; 18+ messages in thread From: Gustavo Padovan @ 2017-05-25 1:38 UTC (permalink / raw) To: Daniel Vetter; +Cc: Daniel Vetter, intel-gfx, dri-devel 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* ✓ Fi.CI.BAT: success for dma-buf/sync-file: Defer creation of sync_file->name (rev2) 2017-05-12 11:30 [PATCH] dma-buf/sync-file: Defer creation of sync_file->name Chris Wilson ` (2 preceding siblings ...) 2017-05-12 18:55 ` [PATCH v3] " Chris Wilson @ 2017-05-12 19:22 ` Patchwork 2017-05-15 11:28 ` ✓ Fi.CI.BAT: success for dma-buf/sync-file: Defer creation of sync_file->name (rev3) Patchwork ` (2 subsequent siblings) 6 siblings, 0 replies; 18+ messages in thread From: Patchwork @ 2017-05-12 19:22 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx == Series Details == Series: dma-buf/sync-file: Defer creation of sync_file->name (rev2) URL : https://patchwork.freedesktop.org/series/24353/ State : success == Summary == Series 24353v2 dma-buf/sync-file: Defer creation of sync_file->name https://patchwork.freedesktop.org/api/1.0/series/24353/revisions/2/mbox/ Test gem_exec_flush: Subgroup basic-batch-kernel-default-uc: pass -> FAIL (fi-snb-2600) fdo#100007 fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007 fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time:440s fi-bdw-gvtdvm total:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time:436s fi-bsw-n3050 total:278 pass:242 dwarn:0 dfail:0 fail:0 skip:36 time:579s fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time:514s fi-byt-j1900 total:278 pass:254 dwarn:0 dfail:0 fail:0 skip:24 time:492s fi-byt-n2820 total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:489s fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:418s fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:420s fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time:420s fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:487s fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:468s fi-kbl-7500u total:278 pass:255 dwarn:5 dfail:0 fail:0 skip:18 time:462s fi-kbl-7560u total:278 pass:263 dwarn:5 dfail:0 fail:0 skip:10 time:569s fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:460s fi-skl-6700hq total:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time:585s fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time:469s fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:504s fi-skl-gvtdvm total:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time:441s fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:539s fi-snb-2600 total:278 pass:248 dwarn:0 dfail:0 fail:1 skip:29 time:405s fi-bxt-t5700 failed to collect. IGT log at Patchwork_4687/fi-bxt-t5700/igt.log 38d26beef9f31fa7ca983563cf86d4131defd94a drm-tip: 2017y-05m-12d-13h-15m-01s UTC integration manifest f35f9bf dma-buf/sync-file: Defer creation of sync_file->name == Logs == For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4687/ _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
* ✓ Fi.CI.BAT: success for dma-buf/sync-file: Defer creation of sync_file->name (rev3) 2017-05-12 11:30 [PATCH] dma-buf/sync-file: Defer creation of sync_file->name Chris Wilson ` (3 preceding siblings ...) 2017-05-12 19:22 ` ✓ Fi.CI.BAT: success for dma-buf/sync-file: Defer creation of sync_file->name (rev2) Patchwork @ 2017-05-15 11:28 ` Patchwork 2017-05-16 10:21 ` [PATCH] dma-buf/sync-file: Defer creation of sync_file->name kbuild test robot 2017-05-16 12:05 ` ✓ Fi.CI.BAT: success for dma-buf/sync-file: Defer creation of sync_file->name (rev4) Patchwork 6 siblings, 0 replies; 18+ messages in thread From: Patchwork @ 2017-05-15 11:28 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx == Series Details == Series: dma-buf/sync-file: Defer creation of sync_file->name (rev3) URL : https://patchwork.freedesktop.org/series/24353/ State : success == Summary == Series 24353v3 dma-buf/sync-file: Defer creation of sync_file->name https://patchwork.freedesktop.org/api/1.0/series/24353/revisions/3/mbox/ Test gem_exec_flush: Subgroup basic-batch-kernel-default-uc: fail -> PASS (fi-snb-2600) fdo#100007 Test kms_busy: Subgroup basic-flip-default-c: incomplete -> PASS (fi-bxt-j4205) fdo#101047 fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007 fdo#101047 https://bugs.freedesktop.org/show_bug.cgi?id=101047 fi-bdw-gvtdvm total:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time:433s fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time:515s fi-byt-j1900 total:278 pass:254 dwarn:0 dfail:0 fail:0 skip:24 time:495s fi-byt-n2820 total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:487s fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:418s fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time:419s fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:502s fi-kbl-7500u total:278 pass:255 dwarn:5 dfail:0 fail:0 skip:18 time:462s fi-kbl-7560u total:278 pass:263 dwarn:5 dfail:0 fail:0 skip:10 time:572s fi-skl-6700hq total:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time:577s fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time:472s fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:497s fi-skl-gvtdvm total:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time:436s fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:531s fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time:404s fi-bxt-t5700 failed to collect. IGT log at Patchwork_4694/fi-bxt-t5700/igt.log d79d2bb351aa06bc5b39c070068fea112cfa29ef drm-tip: 2017y-05m-15d-08h-55m-10s UTC integration manifest 3ce68b0 dma-buf/sync-file: Defer creation of sync_file->name == Logs == For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4694/ _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] dma-buf/sync-file: Defer creation of sync_file->name 2017-05-12 11:30 [PATCH] dma-buf/sync-file: Defer creation of sync_file->name Chris Wilson ` (4 preceding siblings ...) 2017-05-15 11:28 ` ✓ Fi.CI.BAT: success for dma-buf/sync-file: Defer creation of sync_file->name (rev3) Patchwork @ 2017-05-16 10:21 ` kbuild test robot 2017-05-16 12:05 ` ✓ Fi.CI.BAT: success for dma-buf/sync-file: Defer creation of sync_file->name (rev4) Patchwork 6 siblings, 0 replies; 18+ messages in thread From: kbuild test robot @ 2017-05-16 10:21 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx, kbuild-all, dri-devel [-- Attachment #1: Type: text/plain, Size: 2399 bytes --] Hi Chris, [auto build test ERROR on linus/master] [also build test ERROR on v4.12-rc1 next-20170516] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Chris-Wilson/dma-buf-sync-file-Defer-creation-of-sync_file-name/20170512-193751 config: x86_64-rhel (attached as .config) compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): drivers//dma-buf/sync_debug.c: In function 'sync_print_sync_file': >> drivers//dma-buf/sync_debug.c:137:53: error: 'struct sync_file' has no member named 'name' seq_printf(s, "[%p] %s: %s\n", sync_file, sync_file->name, ^~ vim +137 drivers//dma-buf/sync_debug.c 0f0d8406 drivers/staging/android/sync_debug.c Maarten Lankhorst 2014-07-01 131 d7fdb0ae drivers/staging/android/sync_debug.c Gustavo Padovan 2016-01-21 132 static void sync_print_sync_file(struct seq_file *s, d7fdb0ae drivers/staging/android/sync_debug.c Gustavo Padovan 2016-01-21 133 struct sync_file *sync_file) 0f0d8406 drivers/staging/android/sync_debug.c Maarten Lankhorst 2014-07-01 134 { 0f0d8406 drivers/staging/android/sync_debug.c Maarten Lankhorst 2014-07-01 135 int i; 0f0d8406 drivers/staging/android/sync_debug.c Maarten Lankhorst 2014-07-01 136 d7fdb0ae drivers/staging/android/sync_debug.c Gustavo Padovan 2016-01-21 @137 seq_printf(s, "[%p] %s: %s\n", sync_file, sync_file->name, d6c99f4b drivers/dma-buf/sync_debug.c Chris Wilson 2017-01-04 138 sync_status_str(dma_fence_get_status(sync_file->fence))); 0f0d8406 drivers/staging/android/sync_debug.c Maarten Lankhorst 2014-07-01 139 f54d1867 drivers/dma-buf/sync_debug.c Chris Wilson 2016-10-25 140 if (dma_fence_is_array(sync_file->fence)) { :::::: The code at line 137 was first introduced by commit :::::: d7fdb0ae9d115fa14ff3a5382c8a62de41c7786a staging/android: rename sync_fence to sync_file :::::: TO: Gustavo Padovan <gustavo.padovan@collabora.co.uk> :::::: CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 39258 bytes --] [-- Attachment #3: Type: text/plain, Size: 160 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
* ✓ Fi.CI.BAT: success for dma-buf/sync-file: Defer creation of sync_file->name (rev4) 2017-05-12 11:30 [PATCH] dma-buf/sync-file: Defer creation of sync_file->name Chris Wilson ` (5 preceding siblings ...) 2017-05-16 10:21 ` [PATCH] dma-buf/sync-file: Defer creation of sync_file->name kbuild test robot @ 2017-05-16 12:05 ` Patchwork 6 siblings, 0 replies; 18+ messages in thread From: Patchwork @ 2017-05-16 12:05 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx == Series Details == Series: dma-buf/sync-file: Defer creation of sync_file->name (rev4) URL : https://patchwork.freedesktop.org/series/24353/ State : success == Summary == Series 24353v4 dma-buf/sync-file: Defer creation of sync_file->name https://patchwork.freedesktop.org/api/1.0/series/24353/revisions/4/mbox/ Test gem_exec_flush: Subgroup basic-batch-kernel-default-uc: pass -> FAIL (fi-snb-2600) fdo#100007 Test kms_cursor_legacy: Subgroup basic-busy-flip-before-cursor-legacy: pass -> FAIL (fi-snb-2600) fdo#100215 fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007 fdo#100215 https://bugs.freedesktop.org/show_bug.cgi?id=100215 fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time:447s fi-bdw-gvtdvm total:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time:431s fi-bsw-n3050 total:278 pass:242 dwarn:0 dfail:0 fail:0 skip:36 time:582s fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time:512s fi-byt-j1900 total:278 pass:254 dwarn:0 dfail:0 fail:0 skip:24 time:495s fi-byt-n2820 total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:493s fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:423s fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:417s fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time:417s fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:506s fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:472s fi-kbl-7500u total:278 pass:255 dwarn:5 dfail:0 fail:0 skip:18 time:467s fi-kbl-7560u total:278 pass:263 dwarn:5 dfail:0 fail:0 skip:10 time:579s fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:459s fi-skl-6700hq total:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time:582s fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time:466s fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:500s fi-skl-gvtdvm total:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time:442s fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:538s fi-snb-2600 total:278 pass:247 dwarn:0 dfail:0 fail:2 skip:29 time:404s fb5cb398a048f8e0fd9dab5686caba61961131be drm-tip: 2017y-05m-16d-11h-13m-20s UTC integration manifest d559328 dma-buf/sync-file: Defer creation of sync_file->name == Logs == For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4709/ _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2017-05-25 1:38 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-05-12 11:30 [PATCH] dma-buf/sync-file: Defer creation of sync_file->name Chris Wilson 2017-05-12 12:48 ` kbuild test robot 2017-05-12 14:39 ` Gustavo Padovan 2017-05-12 18:55 ` [PATCH v3] " Chris Wilson 2017-05-15 7:01 ` Daniel Vetter 2017-05-15 8:53 ` [Intel-gfx] " Chris Wilson 2017-05-15 10:10 ` [PATCH v4] " Chris Wilson 2017-05-15 10:50 ` David Herrmann 2017-05-15 11:42 ` Chris Wilson 2017-05-15 11:51 ` David Herrmann 2017-05-16 11:10 ` [PATCH v5] " Chris Wilson 2017-05-22 9:21 ` Chris Wilson 2017-05-22 19:31 ` Daniel Vetter 2017-05-25 1:38 ` Gustavo Padovan 2017-05-12 19:22 ` ✓ Fi.CI.BAT: success for dma-buf/sync-file: Defer creation of sync_file->name (rev2) Patchwork 2017-05-15 11:28 ` ✓ Fi.CI.BAT: success for dma-buf/sync-file: Defer creation of sync_file->name (rev3) Patchwork 2017-05-16 10:21 ` [PATCH] dma-buf/sync-file: Defer creation of sync_file->name kbuild test robot 2017-05-16 12:05 ` ✓ Fi.CI.BAT: success for dma-buf/sync-file: Defer creation of sync_file->name (rev4) Patchwork
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.