* [PATCH 1/3] dma-buf/sw_sync: move timeline_fence_ops around
@ 2017-07-29 15:22 Gustavo Padovan
2017-07-29 15:22 ` [PATCH 2/3] dma-buf/sw_sync: clean up list before signaling the fence Gustavo Padovan
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Gustavo Padovan @ 2017-07-29 15:22 UTC (permalink / raw)
To: dri-devel; +Cc: Gustavo Padovan
From: Gustavo Padovan <gustavo.padovan@collabora.com>
We are going to use timeline_fence_signaled() in a internal function in
the next commit.
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
---
drivers/dma-buf/sw_sync.c | 138 +++++++++++++++++++++++-----------------------
1 file changed, 69 insertions(+), 69 deletions(-)
diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index af1bc84..ef0cc08 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -125,6 +125,75 @@ static void sync_timeline_put(struct sync_timeline *obj)
kref_put(&obj->kref, sync_timeline_free);
}
+static const char *timeline_fence_get_driver_name(struct dma_fence *fence)
+{
+ return "sw_sync";
+}
+
+static const char *timeline_fence_get_timeline_name(struct dma_fence *fence)
+{
+ struct sync_timeline *parent = dma_fence_parent(fence);
+
+ return parent->name;
+}
+
+static void timeline_fence_release(struct dma_fence *fence)
+{
+ struct sync_pt *pt = dma_fence_to_sync_pt(fence);
+ struct sync_timeline *parent = dma_fence_parent(fence);
+
+ if (!list_empty(&pt->link)) {
+ unsigned long flags;
+
+ spin_lock_irqsave(fence->lock, flags);
+ if (!list_empty(&pt->link)) {
+ list_del(&pt->link);
+ rb_erase(&pt->node, &parent->pt_tree);
+ }
+ spin_unlock_irqrestore(fence->lock, flags);
+ }
+
+ sync_timeline_put(parent);
+ dma_fence_free(fence);
+}
+
+static bool timeline_fence_signaled(struct dma_fence *fence)
+{
+ struct sync_timeline *parent = dma_fence_parent(fence);
+
+ return !__dma_fence_is_later(fence->seqno, parent->value);
+}
+
+static bool timeline_fence_enable_signaling(struct dma_fence *fence)
+{
+ return true;
+}
+
+static void timeline_fence_value_str(struct dma_fence *fence,
+ char *str, int size)
+{
+ snprintf(str, size, "%d", fence->seqno);
+}
+
+static void timeline_fence_timeline_value_str(struct dma_fence *fence,
+ char *str, int size)
+{
+ struct sync_timeline *parent = dma_fence_parent(fence);
+
+ snprintf(str, size, "%d", parent->value);
+}
+
+static const struct dma_fence_ops timeline_fence_ops = {
+ .get_driver_name = timeline_fence_get_driver_name,
+ .get_timeline_name = timeline_fence_get_timeline_name,
+ .enable_signaling = timeline_fence_enable_signaling,
+ .signaled = timeline_fence_signaled,
+ .wait = dma_fence_default_wait,
+ .release = timeline_fence_release,
+ .fence_value_str = timeline_fence_value_str,
+ .timeline_value_str = timeline_fence_timeline_value_str,
+};
+
/**
* sync_timeline_signal() - signal a status change on a sync_timeline
* @obj: sync_timeline to signal
@@ -216,75 +285,6 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj,
return pt;
}
-static const char *timeline_fence_get_driver_name(struct dma_fence *fence)
-{
- return "sw_sync";
-}
-
-static const char *timeline_fence_get_timeline_name(struct dma_fence *fence)
-{
- struct sync_timeline *parent = dma_fence_parent(fence);
-
- return parent->name;
-}
-
-static void timeline_fence_release(struct dma_fence *fence)
-{
- struct sync_pt *pt = dma_fence_to_sync_pt(fence);
- struct sync_timeline *parent = dma_fence_parent(fence);
-
- if (!list_empty(&pt->link)) {
- unsigned long flags;
-
- spin_lock_irqsave(fence->lock, flags);
- if (!list_empty(&pt->link)) {
- list_del(&pt->link);
- rb_erase(&pt->node, &parent->pt_tree);
- }
- spin_unlock_irqrestore(fence->lock, flags);
- }
-
- sync_timeline_put(parent);
- dma_fence_free(fence);
-}
-
-static bool timeline_fence_signaled(struct dma_fence *fence)
-{
- struct sync_timeline *parent = dma_fence_parent(fence);
-
- return !__dma_fence_is_later(fence->seqno, parent->value);
-}
-
-static bool timeline_fence_enable_signaling(struct dma_fence *fence)
-{
- return true;
-}
-
-static void timeline_fence_value_str(struct dma_fence *fence,
- char *str, int size)
-{
- snprintf(str, size, "%d", fence->seqno);
-}
-
-static void timeline_fence_timeline_value_str(struct dma_fence *fence,
- char *str, int size)
-{
- struct sync_timeline *parent = dma_fence_parent(fence);
-
- snprintf(str, size, "%d", parent->value);
-}
-
-static const struct dma_fence_ops timeline_fence_ops = {
- .get_driver_name = timeline_fence_get_driver_name,
- .get_timeline_name = timeline_fence_get_timeline_name,
- .enable_signaling = timeline_fence_enable_signaling,
- .signaled = timeline_fence_signaled,
- .wait = dma_fence_default_wait,
- .release = timeline_fence_release,
- .fence_value_str = timeline_fence_value_str,
- .timeline_value_str = timeline_fence_timeline_value_str,
-};
-
/*
* *WARNING*
*
--
2.9.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] dma-buf/sw_sync: clean up list before signaling the fence
2017-07-29 15:22 [PATCH 1/3] dma-buf/sw_sync: move timeline_fence_ops around Gustavo Padovan
@ 2017-07-29 15:22 ` Gustavo Padovan
2017-07-30 9:10 ` Chris Wilson
2017-07-29 15:22 ` [PATCH 3/3] selftests: sync: add test that closes the fd before fence signal Gustavo Padovan
2017-07-30 9:09 ` [PATCH 1/3] dma-buf/sw_sync: move timeline_fence_ops around Chris Wilson
2 siblings, 1 reply; 11+ messages in thread
From: Gustavo Padovan @ 2017-07-29 15:22 UTC (permalink / raw)
To: dri-devel; +Cc: Gustavo Padovan
From: Gustavo Padovan <gustavo.padovan@collabora.com>
If userspace already dropped its own reference by closing the sw_sync
fence fd we might end up in a deadlock where
dma_fence_is_signaled_locked() will trigger the release of the fence and
thus try to hold the lock to remove the fence from the list.
dma_fence_is_signaled_locked() tries to release/free the fence and hold
the lock in the process.
We fix that by changing the order operation and clean up the list and
rb-tree first.
v2: Drop the fence get/put dance and manipulate the list first (Chris Wilson)
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
---
drivers/dma-buf/sw_sync.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
index ef0cc08..38cc738 100644
--- a/drivers/dma-buf/sw_sync.c
+++ b/drivers/dma-buf/sw_sync.c
@@ -213,11 +213,21 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc)
obj->value += inc;
list_for_each_entry_safe(pt, next, &obj->pt_list, link) {
- if (!dma_fence_is_signaled_locked(&pt->base))
+ if (!timeline_fence_signaled(&pt->base))
break;
list_del_init(&pt->link);
rb_erase(&pt->node, &obj->pt_tree);
+
+ /*
+ * A signal callback may release the last reference to this
+ * fence, causing it to be freed. That operation has to be
+ * last to avoid a use after free inside this loop, and must
+ * be after we remove the fence from the timeline in order to
+ * prevent deadlocking on timeline->lock inside
+ * timeline_fence_release().
+ */
+ dma_fence_signal_locked(&pt->base);
}
spin_unlock_irq(&obj->lock);
--
2.9.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] selftests: sync: add test that closes the fd before fence signal
2017-07-29 15:22 [PATCH 1/3] dma-buf/sw_sync: move timeline_fence_ops around Gustavo Padovan
2017-07-29 15:22 ` [PATCH 2/3] dma-buf/sw_sync: clean up list before signaling the fence Gustavo Padovan
@ 2017-07-29 15:22 ` Gustavo Padovan
2017-07-30 9:20 ` Chris Wilson
2017-07-30 9:09 ` [PATCH 1/3] dma-buf/sw_sync: move timeline_fence_ops around Chris Wilson
2 siblings, 1 reply; 11+ messages in thread
From: Gustavo Padovan @ 2017-07-29 15:22 UTC (permalink / raw)
To: dri-devel; +Cc: Gustavo Padovan, Shuah Khan, linux-kselftest
From: Gustavo Padovan <gustavo.padovan@collabora.com>
We found this bug in the sw_sync so adding a test case to prevent it to
happen in the future.
Cc: Shuah Khan <shuahkh@osg.samsung.com>
Cc: linux-kselftest@vger.kernel.org
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
---
To be applied after the TAP13 convertion patches.
---
tools/testing/selftests/sync/sync_fence.c | 23 +++++++++++++++++++++++
tools/testing/selftests/sync/sync_test.c | 1 +
tools/testing/selftests/sync/synctest.h | 1 +
3 files changed, 25 insertions(+)
diff --git a/tools/testing/selftests/sync/sync_fence.c b/tools/testing/selftests/sync/sync_fence.c
index 13f1752..70cfa61 100644
--- a/tools/testing/selftests/sync/sync_fence.c
+++ b/tools/testing/selftests/sync/sync_fence.c
@@ -29,6 +29,29 @@
#include "sw_sync.h"
#include "synctest.h"
+int test_close_fence_fd_before_inc(void)
+{
+ int fence, valid, ret;
+ int timeline = sw_sync_timeline_create();
+
+ valid = sw_sync_timeline_is_valid(timeline);
+ ASSERT(valid, "Failure allocating timeline\n");
+
+ fence = sw_sync_fence_create(timeline, "allocFence", 1);
+ valid = sw_sync_fence_is_valid(fence);
+ ASSERT(valid, "Failure allocating fence\n");
+
+ sw_sync_fence_destroy(fence);
+
+ /* Advance timeline from 0 -> 1 */
+ ret = sw_sync_timeline_inc(timeline, 1);
+ ASSERT(ret == 0, "Failure advancing timeline\n");
+
+ sw_sync_timeline_destroy(timeline);
+
+ return 0;
+}
+
int test_fence_one_timeline_wait(void)
{
int fence, valid, ret;
diff --git a/tools/testing/selftests/sync/sync_test.c b/tools/testing/selftests/sync/sync_test.c
index 7f79382..2f73ace 100644
--- a/tools/testing/selftests/sync/sync_test.c
+++ b/tools/testing/selftests/sync/sync_test.c
@@ -95,6 +95,7 @@ int main(void)
RUN_TEST(test_alloc_fence);
RUN_TEST(test_alloc_fence_negative);
+ RUN_TEST(test_close_fence_fd_before_inc);
RUN_TEST(test_fence_one_timeline_wait);
RUN_TEST(test_fence_one_timeline_merge);
RUN_TEST(test_fence_merge_same_fence);
diff --git a/tools/testing/selftests/sync/synctest.h b/tools/testing/selftests/sync/synctest.h
index 90a8e53..86a9532 100644
--- a/tools/testing/selftests/sync/synctest.h
+++ b/tools/testing/selftests/sync/synctest.h
@@ -46,6 +46,7 @@ int test_alloc_fence(void);
int test_alloc_fence_negative(void);
/* Fence tests with one timeline */
+int test_close_fence_fd_before_inc(void);
int test_fence_one_timeline_wait(void);
int test_fence_one_timeline_merge(void);
--
2.9.4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] dma-buf/sw_sync: move timeline_fence_ops around
2017-07-29 15:22 [PATCH 1/3] dma-buf/sw_sync: move timeline_fence_ops around Gustavo Padovan
2017-07-29 15:22 ` [PATCH 2/3] dma-buf/sw_sync: clean up list before signaling the fence Gustavo Padovan
2017-07-29 15:22 ` [PATCH 3/3] selftests: sync: add test that closes the fd before fence signal Gustavo Padovan
@ 2017-07-30 9:09 ` Chris Wilson
2017-07-31 19:32 ` Gustavo Padovan
2 siblings, 1 reply; 11+ messages in thread
From: Chris Wilson @ 2017-07-30 9:09 UTC (permalink / raw)
To: Gustavo Padovan, dri-devel; +Cc: Gustavo Padovan
Quoting Gustavo Padovan (2017-07-29 16:22:15)
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
>
> We are going to use timeline_fence_signaled() in a internal function in
> the next commit.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
Purely mechanical,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] dma-buf/sw_sync: clean up list before signaling the fence
2017-07-29 15:22 ` [PATCH 2/3] dma-buf/sw_sync: clean up list before signaling the fence Gustavo Padovan
@ 2017-07-30 9:10 ` Chris Wilson
2017-07-31 19:32 ` Gustavo Padovan
0 siblings, 1 reply; 11+ messages in thread
From: Chris Wilson @ 2017-07-30 9:10 UTC (permalink / raw)
To: Gustavo Padovan, dri-devel; +Cc: Gustavo Padovan
Quoting Gustavo Padovan (2017-07-29 16:22:16)
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
>
> If userspace already dropped its own reference by closing the sw_sync
> fence fd we might end up in a deadlock where
> dma_fence_is_signaled_locked() will trigger the release of the fence and
> thus try to hold the lock to remove the fence from the list.
>
> dma_fence_is_signaled_locked() tries to release/free the fence and hold
> the lock in the process.
>
> We fix that by changing the order operation and clean up the list and
> rb-tree first.
>
> v2: Drop the fence get/put dance and manipulate the list first (Chris Wilson)
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] selftests: sync: add test that closes the fd before fence signal
2017-07-29 15:22 ` [PATCH 3/3] selftests: sync: add test that closes the fd before fence signal Gustavo Padovan
@ 2017-07-30 9:20 ` Chris Wilson
2017-07-31 19:43 ` Gustavo Padovan
0 siblings, 1 reply; 11+ messages in thread
From: Chris Wilson @ 2017-07-30 9:20 UTC (permalink / raw)
To: Gustavo Padovan, dri-devel; +Cc: Gustavo Padovan, Shuah Khan, linux-kselftest
Quoting Gustavo Padovan (2017-07-29 16:22:17)
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
>
> We found this bug in the sw_sync so adding a test case to prevent it to
> happen in the future.
>
> Cc: Shuah Khan <shuahkh@osg.samsung.com>
> Cc: linux-kselftest@vger.kernel.org
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
>
> ---
> To be applied after the TAP13 convertion patches.
> ---
> tools/testing/selftests/sync/sync_fence.c | 23 +++++++++++++++++++++++
> tools/testing/selftests/sync/sync_test.c | 1 +
> tools/testing/selftests/sync/synctest.h | 1 +
> 3 files changed, 25 insertions(+)
>
> diff --git a/tools/testing/selftests/sync/sync_fence.c b/tools/testing/selftests/sync/sync_fence.c
> index 13f1752..70cfa61 100644
> --- a/tools/testing/selftests/sync/sync_fence.c
> +++ b/tools/testing/selftests/sync/sync_fence.c
> @@ -29,6 +29,29 @@
> #include "sw_sync.h"
> #include "synctest.h"
>
> +int test_close_fence_fd_before_inc(void)
> +{
> + int fence, valid, ret;
> + int timeline = sw_sync_timeline_create();
> +
> + valid = sw_sync_timeline_is_valid(timeline);
> + ASSERT(valid, "Failure allocating timeline\n");
> +
> + fence = sw_sync_fence_create(timeline, "allocFence", 1);
> + valid = sw_sync_fence_is_valid(fence);
> + ASSERT(valid, "Failure allocating fence\n");
> +
/*
* We want the destroy + inc to run within the same RCU grace period so
* that the zombie fence still exists on the timeline.
*/
> + sw_sync_fence_destroy(fence);
I think this doesn't exercise the bug you found as we should be entering
the timeline_inc loop with fence.refcount==0 rather than the refcount
going to zero within the loop.
To achieve that we need to find a callback that does unreference a
dma-fence and chain those together so that it frees a sw_sync from the
same timeline.
Still add the comment about what you are intending to hit here and
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] dma-buf/sw_sync: move timeline_fence_ops around
2017-07-30 9:09 ` [PATCH 1/3] dma-buf/sw_sync: move timeline_fence_ops around Chris Wilson
@ 2017-07-31 19:32 ` Gustavo Padovan
0 siblings, 0 replies; 11+ messages in thread
From: Gustavo Padovan @ 2017-07-31 19:32 UTC (permalink / raw)
To: Chris Wilson; +Cc: Gustavo Padovan, dri-devel
2017-07-30 Chris Wilson <chris@chris-wilson.co.uk>:
> Quoting Gustavo Padovan (2017-07-29 16:22:15)
> > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> >
> > We are going to use timeline_fence_signaled() in a internal function in
> > the next commit.
> >
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
>
> Purely mechanical,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Thanks for reviewing. Pushed to drm-misc-next.
Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] dma-buf/sw_sync: clean up list before signaling the fence
2017-07-30 9:10 ` Chris Wilson
@ 2017-07-31 19:32 ` Gustavo Padovan
0 siblings, 0 replies; 11+ messages in thread
From: Gustavo Padovan @ 2017-07-31 19:32 UTC (permalink / raw)
To: Chris Wilson; +Cc: Gustavo Padovan, dri-devel
2017-07-30 Chris Wilson <chris@chris-wilson.co.uk>:
> Quoting Gustavo Padovan (2017-07-29 16:22:16)
> > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> >
> > If userspace already dropped its own reference by closing the sw_sync
> > fence fd we might end up in a deadlock where
> > dma_fence_is_signaled_locked() will trigger the release of the fence and
> > thus try to hold the lock to remove the fence from the list.
> >
> > dma_fence_is_signaled_locked() tries to release/free the fence and hold
> > the lock in the process.
> >
> > We fix that by changing the order operation and clean up the list and
> > rb-tree first.
> >
> > v2: Drop the fence get/put dance and manipulate the list first (Chris Wilson)
> >
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Thanks for reviewing. Pushed to drm-misc-next.
Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] selftests: sync: add test that closes the fd before fence signal
2017-07-30 9:20 ` Chris Wilson
@ 2017-07-31 19:43 ` Gustavo Padovan
2017-08-02 19:45 ` Shuah Khan
0 siblings, 1 reply; 11+ messages in thread
From: Gustavo Padovan @ 2017-07-31 19:43 UTC (permalink / raw)
To: Chris Wilson; +Cc: Gustavo Padovan, linux-kselftest, Shuah Khan, dri-devel
2017-07-30 Chris Wilson <chris@chris-wilson.co.uk>:
> Quoting Gustavo Padovan (2017-07-29 16:22:17)
> > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> >
> > We found this bug in the sw_sync so adding a test case to prevent it to
> > happen in the future.
> >
> > Cc: Shuah Khan <shuahkh@osg.samsung.com>
> > Cc: linux-kselftest@vger.kernel.org
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> >
> > ---
> > To be applied after the TAP13 convertion patches.
> > ---
> > tools/testing/selftests/sync/sync_fence.c | 23 +++++++++++++++++++++++
> > tools/testing/selftests/sync/sync_test.c | 1 +
> > tools/testing/selftests/sync/synctest.h | 1 +
> > 3 files changed, 25 insertions(+)
> >
> > diff --git a/tools/testing/selftests/sync/sync_fence.c b/tools/testing/selftests/sync/sync_fence.c
> > index 13f1752..70cfa61 100644
> > --- a/tools/testing/selftests/sync/sync_fence.c
> > +++ b/tools/testing/selftests/sync/sync_fence.c
> > @@ -29,6 +29,29 @@
> > #include "sw_sync.h"
> > #include "synctest.h"
> >
> > +int test_close_fence_fd_before_inc(void)
> > +{
> > + int fence, valid, ret;
> > + int timeline = sw_sync_timeline_create();
> > +
> > + valid = sw_sync_timeline_is_valid(timeline);
> > + ASSERT(valid, "Failure allocating timeline\n");
> > +
> > + fence = sw_sync_fence_create(timeline, "allocFence", 1);
> > + valid = sw_sync_fence_is_valid(fence);
> > + ASSERT(valid, "Failure allocating fence\n");
> > +
>
> /*
> * We want the destroy + inc to run within the same RCU grace period so
> * that the zombie fence still exists on the timeline.
> */
>
> > + sw_sync_fence_destroy(fence);
>
> I think this doesn't exercise the bug you found as we should be entering
> the timeline_inc loop with fence.refcount==0 rather than the refcount
> going to zero within the loop.
>
> To achieve that we need to find a callback that does unreference a
> dma-fence and chain those together so that it frees a sw_sync from the
> same timeline.
Indeed. Without the internal callback this test is a bit useless. We
could test this under drm atomic tests on IGT. Particulary, I hit it
playing with tests for v4l2 fences.
Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] selftests: sync: add test that closes the fd before fence signal
2017-07-31 19:43 ` Gustavo Padovan
@ 2017-08-02 19:45 ` Shuah Khan
2017-08-02 20:28 ` Gustavo Padovan
0 siblings, 1 reply; 11+ messages in thread
From: Shuah Khan @ 2017-08-02 19:45 UTC (permalink / raw)
To: Gustavo Padovan, Chris Wilson
Cc: Gustavo Padovan, Shuah Khan, linux-kselftest, dri-devel, Shuah Khan
On 07/31/2017 01:43 PM, Gustavo Padovan wrote:
> 2017-07-30 Chris Wilson <chris@chris-wilson.co.uk>:
>
>> Quoting Gustavo Padovan (2017-07-29 16:22:17)
>>> From: Gustavo Padovan <gustavo.padovan@collabora.com>
>>>
>>> We found this bug in the sw_sync so adding a test case to prevent it to
>>> happen in the future.
>>>
>>> Cc: Shuah Khan <shuahkh@osg.samsung.com>
>>> Cc: linux-kselftest@vger.kernel.org
>>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
>>>
>>> ---
>>> To be applied after the TAP13 convertion patches.
>>> ---
>>> tools/testing/selftests/sync/sync_fence.c | 23 +++++++++++++++++++++++
>>> tools/testing/selftests/sync/sync_test.c | 1 +
>>> tools/testing/selftests/sync/synctest.h | 1 +
>>> 3 files changed, 25 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/sync/sync_fence.c b/tools/testing/selftests/sync/sync_fence.c
>>> index 13f1752..70cfa61 100644
>>> --- a/tools/testing/selftests/sync/sync_fence.c
>>> +++ b/tools/testing/selftests/sync/sync_fence.c
>>> @@ -29,6 +29,29 @@
>>> #include "sw_sync.h"
>>> #include "synctest.h"
>>>
>>> +int test_close_fence_fd_before_inc(void)
>>> +{
>>> + int fence, valid, ret;
>>> + int timeline = sw_sync_timeline_create();
>>> +
>>> + valid = sw_sync_timeline_is_valid(timeline);
>>> + ASSERT(valid, "Failure allocating timeline\n");
>>> +
>>> + fence = sw_sync_fence_create(timeline, "allocFence", 1);
>>> + valid = sw_sync_fence_is_valid(fence);
>>> + ASSERT(valid, "Failure allocating fence\n");
>>> +
>>
>> /*
>> * We want the destroy + inc to run within the same RCU grace period so
>> * that the zombie fence still exists on the timeline.
>> */
>>
>>> + sw_sync_fence_destroy(fence);
>>
>> I think this doesn't exercise the bug you found as we should be entering
>> the timeline_inc loop with fence.refcount==0 rather than the refcount
>> going to zero within the loop.
>>
>> To achieve that we need to find a callback that does unreference a
>> dma-fence and chain those together so that it frees a sw_sync from the
>> same timeline.
>
> Indeed. Without the internal callback this test is a bit useless. We
> could test this under drm atomic tests on IGT. Particulary, I hit it
> playing with tests for v4l2 fences.
>
Hi Gustavo,
Would you like this pulled into 4.14-rc1? Sounds like this test is for a
feature that doesn't exist yet?
thanks,
-- Shuah
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] selftests: sync: add test that closes the fd before fence signal
2017-08-02 19:45 ` Shuah Khan
@ 2017-08-02 20:28 ` Gustavo Padovan
0 siblings, 0 replies; 11+ messages in thread
From: Gustavo Padovan @ 2017-08-02 20:28 UTC (permalink / raw)
To: Shuah Khan, Gustavo Padovan, Chris Wilson
Cc: Shuah Khan, linux-kselftest, dri-devel
Hi Shuah,
On Wed, 2017-08-02 at 13:45 -0600, Shuah Khan wrote:
> On 07/31/2017 01:43 PM, Gustavo Padovan wrote:
> > 2017-07-30 Chris Wilson <chris@chris-wilson.co.uk>:
> >
> > > Quoting Gustavo Padovan (2017-07-29 16:22:17)
> > > > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> > > >
> > > > We found this bug in the sw_sync so adding a test case to
> > > > prevent it to
> > > > happen in the future.
> > > >
> > > > Cc: Shuah Khan <shuahkh@osg.samsung.com>
> > > > Cc: linux-kselftest@vger.kernel.org
> > > > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> > > >
> > > > ---
> > > > To be applied after the TAP13 convertion patches.
> > > > ---
> > > > tools/testing/selftests/sync/sync_fence.c | 23
> > > > +++++++++++++++++++++++
> > > > tools/testing/selftests/sync/sync_test.c | 1 +
> > > > tools/testing/selftests/sync/synctest.h | 1 +
> > > > 3 files changed, 25 insertions(+)
> > > >
> > > > diff --git a/tools/testing/selftests/sync/sync_fence.c
> > > > b/tools/testing/selftests/sync/sync_fence.c
> > > > index 13f1752..70cfa61 100644
> > > > --- a/tools/testing/selftests/sync/sync_fence.c
> > > > +++ b/tools/testing/selftests/sync/sync_fence.c
> > > > @@ -29,6 +29,29 @@
> > > > #include "sw_sync.h"
> > > > #include "synctest.h"
> > > >
> > > > +int test_close_fence_fd_before_inc(void)
> > > > +{
> > > > + int fence, valid, ret;
> > > > + int timeline = sw_sync_timeline_create();
> > > > +
> > > > + valid = sw_sync_timeline_is_valid(timeline);
> > > > + ASSERT(valid, "Failure allocating timeline\n");
> > > > +
> > > > + fence = sw_sync_fence_create(timeline, "allocFence",
> > > > 1);
> > > > + valid = sw_sync_fence_is_valid(fence);
> > > > + ASSERT(valid, "Failure allocating fence\n");
> > > > +
> > >
> > > /*
> > > * We want the destroy + inc to run within the same RCU grace
> > > period so
> > > * that the zombie fence still exists on the timeline.
> > > */
> > >
> > > > + sw_sync_fence_destroy(fence);
> > >
> > > I think this doesn't exercise the bug you found as we should be
> > > entering
> > > the timeline_inc loop with fence.refcount==0 rather than the
> > > refcount
> > > going to zero within the loop.
> > >
> > > To achieve that we need to find a callback that does unreference
> > > a
> > > dma-fence and chain those together so that it frees a sw_sync
> > > from the
> > > same timeline.
> >
> > Indeed. Without the internal callback this test is a bit useless.
> > We
> > could test this under drm atomic tests on IGT. Particulary, I hit
> > it
> > playing with tests for v4l2 fences.
> >
>
> Hi Gustavo,
>
> Would you like this pulled into 4.14-rc1? Sounds like this test is
> for a
> feature that doesn't exist yet?
It is for the current code, but I need to find a better way to trigger
what I want. I'll rethink this test and resend.
Gustavo
--
Gustavo Padovan
Collabora Ltd.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-08-02 20:28 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-29 15:22 [PATCH 1/3] dma-buf/sw_sync: move timeline_fence_ops around Gustavo Padovan
2017-07-29 15:22 ` [PATCH 2/3] dma-buf/sw_sync: clean up list before signaling the fence Gustavo Padovan
2017-07-30 9:10 ` Chris Wilson
2017-07-31 19:32 ` Gustavo Padovan
2017-07-29 15:22 ` [PATCH 3/3] selftests: sync: add test that closes the fd before fence signal Gustavo Padovan
2017-07-30 9:20 ` Chris Wilson
2017-07-31 19:43 ` Gustavo Padovan
2017-08-02 19:45 ` Shuah Khan
2017-08-02 20:28 ` Gustavo Padovan
2017-07-30 9:09 ` [PATCH 1/3] dma-buf/sw_sync: move timeline_fence_ops around Chris Wilson
2017-07-31 19:32 ` Gustavo Padovan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.