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

[Chris Wilson <chris@chris-wilson.co.uk>]

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