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