Re: [Intel-gfx] [PATCH v3] dma-buf/sync-file: Defer creation of sync_file->name

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v3] dma-buf/sync-file: Defer creation of sync_file->name
Date: Mon, 15 May 2017 09:53:24 +0100	[thread overview]
Message-ID: <20170515085324.GA19912@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <20170515070129.grvnp3umlm2yvdip@phenom.ffwll.local>

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