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