[PATCH 1/4] sync_file: add a mutex to protect fence and callback members. (v3)

[Daniel Vetter]

On Mon, Mar 20, 2017 at 08:16:36AM +0000, Chris Wilson wrote:
> On Mon, Mar 20, 2017 at 05:03:04PM +1000, Dave Airlie wrote:
> > From: Dave Airlie <airlied at redhat.com>
> > 
> > This patch allows the underlying fence in a sync_file to be changed
> > or set to NULL. This isn't currently required but for Vulkan
> > semaphores we need to be able to swap and reset the fence.
> > 
> > In order to faciliate this, it uses rcu to protect the fence,
> > along with a new mutex. The mutex also protects the callback.
> > It also checks for NULL when retrieving the rcu protected
> > fence in case it has been reset.
> > 
> > v1.1: fix the locking (Julia Lawall).
> > v2: use rcu try one
> > v3: fix poll to use proper rcu, fixup merge/fill ioctls
> > to not crash on NULL fence cases.
> > 
> > Signed-off-by: Dave Airlie <airlied at redhat.com>
> > ---
> > @@ -80,7 +87,9 @@ struct sync_file *sync_file_create(struct dma_fence *fence)
> >  	if (!sync_file)
> >  		return NULL;
> >  
> > -	sync_file->fence = dma_fence_get(fence);
> > +	dma_fence_get(fence);
> > +
> > +	RCU_INIT_POINTER(sync_file->fence, fence);
> >  
> >  	snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%llu-%d",
> >  		 fence->ops->get_driver_name(fence),
> > @@ -124,13 +133,26 @@ struct dma_fence *sync_file_get_fence(int fd)
> >  	if (!sync_file)
> >  		return NULL;
> >  
> > -	fence = dma_fence_get(sync_file->fence);
> > +	if (!rcu_access_pointer(sync_file->fence))
> > +		return NULL;
> 
> Missed fput.
> 
> > +
> > +	rcu_read_lock();
> > +	fence = dma_fence_get_rcu_safe(&sync_file->fence);
> > +	rcu_read_unlock();
> > +
> >  	fput(sync_file->file);
> >  
> >  	return fence;
> >  }
> >  EXPORT_SYMBOL(sync_file_get_fence);
> >  
> > @@ -204,10 +234,16 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
> >  	if (!sync_file)
> >  		return NULL;
> >  
> > +	mutex_lock(&a->lock);
> > +	mutex_lock(&b->lock);
> 
> This allows userspace to trigger lockdep (just merge the same pair of
> sync_files again in opposite order). if (b < a) swap(a, b); ?

Do we even need the look? 1) rcu-lookup the fences (which are invariant)
2) merge them 3) create new syncfile for them. I don't see a need for
taking locks here.

> 
> >  	a_fences = get_fences(a, &a_num_fences);
> >  	b_fences = get_fences(b, &b_num_fences);
> > -	if (a_num_fences > INT_MAX - b_num_fences)
> > -		return NULL;
> > +	if (!a_num_fences || !b_num_fences)
> > +		goto unlock;
> > +
> > +	if (a_num_fences > INT_MAX - b_num_fences) {
> > +		goto unlock;
> > +	}
> >  
> >  	num_fences = a_num_fences + b_num_fences;
> >  
> > @@ -281,10 +323,15 @@ static void sync_file_free(struct kref *kref)
> >  {
> >  	struct sync_file *sync_file = container_of(kref, struct sync_file,
> >  						     kref);
> > +	struct dma_fence *fence;
> > +
> 
> Somewhere, here?, it would be useful to add a comment that the rcu
> delayed free is provided by fput.
> 
> > +	fence = rcu_dereference_protected(sync_file->fence, 1);
> > +	if (fence) {
> > +		if (test_bit(POLL_ENABLED, &fence->flags))
> > +			dma_fence_remove_callback(fence, &sync_file->cb);
> > +		dma_fence_put(fence);
> > +	}
> >  
> > -	if (test_bit(POLL_ENABLED, &sync_file->fence->flags))
> > -		dma_fence_remove_callback(sync_file->fence, &sync_file->cb);
> > -	dma_fence_put(sync_file->fence);
> >  	kfree(sync_file);
> >  }
> >  
> > @@ -299,16 +346,25 @@ static int sync_file_release(struct inode *inode, struct file *file)
> >  static unsigned int sync_file_poll(struct file *file, poll_table *wait)
> >  {
> >  	struct sync_file *sync_file = file->private_data;
> > +	unsigned int ret_val = 0;
> > +	struct dma_fence *fence;
> >  
> >  	poll_wait(file, &sync_file->wq, wait);
> >  
> > -	if (!test_and_set_bit(POLL_ENABLED, &sync_file->fence->flags)) {
> > -		if (dma_fence_add_callback(sync_file->fence, &sync_file->cb,
> > -					   fence_check_cb_func) < 0)
> > -			wake_up_all(&sync_file->wq);
> > +	mutex_lock(&sync_file->lock);
> > +
> > +	fence = sync_file_get_fence_locked(sync_file);
> 
> Why do you need the locked version here and not just the rcu variant?

+1 :-) I think the lock should only be needed when you update the fence,
everywhere else we should be able to get away with rcu.

> > +	if (fence) {
> > +		if (!test_and_set_bit(POLL_ENABLED, &fence->flags)) {
> > +			if (dma_fence_add_callback(fence, &sync_file->cb,
> > +						   fence_check_cb_func) < 0)
> > +				wake_up_all(&sync_file->wq);
> > +		}
> > +		ret_val = dma_fence_is_signaled(fence) ? POLLIN : 0;
> >  	}
> > +	mutex_unlock(&sync_file->lock);
> 
> So an empty sync_file is incomplete and blocks forever? Why? It's the
> opposite behaviour to e.g. reservation_object so a quick explanation of
> how that is used by VkSemaphore will cement the differences.

Well if we reuse this for future fences then "not yet signalled" is kinda
what we want. In the end it doesn't matter until we wire up things
properly (i.e. block in sync_file_get_fence until the fence shows up).

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch