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

[Chris Wilson <chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn@public.gmane.org>]

On Mon, Mar 20, 2017 at 05:03:04PM +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied@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@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); ?

>  	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?

> +	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.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx