[igt-dev] [PATCH i-g-t] igt/sw_sync: Wrap threaded counter manipulation with mb

[igt-dev] [PATCH i-g-t] igt/sw_sync: Wrap threaded counter manipulation with mb

[Chris Wilson]

sw_sync/sync_multi_consumer_producer was communicating between threads
using the sw_sync ioctl and manipulating a shared volatile counter.
However, the ioctl itself does not imply a memory barrier, and so
different CPUs may see different states of the counter (the volatile
making GCC perform the operation in stages making the race even more
likely). Instead of using volatile, use locked operations to make the
counter manipulation thread-safe.

References: https://bugs.freedesktop.org/show_bug.cgi?id=106344
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/sw_sync.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/tests/sw_sync.c b/tests/sw_sync.c
index 20dfbbb98..c6867e097 100644
--- a/tests/sw_sync.c
+++ b/tests/sw_sync.c
@@ -43,7 +43,7 @@ IGT_TEST_DESCRIPTION("Test SW Sync Framework");
 typedef struct {
 	int timeline;
 	uint32_t thread_id;
-	volatile uint32_t * volatile counter;
+	uint32_t *counter;
 	sem_t *sem;
 } data_t;
 
@@ -508,7 +508,7 @@ static void * test_sync_multi_consumer_thread(void *arg)
 		if (sync_fence_wait(fence, 1000) < 0)
 			return (void *) 1;
 
-		if (*(data->counter) != next_point)
+		if (READ_ONCE(*data->counter) != next_point)
 			return (void *) 1;
 
 		sem_post(data->sem);
@@ -524,7 +524,7 @@ static void test_sync_multi_consumer(void)
 	pthread_t thread_arr[MULTI_CONSUMER_THREADS];
 	sem_t sem;
 	int timeline;
-	volatile uint32_t counter = 0;
+	uint32_t counter = 0;
 	uintptr_t thread_ret = 0;
 	data_t data;
 	int i, ret;
@@ -552,7 +552,7 @@ static void test_sync_multi_consumer(void)
 	{
 		sem_wait(&sem);
 
-		counter++;
+		__sync_fetch_and_add(&counter, 1);
 		sw_sync_timeline_inc(timeline, 1);
 	}
 
@@ -567,8 +567,8 @@ static void test_sync_multi_consumer(void)
 	close(timeline);
 	sem_destroy(&sem);
 
-	igt_assert_f(counter == MULTI_CONSUMER_THREADS * MULTI_CONSUMER_ITERATIONS,
-		     "Counter has unexpected value.\n");
+	igt_assert_eq(counter,
+		      MULTI_CONSUMER_THREADS * MULTI_CONSUMER_ITERATIONS);
 
 	igt_assert_f(thread_ret == 0, "A sync thread reported failure.\n");
 }
@@ -589,11 +589,9 @@ static void * test_sync_multi_consumer_producer_thread(void *arg)
 		if (sync_fence_wait(fence, 1000) < 0)
 			return (void *) 1;
 
-		if (*(data->counter) != next_point)
+		if (__sync_fetch_and_add(data->counter, 1) != next_point)
 			return (void *) 1;
 
-		(*data->counter)++;
-
 		/* Kick off the next thread. */
 		sw_sync_timeline_inc(timeline, 1);
 
@@ -607,7 +605,7 @@ static void test_sync_multi_consumer_producer(void)
 	data_t data_arr[MULTI_CONSUMER_PRODUCER_THREADS];
 	pthread_t thread_arr[MULTI_CONSUMER_PRODUCER_THREADS];
 	int timeline;
-	volatile uint32_t counter = 0;
+	uint32_t counter = 0;
 	uintptr_t thread_ret = 0;
 	data_t data;
 	int i, ret;
@@ -638,9 +636,9 @@ static void test_sync_multi_consumer_producer(void)
 
 	close(timeline);
 
