| Message ID | 20170729152217.8362-3-gustavo@padovan.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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);