Patchwork [3/3] selftests: sync: add test that closes the fd before fence signal

login
register
mail settings
Submitter Gustavo F. Padovan
Date July 29, 2017, 3:22 p.m.
Message ID <20170729152217.8362-3-gustavo@padovan.org>
Download mbox | patch
Permalink /patch/9870199/
State New
Headers show

Comments

Gustavo F. Padovan - July 29, 2017, 3:22 p.m.
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(+)
Chris Wilson - July 30, 2017, 9:20 a.m.
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
Gustavo F. Padovan - July 31, 2017, 7:43 p.m.
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
Shuah Khan - Aug. 2, 2017, 7:45 p.m.
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
Gustavo Padovan - Aug. 2, 2017, 8:28 p.m.
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

Patch

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