Re: [PATCH 1/4] sync_file: add a mutex to protect fence and callback members. (v3)
On Mon, Mar 20, 2017 at 08:16:36AM +, Chris Wilson wrote:
> On Mon, Mar 20, 2017 at 05:03:04PM +1000, Dave Airlie wrote:
> > From: Dave Airlie
> >
> > 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
> > ---
> > @@ -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
>
Re: [PATCH 1/4] sync_file: add a mutex to protect fence and callback members. (v3)
On Mon, Mar 20, 2017 at 05:03:04PM +1000, Dave Airlie wrote: > From: Dave Airlie > > 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 > --- > @@ -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; > + > + rcu_read_lock(); > + fence = dma_fence_get_rcu_safe(&sync_file->fence); > + rcu_read_unlock(); > + > fput(sync_file->file); So poll will wait until the fence is set before the sync_file is signaled, but here we return NULL. At the moment this is interpretted by the callers as an error (since we can't distinguish between the lookup error and the empty sync_file). However, if it is empty we also want to delay the dependent execution until the fence is set to match the poll semantics. -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
Re: [PATCH 1/4] sync_file: add a mutex to protect fence and callback members. (v3)
On Mon, Mar 20, 2017 at 05:03:04PM +1000, Dave Airlie wrote:
> From: Dave Airlie
>
> 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
> ---
> @@ -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
[PATCH 1/4] sync_file: add a mutex to protect fence and callback members. (v3)
From: Dave Airlie
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
---
drivers/dma-buf/sync_file.c | 112 +++-
include/linux/sync_file.h | 5 +-
2 files changed, 94 insertions(+), 23 deletions(-)
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 2321035..dcba931 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)
+
static struct sync_file *sync_file_alloc(void)
{
struct sync_file *sync_file;
@@ -47,6 +51,9 @@ static struct sync_file *sync_file_alloc(void)
INIT_LIST_HEAD(&sync_file->cb.node);
+ RCU_INIT_POINTER(sync_file->fence, NULL);
+
+ mutex_init(&sync_file->lock);
return sync_file;
err:
@@ -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;
+
+ 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)
{
@@ -143,7 +165,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,
@@ -152,17 +174,25 @@ 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;
}
+/* must be called with sync_file lock taken */
static struct dma_fence **get_fences(struct sync_file *sync_file,
int *num_fences)
{
- if (dma_fence_is_array(sync_file->fence)) {
- struct dma_fence_array *array =
to_dma_fence_array(sync_file->fence);
+ struct dma_fence *fence = sync_file_get_fence_locked(sync_file);
+
+ 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;
@@ -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);
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;
@@ -268,11 +304,17 @@ static struct sync_file *sync_file_merge(const char
*name, struct sync_file *a,
goto err;
}
+ mutex_unlock(&b->lock);
+ mutex_unlock(&a->lock);
+
strlcpy(sync_file->name, name, sizeof(sync_file->nam
[PATCH 1/4] sync_file: add a mutex to protect fence and callback members. (v22)
From: Dave Airlie
This isn't needed currently, but to reuse sync file for Vulkan
permanent shared semaphore semantics, we need to be able to swap
the fence backing a sync file. This patch adds a mutex to the
sync file and uses to protect accesses to the fence and cb members.
v1.1: fix the locking (Julia Lawall).
v2: use rcu try one
Signed-off-by: Dave Airlie
---
drivers/dma-buf/sync_file.c | 82 +++--
include/linux/sync_file.h | 5 ++-
2 files changed, 69 insertions(+), 18 deletions(-)
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 2321035..8b34f21 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)
+
static struct sync_file *sync_file_alloc(void)
{
struct sync_file *sync_file;
@@ -47,6 +51,9 @@ static struct sync_file *sync_file_alloc(void)
INIT_LIST_HEAD(&sync_file->cb.node);
+ RCU_INIT_POINTER(sync_file->fence, NULL);
+
+ mutex_init(&sync_file->lock);
return sync_file;
err:
@@ -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;
+
+ 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)
{
@@ -143,7 +165,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,
@@ -152,17 +174,20 @@ 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;
}
+/* must be called with sync_file lock taken */
static struct dma_fence **get_fences(struct sync_file *sync_file,
int *num_fences)
{
- if (dma_fence_is_array(sync_file->fence)) {
- struct dma_fence_array *array =
to_dma_fence_array(sync_file->fence);
+ struct dma_fence *fence = sync_file_get_fence_locked(sync_file);
+
+ if (dma_fence_is_array(fence)) {
+ struct dma_fence_array *array = to_dma_fence_array(fence);
*num_fences = array->num_fences;
return array->fences;
@@ -204,10 +229,13 @@ 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);
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 > INT_MAX - b_num_fences) {
+ goto unlock;
+ }
num_fences = a_num_fences + b_num_fences;
@@ -268,11 +296,17 @@ static struct sync_file *sync_file_merge(const char
*name, struct sync_file *a,
goto err;
}
+ mutex_unlock(&b->lock);
+ mutex_unlock(&a->lock);
+
strlcpy(sync_file->name, name, sizeof(sync_file->name));
return sync_file;
err:
fput(sync_file->file);
+unlock:
+ mutex_unlock(&b->lock);
+ mutex_unlock(&a->lock);
return NULL;
}
@@ -281,10 +315,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;
Re: [PATCH 1/4] sync_file: add a mutex to protect fence and callback members.
On 15 March 2017 at 10:47, Dave Airlie wrote: > On 14 March 2017 at 19:30, Christian König wrote: >> Am 14.03.2017 um 10:29 schrieb Chris Wilson: >>> >>> On Tue, Mar 14, 2017 at 10:13:37AM +0100, Christian König wrote: Am 14.03.2017 um 09:45 schrieb Daniel Vetter: > > On Tue, Mar 14, 2017 at 10:50:51AM +1000, Dave Airlie wrote: >> >> From: Dave Airlie >> >> This isn't needed currently, but to reuse sync file for Vulkan >> permanent shared semaphore semantics, we need to be able to swap >> the fence backing a sync file. This patch adds a mutex to the >> sync file and uses to protect accesses to the fence and cb members. >> >> Signed-off-by: Dave Airlie > > We've gone to pretty great lengths to rcu-protect all the fence stuff, > so > that a peek-only is entirely lockless. Can we haz the same for this pls? Yes, just wanted to suggest the same thing. Basically you just need the following to retrieve the fence: while (sync_file->fence && !(fence = fence_get_rcu(sync_file->fence)); >>> >>> We even have a helper for that: >>> >>> fence = dma_fence_get_rcu_safe(&sync_file->fence); >>> >>> (Still going to suggest using a reservation_object rather than an >>> exclusive-only implementation.) >> >> >> Yeah, thought about that as well. But the reservation object doesn't seem to >> match the required userspace semantic. >> >> E.g. you actually don't want more than one fence it in as far as I >> understand it. > > I don't think a reservation object is what the vulkan semantics ask for. > I suppose a reservation object with a single exclusive fence is close enough, just wouldn't want to create one with non-exclusive fences on it. Dave. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/4] sync_file: add a mutex to protect fence and callback members.
On 14 March 2017 at 19:30, Christian König wrote: > Am 14.03.2017 um 10:29 schrieb Chris Wilson: >> >> On Tue, Mar 14, 2017 at 10:13:37AM +0100, Christian König wrote: >>> >>> Am 14.03.2017 um 09:45 schrieb Daniel Vetter: On Tue, Mar 14, 2017 at 10:50:51AM +1000, Dave Airlie wrote: > > From: Dave Airlie > > This isn't needed currently, but to reuse sync file for Vulkan > permanent shared semaphore semantics, we need to be able to swap > the fence backing a sync file. This patch adds a mutex to the > sync file and uses to protect accesses to the fence and cb members. > > Signed-off-by: Dave Airlie We've gone to pretty great lengths to rcu-protect all the fence stuff, so that a peek-only is entirely lockless. Can we haz the same for this pls? >>> >>> Yes, just wanted to suggest the same thing. >>> >>> Basically you just need the following to retrieve the fence: >>> >>> while (sync_file->fence && !(fence = fence_get_rcu(sync_file->fence)); >> >> We even have a helper for that: >> >> fence = dma_fence_get_rcu_safe(&sync_file->fence); >> >> (Still going to suggest using a reservation_object rather than an >> exclusive-only implementation.) > > > Yeah, thought about that as well. But the reservation object doesn't seem to > match the required userspace semantic. > > E.g. you actually don't want more than one fence it in as far as I > understand it. I don't think a reservation object is what the vulkan semantics ask for. Dave. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/4] sync_file: add a mutex to protect fence and callback members. (fwd)
Perhaps the mutex on line 410 needs to be considered on line 423. julia -- Forwarded message -- Hi Dave, [auto build test WARNING on drm/drm-next] [also build test WARNING on v4.11-rc2 next-20170310] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Dave-Airlie/sync_file-add-a-mutex-to-protect-fence-and-callback-members/20170314-155609 base: git://people.freedesktop.org/~airlied/linux.git drm-next :: branch date: 52 minutes ago :: commit date: 52 minutes ago >> drivers/dma-buf/sync_file.c:423:2-8: preceding lock on line 410 git remote add linux-review https://github.com/0day-ci/linux git remote update linux-review git checkout a319d478cdd641742a07f809ddb7c143a0a685e9 vim +423 drivers/dma-buf/sync_file.c d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28 404 if (copy_from_user(&info, (void __user *)arg, sizeof(info))) d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28 405 return -EFAULT; d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28 406 d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28 407 if (info.flags || info.pad) d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28 408 return -EINVAL; d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28 409 a319d478 drivers/dma-buf/sync_file.c Dave Airlie 2017-03-14 @410 mutex_lock(&sync_file->lock); a02b9dc9 drivers/dma-buf/sync_file.c Gustavo Padovan 2016-08-05 411 fences = get_fences(sync_file, &num_fences); a02b9dc9 drivers/dma-buf/sync_file.c Gustavo Padovan 2016-08-05 412 d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28 413 /* d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28 414 * Passing num_fences = 0 means that userspace doesn't want to d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28 415 * retrieve any sync_fence_info. If num_fences = 0 we skip filling d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28 416 * sync_fence_info and return the actual number of fences on d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28 417 * info->num_fences. d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28 418 */ d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28 419 if (!info.num_fences) d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28 420 goto no_fences; d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28 421 a02b9dc9 drivers/dma-buf/sync_file.c Gustavo Padovan 2016-08-05 422 if (info.num_fences < num_fences) d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28 @423 return -EINVAL; d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28 424 a02b9dc9 drivers/dma-buf/sync_file.c Gustavo Padovan 2016-08-05 425 size = num_fences * sizeof(*fence_info); d4cab38e drivers/staging/android/sync_file.c Gustavo Padovan 2016-04-28 426 fence_info = kzalloc(size, GFP_KERNEL); :: The code at line 423 was first introduced by commit :: d4cab38e153d62ecd502645390c0289c1b8337df staging/android: prepare sync_file for de-staging :: TO: Gustavo Padovan :: CC: Greg Kroah-Hartman --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/4] sync_file: add a mutex to protect fence and callback members.
Am 14.03.2017 um 10:29 schrieb Chris Wilson: On Tue, Mar 14, 2017 at 10:13:37AM +0100, Christian König wrote: Am 14.03.2017 um 09:45 schrieb Daniel Vetter: On Tue, Mar 14, 2017 at 10:50:51AM +1000, Dave Airlie wrote: From: Dave Airlie This isn't needed currently, but to reuse sync file for Vulkan permanent shared semaphore semantics, we need to be able to swap the fence backing a sync file. This patch adds a mutex to the sync file and uses to protect accesses to the fence and cb members. Signed-off-by: Dave Airlie We've gone to pretty great lengths to rcu-protect all the fence stuff, so that a peek-only is entirely lockless. Can we haz the same for this pls? Yes, just wanted to suggest the same thing. Basically you just need the following to retrieve the fence: while (sync_file->fence && !(fence = fence_get_rcu(sync_file->fence)); We even have a helper for that: fence = dma_fence_get_rcu_safe(&sync_file->fence); (Still going to suggest using a reservation_object rather than an exclusive-only implementation.) Yeah, thought about that as well. But the reservation object doesn't seem to match the required userspace semantic. E.g. you actually don't want more than one fence it in as far as I understand it. Christian. -Chris ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/4] sync_file: add a mutex to protect fence and callback members.
On Tue, Mar 14, 2017 at 10:13:37AM +0100, Christian König wrote: > Am 14.03.2017 um 09:45 schrieb Daniel Vetter: > >On Tue, Mar 14, 2017 at 10:50:51AM +1000, Dave Airlie wrote: > >>From: Dave Airlie > >> > >>This isn't needed currently, but to reuse sync file for Vulkan > >>permanent shared semaphore semantics, we need to be able to swap > >>the fence backing a sync file. This patch adds a mutex to the > >>sync file and uses to protect accesses to the fence and cb members. > >> > >>Signed-off-by: Dave Airlie > >We've gone to pretty great lengths to rcu-protect all the fence stuff, so > >that a peek-only is entirely lockless. Can we haz the same for this pls? > > Yes, just wanted to suggest the same thing. > > Basically you just need the following to retrieve the fence: > > while (sync_file->fence && !(fence = fence_get_rcu(sync_file->fence)); We even have a helper for that: fence = dma_fence_get_rcu_safe(&sync_file->fence); (Still going to suggest using a reservation_object rather than an exclusive-only implementation.) -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
Re: [PATCH 1/4] sync_file: add a mutex to protect fence and callback members.
Am 14.03.2017 um 09:45 schrieb Daniel Vetter:
On Tue, Mar 14, 2017 at 10:50:51AM +1000, Dave Airlie wrote:
From: Dave Airlie
This isn't needed currently, but to reuse sync file for Vulkan
permanent shared semaphore semantics, we need to be able to swap
the fence backing a sync file. This patch adds a mutex to the
sync file and uses to protect accesses to the fence and cb members.
Signed-off-by: Dave Airlie
We've gone to pretty great lengths to rcu-protect all the fence stuff, so
that a peek-only is entirely lockless. Can we haz the same for this pls?
Yes, just wanted to suggest the same thing.
Basically you just need the following to retrieve the fence:
while (sync_file->fence && !(fence = fence_get_rcu(sync_file->fence));
And then only taking a look when replacing it.
Yes I know it's probably going to be slightly nasty when you get around to
implementing the replacement logic. For just normal fences we can probably
get away with not doing an rcu dance when freeing it, since the
refcounting should protect us already.
The only tricky thing I can see is the fence_callback structure in the
sync file. And that can be handled while holding the lock in the replace
function.
But for the replacement you need to have an rcu-delayed fence_put on the
old fences.
Freeing fences is RCU save anyway, see the default implementation of
fence_free().
Had to fix that in amdgpu and radeon because our private implementation
wasn't RCU save and we run into problems because of that.
So at least that should already been taken care of.
Christian.
-Daniel
---
drivers/dma-buf/sync_file.c | 23 +++
include/linux/sync_file.h | 3 +++
2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 2321035..105f48c 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -47,6 +47,7 @@ static struct sync_file *sync_file_alloc(void)
INIT_LIST_HEAD(&sync_file->cb.node);
+ mutex_init(&sync_file->lock);
return sync_file;
err:
@@ -204,10 +205,13 @@ 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);
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 > INT_MAX - b_num_fences) {
+ goto unlock;
+ }
num_fences = a_num_fences + b_num_fences;
@@ -268,11 +272,17 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a,
goto err;
}
+ mutex_unlock(&b->lock);
+ mutex_unlock(&a->lock);
+
strlcpy(sync_file->name, name, sizeof(sync_file->name));
return sync_file;
err:
fput(sync_file->file);
+unlock:
+ mutex_unlock(&b->lock);
+ mutex_unlock(&a->lock);
return NULL;
}
@@ -299,16 +309,20 @@ 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;
poll_wait(file, &sync_file->wq, wait);
+ mutex_lock(&sync_file->lock);
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);
}
+ ret_val = dma_fence_is_signaled(sync_file->fence) ? POLLIN : 0;
+ mutex_unlock(&sync_file->lock);
- return dma_fence_is_signaled(sync_file->fence) ? POLLIN : 0;
+ return ret_val;
}
static long sync_file_ioctl_merge(struct sync_file *sync_file,
@@ -393,6 +407,7 @@ static long sync_file_ioctl_fence_info(struct sync_file
*sync_file,
if (info.flags || info.pad)
return -EINVAL;
+ mutex_lock(&sync_file->lock);
fences = get_fences(sync_file, &num_fences);
/*
@@ -433,7 +448,7 @@ static long sync_file_ioctl_fence_info(struct sync_file
*sync_file,
out:
kfree(fence_info);
-
+ mutex_unlock(&sync_file->lock);
return ret;
}
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
index 3e3ab84..5aef17f 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -30,6 +30,7 @@
* @wq: wait queue for fence signaling
* @fence:fence with the fences in the sync_file
* @cb: fence callback information
+ * @lock: mutex to protect fence/cb - used for semaphores
*/
struct sync_file {
struct file *file;
@@ -43,6 +44,8 @@ struct sync_file {
struct dma_fence *fence;
s
Re: [PATCH 1/4] sync_file: add a mutex to protect fence and callback members.
On Tue, Mar 14, 2017 at 10:50:51AM +1000, Dave Airlie wrote:
> From: Dave Airlie
>
> This isn't needed currently, but to reuse sync file for Vulkan
> permanent shared semaphore semantics, we need to be able to swap
> the fence backing a sync file. This patch adds a mutex to the
> sync file and uses to protect accesses to the fence and cb members.
>
> Signed-off-by: Dave Airlie
We've gone to pretty great lengths to rcu-protect all the fence stuff, so
that a peek-only is entirely lockless. Can we haz the same for this pls?
Yes I know it's probably going to be slightly nasty when you get around to
implementing the replacement logic. For just normal fences we can probably
get away with not doing an rcu dance when freeing it, since the
refcounting should protect us already.
But for the replacement you need to have an rcu-delayed fence_put on the
old fences.
-Daniel
> ---
> drivers/dma-buf/sync_file.c | 23 +++
> include/linux/sync_file.h | 3 +++
> 2 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index 2321035..105f48c 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -47,6 +47,7 @@ static struct sync_file *sync_file_alloc(void)
>
> INIT_LIST_HEAD(&sync_file->cb.node);
>
> + mutex_init(&sync_file->lock);
> return sync_file;
>
> err:
> @@ -204,10 +205,13 @@ 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);
> 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 > INT_MAX - b_num_fences) {
> + goto unlock;
> + }
>
> num_fences = a_num_fences + b_num_fences;
>
> @@ -268,11 +272,17 @@ static struct sync_file *sync_file_merge(const char
> *name, struct sync_file *a,
> goto err;
> }
>
> + mutex_unlock(&b->lock);
> + mutex_unlock(&a->lock);
> +
> strlcpy(sync_file->name, name, sizeof(sync_file->name));
> return sync_file;
>
> err:
> fput(sync_file->file);
> +unlock:
> + mutex_unlock(&b->lock);
> + mutex_unlock(&a->lock);
> return NULL;
>
> }
> @@ -299,16 +309,20 @@ 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;
>
> poll_wait(file, &sync_file->wq, wait);
>
> + mutex_lock(&sync_file->lock);
> 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);
> }
> + ret_val = dma_fence_is_signaled(sync_file->fence) ? POLLIN : 0;
> + mutex_unlock(&sync_file->lock);
>
> - return dma_fence_is_signaled(sync_file->fence) ? POLLIN : 0;
> + return ret_val;
> }
>
> static long sync_file_ioctl_merge(struct sync_file *sync_file,
> @@ -393,6 +407,7 @@ static long sync_file_ioctl_fence_info(struct sync_file
> *sync_file,
> if (info.flags || info.pad)
> return -EINVAL;
>
> + mutex_lock(&sync_file->lock);
> fences = get_fences(sync_file, &num_fences);
>
> /*
> @@ -433,7 +448,7 @@ static long sync_file_ioctl_fence_info(struct sync_file
> *sync_file,
>
> out:
> kfree(fence_info);
> -
> + mutex_unlock(&sync_file->lock);
> return ret;
> }
>
> diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
> index 3e3ab84..5aef17f 100644
> --- a/include/linux/sync_file.h
> +++ b/include/linux/sync_file.h
> @@ -30,6 +30,7 @@
> * @wq: wait queue for fence signaling
> * @fence: fence with the fences in the sync_file
> * @cb: fence callback information
> + * @lock: mutex to protect fence/cb - used for semaphores
> */
> struct sync_file {
> struct file *file;
> @@ -43,6 +44,8 @@ struct sync_file {
>
> struct dma_fence*fence;
> struct dma_fence_cb cb;
> + /* protects the fence pointer and cb */
> + struct mutex lock;
> };
>
> #define POLL_ENABLED DMA_FENCE_FLAG_USER_BITS
> --
> 2.7.4
>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-g
[PATCH 1/4] sync_file: add a mutex to protect fence and callback members.
From: Dave Airlie
This isn't needed currently, but to reuse sync file for Vulkan
permanent shared semaphore semantics, we need to be able to swap
the fence backing a sync file. This patch adds a mutex to the
sync file and uses to protect accesses to the fence and cb members.
Signed-off-by: Dave Airlie
---
drivers/dma-buf/sync_file.c | 23 +++
include/linux/sync_file.h | 3 +++
2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index 2321035..105f48c 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -47,6 +47,7 @@ static struct sync_file *sync_file_alloc(void)
INIT_LIST_HEAD(&sync_file->cb.node);
+ mutex_init(&sync_file->lock);
return sync_file;
err:
@@ -204,10 +205,13 @@ 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);
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 > INT_MAX - b_num_fences) {
+ goto unlock;
+ }
num_fences = a_num_fences + b_num_fences;
@@ -268,11 +272,17 @@ static struct sync_file *sync_file_merge(const char
*name, struct sync_file *a,
goto err;
}
+ mutex_unlock(&b->lock);
+ mutex_unlock(&a->lock);
+
strlcpy(sync_file->name, name, sizeof(sync_file->name));
return sync_file;
err:
fput(sync_file->file);
+unlock:
+ mutex_unlock(&b->lock);
+ mutex_unlock(&a->lock);
return NULL;
}
@@ -299,16 +309,20 @@ 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;
poll_wait(file, &sync_file->wq, wait);
+ mutex_lock(&sync_file->lock);
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);
}
+ ret_val = dma_fence_is_signaled(sync_file->fence) ? POLLIN : 0;
+ mutex_unlock(&sync_file->lock);
- return dma_fence_is_signaled(sync_file->fence) ? POLLIN : 0;
+ return ret_val;
}
static long sync_file_ioctl_merge(struct sync_file *sync_file,
@@ -393,6 +407,7 @@ static long sync_file_ioctl_fence_info(struct sync_file
*sync_file,
if (info.flags || info.pad)
return -EINVAL;
+ mutex_lock(&sync_file->lock);
fences = get_fences(sync_file, &num_fences);
/*
@@ -433,7 +448,7 @@ static long sync_file_ioctl_fence_info(struct sync_file
*sync_file,
out:
kfree(fence_info);
-
+ mutex_unlock(&sync_file->lock);
return ret;
}
diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
index 3e3ab84..5aef17f 100644
--- a/include/linux/sync_file.h
+++ b/include/linux/sync_file.h
@@ -30,6 +30,7 @@
* @wq:wait queue for fence signaling
* @fence: fence with the fences in the sync_file
* @cb:fence callback information
+ * @lock: mutex to protect fence/cb - used for semaphores
*/
struct sync_file {
struct file *file;
@@ -43,6 +44,8 @@ struct sync_file {
struct dma_fence*fence;
struct dma_fence_cb cb;
+ /* protects the fence pointer and cb */
+ struct mutex lock;
};
#define POLL_ENABLED DMA_FENCE_FLAG_USER_BITS
--
2.7.4
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