-	igt_assert_f(counter == MULTI_CONSUMER_PRODUCER_THREADS *
-	                        MULTI_CONSUMER_PRODUCER_ITERATIONS,
-		     "Counter has unexpected value.\n");
+	igt_assert_eq(counter,
+		      MULTI_CONSUMER_PRODUCER_THREADS *
+		      MULTI_CONSUMER_PRODUCER_ITERATIONS);
 
 	igt_assert_f(thread_ret == 0, "A sync thread reported failure.\n");
 }
-- 
2.17.0

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.BAT: success for igt/sw_sync: Wrap threaded counter manipulation with mb

[Patchwork]

== Series Details ==

Series: igt/sw_sync: Wrap threaded counter manipulation with mb
URL   : https://patchwork.freedesktop.org/series/42559/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4121 -> IGTPW_1312 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42559/revisions/1/mbox/

== Known issues ==

  Here are the changes found in IGTPW_1312 that come from known issues:

  === IGT changes ===

    ==== Possible fixes ====

    igt@gem_mmap_gtt@basic-small-bo-tiledx:
      fi-gdg-551:         FAIL (fdo#102575) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-snb-2520m:       INCOMPLETE (fdo#103713) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-ivb-3520m:       DMESG-WARN (fdo#106084) -> PASS

    
  fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#106084 https://bugs.freedesktop.org/show_bug.cgi?id=106084


== Participating hosts (40 -> 36) ==

  Missing    (4): fi-ctg-p8600 fi-ilk-m540 fi-bxt-dsi fi-skl-6700hq 


== Build changes ==

    * IGT: IGT_4456 -> IGTPW_1312

  CI_DRM_4121: d4f7520d80ab83ea9053ee080df09a23463b6966 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1312: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1312/
  IGT_4456: 43761534c6482dc67b9c3d8eeecd425ef40b3c4c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  piglit_4456: 30b992bdc047073e1fe99b1ac622f026618a8081 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1312/issues.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.IGT: success for igt/sw_sync: Wrap threaded counter manipulation with mb

[Patchwork]

== Series Details ==

Series: igt/sw_sync: Wrap threaded counter manipulation with mb
URL   : https://patchwork.freedesktop.org/series/42559/
State : success

== Summary ==

= CI Bug Log - changes from IGT_4456_full -> IGTPW_1312_full =

== Summary - WARNING ==

  Minor unknown changes coming with IGTPW_1312_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_1312_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/42559/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in IGTPW_1312_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_schedule@deep-blt:
      shard-kbl:          SKIP -> PASS +1

    igt@gem_exec_schedule@deep-bsd1:
      shard-kbl:          PASS -> SKIP

    igt@pm_rc6_residency@rc6-accuracy:
      shard-snb:          PASS -> SKIP

    
== Known issues ==

  Here are the changes found in IGTPW_1312_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_store@cachelines-bsd:
      shard-hsw:          PASS -> FAIL (fdo#100007)

    igt@kms_flip@plain-flip-ts-check-interruptible:
      shard-hsw:          PASS -> FAIL (fdo#100368)

    igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
      shard-snb:          PASS -> FAIL (fdo#103166)

    igt@kms_setmode@basic:
      shard-kbl:          PASS -> FAIL (fdo#99912)

    
    ==== Possible fixes ====

    igt@drv_selftest@mock_breadcrumbs:
      shard-hsw:          DMESG-FAIL (fdo#106085) -> PASS
      shard-snb:          DMESG-FAIL (fdo#106085) -> PASS
      shard-glk:          DMESG-FAIL (fdo#106085) -> PASS
      shard-apl:          DMESG-FAIL (fdo#106085) -> PASS
      shard-kbl:          DMESG-FAIL (fdo#106085) -> PASS

    igt@gem_exec_suspend@basic-s3:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    igt@kms_cursor_crc@cursor-128x128-suspend:
      shard-glk:          INCOMPLETE (k.org#198133, fdo#103359) -> PASS

    igt@kms_cursor_legacy@flip-vs-cursor-atomic:
      shard-hsw:          FAIL (fdo#102670) -> PASS

    igt@kms_flip@2x-plain-flip-ts-check:
      shard-hsw:          FAIL (fdo#100368) -> PASS

    igt@kms_flip@wf_vblank-ts-check:
      shard-hsw:          FAIL (fdo#103928) -> PASS

    igt@kms_flip@wf_vblank-ts-check-interruptible:
      shard-apl:          FAIL (fdo#100368) -> PASS

    igt@kms_rotation_crc@primary-rotation-180:
      shard-snb:          FAIL (fdo#103925) -> PASS

    igt@kms_setmode@basic:
      shard-hsw:          FAIL (fdo#99912) -> PASS

    
    ==== Warnings ====

    igt@kms_flip@2x-flip-vs-expired-vblank:
      shard-hsw:          FAIL (fdo#105707) -> DMESG-WARN (fdo#102614)

    
  fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#102670 https://bugs.freedesktop.org/show_bug.cgi?id=102670
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
  fdo#103928 https://bugs.freedesktop.org/show_bug.cgi?id=103928
  fdo#105707 https://bugs.freedesktop.org/show_bug.cgi?id=105707
  fdo#106085 https://bugs.freedesktop.org/show_bug.cgi?id=106085
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (7 -> 5) ==

  Missing    (2): pig-skl-6600 pig-glk-j4005 


== Build changes ==

    * IGT: IGT_4456 -> IGTPW_1312
    * Linux: CI_DRM_4117 -> CI_DRM_4121

  CI_DRM_4117: 844dd95837ab995c37d1139d74ff55139987b437 @ git://anongit.freedesktop.org/gfx-ci/linux
  CI_DRM_4121: d4f7520d80ab83ea9053ee080df09a23463b6966 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_1312: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1312/
  IGT_4456: 43761534c6482dc67b9c3d8eeecd425ef40b3c4c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  piglit_4456: 30b992bdc047073e1fe99b1ac622f026618a8081 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_1312/shards.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] igt/sw_sync: Wrap threaded counter manipulation with mb

[Joonas Lahtinen]

Quoting Chris Wilson (2018-05-02 12:28:07)
> sw_sync/sync_multi_consumer_producer was communicating between threads
> using the sw_sync ioctl and manipulating a shared volatile counter.
> However, the ioctl itself does not imply a memory barrier, and so
> different CPUs may see different states of the counter (the volatile
> making GCC perform the operation in stages making the race even more
> likely). Instead of using volatile, use locked operations to make the
> counter manipulation thread-safe.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=106344
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  tests/sw_sync.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/tests/sw_sync.c b/tests/sw_sync.c
> index 20dfbbb98..c6867e097 100644
> --- a/tests/sw_sync.c
> +++ b/tests/sw_sync.c
> @@ -43,7 +43,7 @@ IGT_TEST_DESCRIPTION("Test SW Sync Framework");
>  typedef struct {
>         int timeline;
>         uint32_t thread_id;
> -       volatile uint32_t * volatile counter;
> +       uint32_t *counter;
>         sem_t *sem;
>  } data_t;
>  
> @@ -508,7 +508,7 @@ static void * test_sync_multi_consumer_thread(void *arg)
>                 if (sync_fence_wait(fence, 1000) < 0)
>                         return (void *) 1;
>  
> -               if (*(data->counter) != next_point)
> +               if (READ_ONCE(*data->counter) != next_point)
>                         return (void *) 1;
>  
>                 sem_post(data->sem);
> @@ -524,7 +524,7 @@ static void test_sync_multi_consumer(void)
>         pthread_t thread_arr[MULTI_CONSUMER_THREADS];
>         sem_t sem;
>         int timeline;
> -       volatile uint32_t counter = 0;
> +       uint32_t counter = 0;
>         uintptr_t thread_ret = 0;
>         data_t data;
>         int i, ret;
> @@ -552,7 +552,7 @@ static void test_sync_multi_consumer(void)
>         {
>                 sem_wait(&sem);
>  
> -               counter++;
> +               __sync_fetch_and_add(&counter, 1);
>                 sw_sync_timeline_inc(timeline, 1);
>         }
>  
> @@ -567,8 +567,8 @@ static void test_sync_multi_consumer(void)
>         close(timeline);
>         sem_destroy(&sem);
>  
> -       igt_assert_f(counter == MULTI_CONSUMER_THREADS * MULTI_CONSUMER_ITERATIONS,
> -                    "Counter has unexpected value.\n");
> +       igt_assert_eq(counter,
> +                     MULTI_CONSUMER_THREADS * MULTI_CONSUMER_ITERATIONS);
>  
>         igt_assert_f(thread_ret == 0, "A sync thread reported failure.\n");
>  }
> @@ -589,11 +589,9 @@ static void * test_sync_multi_consumer_producer_thread(void *arg)
>                 if (sync_fence_wait(fence, 1000) < 0)
>                         return (void *) 1;
>  
> -               if (*(data->counter) != next_point)
> +               if (__sync_fetch_and_add(data->counter, 1) != next_point)
>                         return (void *) 1;
>  
> -               (*data->counter)++;

This is a behavior change, the counter gets incremented always :(

Regards, Joonas

> -
>                 /* Kick off the next thread. */
>                 sw_sync_timeline_inc(timeline, 1);
>  
> @@ -607,7 +605,7 @@ static void test_sync_multi_consumer_producer(void)
>         data_t data_arr[MULTI_CONSUMER_PRODUCER_THREADS];
>         pthread_t thread_arr[MULTI_CONSUMER_PRODUCER_THREADS];
>         int timeline;
> -       volatile uint32_t counter = 0;
> +       uint32_t counter = 0;
>         uintptr_t thread_ret = 0;
>         data_t data;
>         int i, ret;
> @@ -638,9 +636,9 @@ static void test_sync_multi_consumer_producer(void)
>  
>         close(timeline);
>  
> -       igt_assert_f(counter == MULTI_CONSUMER_PRODUCER_THREADS *
> -                               MULTI_CONSUMER_PRODUCER_ITERATIONS,
> -                    "Counter has unexpected value.\n");
> +       igt_assert_eq(counter,
> +                     MULTI_CONSUMER_PRODUCER_THREADS *
> +                     MULTI_CONSUMER_PRODUCER_ITERATIONS);
>  
>         igt_assert_f(thread_ret == 0, "A sync thread reported failure.\n");
>  }
> -- 
> 2.17.0
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] igt/sw_sync: Wrap threaded counter manipulation with mb

[Chris Wilson]

Quoting Joonas Lahtinen (2018-05-03 17:55:57)
> Quoting Chris Wilson (2018-05-02 12:28:07)
> > sw_sync/sync_multi_consumer_producer was communicating between threads
> > using the sw_sync ioctl and manipulating a shared volatile counter.
> > However, the ioctl itself does not imply a memory barrier, and so
> > different CPUs may see different states of the counter (the volatile
> > making GCC perform the operation in stages making the race even more
> > likely). Instead of using volatile, use locked operations to make the
> > counter manipulation thread-safe.
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=106344
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  tests/sw_sync.c | 24 +++++++++++-------------
> >  1 file changed, 11 insertions(+), 13 deletions(-)
> > 
> > diff --git a/tests/sw_sync.c b/tests/sw_sync.c
> > index 20dfbbb98..c6867e097 100644
> > --- a/tests/sw_sync.c
> > +++ b/tests/sw_sync.c
> > @@ -43,7 +43,7 @@ IGT_TEST_DESCRIPTION("Test SW Sync Framework");
> >  typedef struct {
> >         int timeline;
> >         uint32_t thread_id;
> > -       volatile uint32_t * volatile counter;
> > +       uint32_t *counter;
> >         sem_t *sem;
> >  } data_t;
> >  
> > @@ -508,7 +508,7 @@ static void * test_sync_multi_consumer_thread(void *arg)
> >                 if (sync_fence_wait(fence, 1000) < 0)
> >                         return (void *) 1;
> >  
> > -               if (*(data->counter) != next_point)
> > +               if (READ_ONCE(*data->counter) != next_point)
> >                         return (void *) 1;
> >  
> >                 sem_post(data->sem);
> > @@ -524,7 +524,7 @@ static void test_sync_multi_consumer(void)
> >         pthread_t thread_arr[MULTI_CONSUMER_THREADS];
> >         sem_t sem;
> >         int timeline;
> > -       volatile uint32_t counter = 0;
> > +       uint32_t counter = 0;
> >         uintptr_t thread_ret = 0;
> >         data_t data;
> >         int i, ret;
> > @@ -552,7 +552,7 @@ static void test_sync_multi_consumer(void)
> >         {
> >                 sem_wait(&sem);
> >  
> > -               counter++;
> > +               __sync_fetch_and_add(&counter, 1);
> >                 sw_sync_timeline_inc(timeline, 1);
> >         }
> >  
> > @@ -567,8 +567,8 @@ static void test_sync_multi_consumer(void)
> >         close(timeline);
> >         sem_destroy(&sem);
> >  
> > -       igt_assert_f(counter == MULTI_CONSUMER_THREADS * MULTI_CONSUMER_ITERATIONS,
> > -                    "Counter has unexpected value.\n");
> > +       igt_assert_eq(counter,
> > +                     MULTI_CONSUMER_THREADS * MULTI_CONSUMER_ITERATIONS);
> >  
> >         igt_assert_f(thread_ret == 0, "A sync thread reported failure.\n");
> >  }
> > @@ -589,11 +589,9 @@ static void * test_sync_multi_consumer_producer_thread(void *arg)
> >                 if (sync_fence_wait(fence, 1000) < 0)
> >                         return (void *) 1;
> >  
> > -               if (*(data->counter) != next_point)
> > +               if (__sync_fetch_and_add(data->counter, 1) != next_point)
> >                         return (void *) 1;
> >  
> > -               (*data->counter)++;
> 
> This is a behavior change, the counter gets incremented always :(

But it doesn't matter, since the return 1 here is failure. And any
thread reporting failure is enough.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] igt/sw_sync: Wrap threaded counter manipulation with mb

[Chris Wilson]

Quoting Chris Wilson (2018-05-03 20:40:53)
> Quoting Joonas Lahtinen (2018-05-03 17:55:57)
> > Quoting Chris Wilson (2018-05-02 12:28:07)
> > > @@ -589,11 +589,9 @@ static void * test_sync_multi_consumer_producer_thread(void *arg)
> > >                 if (sync_fence_wait(fence, 1000) < 0)
> > >                         return (void *) 1;
> > >  
> > > -               if (*(data->counter) != next_point)
> > > +               if (__sync_fetch_and_add(data->counter, 1) != next_point)
> > >                         return (void *) 1;
> > >  
> > > -               (*data->counter)++;
> > 
> > This is a behavior change, the counter gets incremented always :(
> 
> But it doesn't matter, since the return 1 here is failure. And any
> thread reporting failure is enough.

Poke.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] igt/sw_sync: Wrap threaded counter manipulation with mb

[Chris Wilson]

Quoting Joonas Lahtinen (2018-05-03 17:55:57)
> Quoting Chris Wilson (2018-05-02 12:28:07)
> > sw_sync/sync_multi_consumer_producer was communicating between threads
> > using the sw_sync ioctl and manipulating a shared volatile counter.
> > However, the ioctl itself does not imply a memory barrier, and so
> > different CPUs may see different states of the counter (the volatile
> > making GCC perform the operation in stages making the race even more
> > likely). Instead of using volatile, use locked operations to make the
> > counter manipulation thread-safe.
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=106344
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  tests/sw_sync.c | 24 +++++++++++-------------
> >  1 file changed, 11 insertions(+), 13 deletions(-)
> > 
> > diff --git a/tests/sw_sync.c b/tests/sw_sync.c
> > index 20dfbbb98..c6867e097 100644
> > --- a/tests/sw_sync.c
> > +++ b/tests/sw_sync.c
> > @@ -43,7 +43,7 @@ IGT_TEST_DESCRIPTION("Test SW Sync Framework");
> >  typedef struct {
> >         int timeline;
> >         uint32_t thread_id;
> > -       volatile uint32_t * volatile counter;
> > +       uint32_t *counter;
> >         sem_t *sem;
> >  } data_t;
> >  
> > @@ -508,7 +508,7 @@ static void * test_sync_multi_consumer_thread(void *arg)
> >                 if (sync_fence_wait(fence, 1000) < 0)
> >                         return (void *) 1;
> >  
> > -               if (*(data->counter) != next_point)
> > +               if (READ_ONCE(*data->counter) != next_point)
> >                         return (void *) 1;
> >  
> >                 sem_post(data->sem);
> > @@ -524,7 +524,7 @@ static void test_sync_multi_consumer(void)
> >         pthread_t thread_arr[MULTI_CONSUMER_THREADS];
> >         sem_t sem;
> >         int timeline;
> > -       volatile uint32_t counter = 0;
> > +       uint32_t counter = 0;
> >         uintptr_t thread_ret = 0;
> >         data_t data;
> >         int i, ret;
> > @@ -552,7 +552,7 @@ static void test_sync_multi_consumer(void)
> >         {
> >                 sem_wait(&sem);
> >  
> > -               counter++;
> > +               __sync_fetch_and_add(&counter, 1);
> >                 sw_sync_timeline_inc(timeline, 1);
> >         }
> >  
> > @@ -567,8 +567,8 @@ static void test_sync_multi_consumer(void)
> >         close(timeline);
> >         sem_destroy(&sem);
> >  
> > -       igt_assert_f(counter == MULTI_CONSUMER_THREADS * MULTI_CONSUMER_ITERATIONS,
> > -                    "Counter has unexpected value.\n");
> > +       igt_assert_eq(counter,
> > +                     MULTI_CONSUMER_THREADS * MULTI_CONSUMER_ITERATIONS);
> >  
> >         igt_assert_f(thread_ret == 0, "A sync thread reported failure.\n");
> >  }
> > @@ -589,11 +589,9 @@ static void * test_sync_multi_consumer_producer_thread(void *arg)
> >                 if (sync_fence_wait(fence, 1000) < 0)
> >                         return (void *) 1;
> >  
> > -               if (*(data->counter) != next_point)
> > +               if (__sync_fetch_and_add(data->counter, 1) != next_point)
> >                         return (void *) 1;
> >  
> > -               (*data->counter)++;
> 
> This is a behavior change, the counter gets incremented always :(

Care to check that this really doesn't matter as the test reports the
fail.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t] igt/sw_sync: Wrap threaded counter manipulation with mb

[Joonas Lahtinen]

Quoting Chris Wilson (2018-05-02 12:28:07)
> sw_sync/sync_multi_consumer_producer was communicating between threads
> using the sw_sync ioctl and manipulating a shared volatile counter.
> However, the ioctl itself does not imply a memory barrier, and so
> different CPUs may see different states of the counter (the volatile
> making GCC perform the operation in stages making the race even more
> likely). Instead of using volatile, use locked operations to make the
> counter manipulation thread-safe.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=106344
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Sorry, this has got buried along with the probably thousand other
patches.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev