[PATCH 4/8] sync_file: add a mutex to protect fence and callback members. (v4)
Dave Airlie
airlied at gmail.com
Tue Apr 11 02:57:35 UTC 2017
On 4 April 2017 at 18:07, Christian König <deathsimple at vodafone.de> wrote:
> Am 04.04.2017 um 06:27 schrieb Dave Airlie:
>>
>> 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.
>> v4: use rcu in even more places, add missing fput
>>
>> Signed-off-by: Dave Airlie <airlied at redhat.com>
>> ---
>> drivers/dma-buf/sync_file.c | 135
>> +++++++++++++++++++++++++++++++++++---------
>> include/linux/sync_file.h | 6 +-
>> 2 files changed, 112 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
>> index 153bf03..6376f6f 100644
>> --- a/drivers/dma-buf/sync_file.c
>> +++ b/drivers/dma-buf/sync_file.c
>> @@ -28,6 +28,10 @@
>> static const struct file_operations sync_file_fops;
>> +#define sync_file_held(obj) lockdep_is_held(&(obj)->lock)
>> +#define sync_file_assert_held(obj) \
>> + lockdep_assert_held(&(obj)->lock)
>> +
>> /**
>> * sync_file_validate_type_flags - validate type/flags for support
>> * @type: type of sync file object
>> @@ -81,6 +85,10 @@ struct sync_file *sync_file_alloc(uint32_t type,
>> uint32_t flags)
>> sync_file->type = type;
>> sync_file->flags = flags;
>> +
>> + RCU_INIT_POINTER(sync_file->fence, NULL);
>> +
>> + mutex_init(&sync_file->lock);
>> return sync_file;
>> err:
>> @@ -117,7 +125,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),
>> @@ -168,13 +178,28 @@ 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)) {
>> + fput(sync_file->file);
>> + return NULL;
>> + }
>> +
>> + 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);
>> +static inline struct dma_fence *
>> +sync_file_get_fence_locked(struct sync_file *sync_file)
>> +{
>> + return rcu_dereference_protected(sync_file->fence,
>> + sync_file_held(sync_file));
>> +}
>> +
>> static int sync_file_set_fence(struct sync_file *sync_file,
>> struct dma_fence **fences, int num_fences)
>> {
>> @@ -187,7 +212,7 @@ static int sync_file_set_fence(struct sync_file
>> *sync_file,
>> * we own the reference of the dma_fence_array creation.
>> */
>> if (num_fences == 1) {
>> - sync_file->fence = fences[0];
>> + RCU_INIT_POINTER(sync_file->fence, fences[0]);
>> kfree(fences);
>> } else {
>> array = dma_fence_array_create(num_fences, fences,
>> @@ -196,24 +221,30 @@ static int sync_file_set_fence(struct sync_file
>> *sync_file,
>> if (!array)
>> return -ENOMEM;
>> - sync_file->fence = &array->base;
>> + RCU_INIT_POINTER(sync_file->fence, &array->base);
>> }
>> return 0;
>> }
>> -static struct dma_fence **get_fences(struct sync_file *sync_file,
>> +/* must be called with rcu read lock taken */
>> +static struct dma_fence **get_fences(struct dma_fence **fence,
>> int *num_fences)
>> {
>> - if (dma_fence_is_array(sync_file->fence)) {
>> - struct dma_fence_array *array =
>> to_dma_fence_array(sync_file->fence);
>> + if (!*fence) {
>> + *num_fences = 0;
>> + return NULL;
>> + }
>> +
>> + if (dma_fence_is_array(*fence)) {
>> + struct dma_fence_array *array =
>> to_dma_fence_array(*fence);
>> *num_fences = array->num_fences;
>> return array->fences;
>> }
>> *num_fences = 1;
>> - return &sync_file->fence;
>> + return fence;
>> }
>> static void add_fence(struct dma_fence **fences,
>> @@ -243,18 +274,31 @@ static struct sync_file *sync_file_merge(const char
>> *name, struct sync_file *a,
>> struct sync_file *sync_file;
>> struct dma_fence **fences, **nfences, **a_fences, **b_fences;
>> int i, i_a, i_b, num_fences, a_num_fences, b_num_fences;
>> + struct dma_fence *a_fence, *b_fence;
>> if (a->type != b->type)
>> return NULL;
>> - sync_file = sync_file_alloc(a->type, a->flags);
>> - if (!sync_file)
>> + if (!rcu_access_pointer(a->fence) ||
>> + !rcu_access_pointer(b->fence))
>> return NULL;
>> - a_fences = get_fences(a, &a_num_fences);
>> - b_fences = get_fences(b, &b_num_fences);
>> + rcu_read_lock();
>> + a_fence = dma_fence_get_rcu_safe(&a->fence);
>> + b_fence = dma_fence_get_rcu_safe(&b->fence);
>> + rcu_read_unlock();
>> +
>> + a_fences = get_fences(&a_fence, &a_num_fences);
>> + b_fences = get_fences(&b_fence, &b_num_fences);
>> + if (!a_num_fences || !b_num_fences)
>> + goto put_src_fences;
>> +
>> if (a_num_fences > INT_MAX - b_num_fences)
>> - return NULL;
>> + goto put_src_fences;
>> +
>> + sync_file = sync_file_alloc(a->type, a->flags);
>> + if (!sync_file)
>> + goto put_src_fences;
>> num_fences = a_num_fences + b_num_fences;
>> @@ -315,11 +359,16 @@ static struct sync_file *sync_file_merge(const
>> char *name, struct sync_file *a,
>> goto err;
>> }
>> + dma_fence_put(a_fence);
>> + dma_fence_put(b_fence);
>> strlcpy(sync_file->name, name, sizeof(sync_file->name));
>> return sync_file;
>> err:
>> fput(sync_file->file);
>> +put_src_fences:
>> + dma_fence_put(a_fence);
>> + dma_fence_put(b_fence);
>> return NULL;
>> }
>> @@ -328,10 +377,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;
>> +
>> + 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);
>> }
>> @@ -346,16 +400,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);
>> + 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);
>
>
> Maybe I'm a bit confused, but why exactly are you taking the lock here? It
> isn't protecting anything anymore as far as I can see.
>
> Or is it to prevent concurrent adding of the same callback? That would have
> failed before if I'm not completely mistaken.
The lock is to protect the callback change, the fence lookup is opportunistic
since we already have the lock.
I'm not sure what the old code would do here, but it looks like it
would have just
been racy, not sure adding the lock will cause it to do anything different.
Dave.
More information about the dri-devel
mailing list