[PATCH] Fix perf test data race for tsan build

[PATCH] Fix perf test data race for tsan build

[Numfor Mbiziwo-Tiapo]

The tsan build of perf gives a data race warning in
mmap-thread-lookup.c when perf test is run on our local machines.
This is because there is no lock around the "go_away" variable in
"mmap-thread-lookup.c".

The warning is difficult to reproduce from the tip branch and we
suspect it has something to do with tsan working differently
depending on the clang version.

The warning can be silenced by adding locks around the variable
but it is better practice to make the variable atomic and use
the atomic_set and atomic_read functions. This does not actually
silence the warning because tsan doesn't recognize the atomic
functions as thread safe, but in actuality it should prevent a
data race.

Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
---
 tools/perf/tests/mmap-thread-lookup.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/tests/mmap-thread-lookup.c b/tools/perf/tests/mmap-thread-lookup.c
index 5ede9b561d32..f4cbb3d92a46 100644
--- a/tools/perf/tests/mmap-thread-lookup.c
+++ b/tools/perf/tests/mmap-thread-lookup.c
@@ -17,7 +17,7 @@
 
 #define THREADS 4
 
-static int go_away;
+static atomic_t go_away;
 
 struct thread_data {
 	pthread_t	pt;
@@ -64,7 +64,7 @@ static void *thread_fn(void *arg)
 		return NULL;
 	}
 
-	while (!go_away) {
+	while (!atomic_read(&go_away)) {
 		/* Waiting for main thread to kill us. */
 		usleep(100);
 	}
@@ -98,7 +98,7 @@ static int threads_create(void)
 	struct thread_data *td0 = &threads[0];
 	int i, err = 0;
 
-	go_away = 0;
+	atomic_set(&go_away, 0);
 
 	/* 0 is main thread */
 	if (thread_init(td0))
@@ -118,7 +118,7 @@ static int threads_destroy(void)
 	/* cleanup the main thread */
 	munmap(td0->map, page_size);
 
-	go_away = 1;
+	atomic_set(&go_away, 1);
 
 	for (i = 1; !err && i < THREADS; i++)
 		err = pthread_join(threads[i].pt, NULL);
-- 
2.22.0.410.gd8fdbe21b5-goog

Re: [PATCH] Fix perf test data race for tsan build

[Jiri Olsa]

On Wed, Jul 10, 2019 at 03:16:20PM -0700, Numfor Mbiziwo-Tiapo wrote:
> The tsan build of perf gives a data race warning in

hi,
what's 'tsan build of perf' ?

thanks,
jirka


> mmap-thread-lookup.c when perf test is run on our local machines.
> This is because there is no lock around the "go_away" variable in
> "mmap-thread-lookup.c".
> 
> The warning is difficult to reproduce from the tip branch and we
> suspect it has something to do with tsan working differently
> depending on the clang version.
> 
> The warning can be silenced by adding locks around the variable
> but it is better practice to make the variable atomic and use
> the atomic_set and atomic_read functions. This does not actually
> silence the warning because tsan doesn't recognize the atomic
> functions as thread safe, but in actuality it should prevent a
> data race.
> 
> Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> ---
>  tools/perf/tests/mmap-thread-lookup.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/tests/mmap-thread-lookup.c b/tools/perf/tests/mmap-thread-lookup.c
> index 5ede9b561d32..f4cbb3d92a46 100644
> --- a/tools/perf/tests/mmap-thread-lookup.c
> +++ b/tools/perf/tests/mmap-thread-lookup.c
> @@ -17,7 +17,7 @@
>  
>  #define THREADS 4
>  
> -static int go_away;
> +static atomic_t go_away;
>  
>  struct thread_data {
>  	pthread_t	pt;
> @@ -64,7 +64,7 @@ static void *thread_fn(void *arg)
>  		return NULL;
>  	}
>  
> -	while (!go_away) {
> +	while (!atomic_read(&go_away)) {
>  		/* Waiting for main thread to kill us. */
>  		usleep(100);
>  	}
> @@ -98,7 +98,7 @@ static int threads_create(void)
>  	struct thread_data *td0 = &threads[0];
>  	int i, err = 0;
>  
> -	go_away = 0;
> +	atomic_set(&go_away, 0);
>  
>  	/* 0 is main thread */
>  	if (thread_init(td0))
> @@ -118,7 +118,7 @@ static int threads_destroy(void)
>  	/* cleanup the main thread */
>  	munmap(td0->map, page_size);
>  
> -	go_away = 1;
> +	atomic_set(&go_away, 1);
>  
>  	for (i = 1; !err && i < THREADS; i++)
>  		err = pthread_join(threads[i].pt, NULL);
> -- 
> 2.22.0.410.gd8fdbe21b5-goog
> 

Re: [PATCH] Fix perf test data race for tsan build

[Peter Zijlstra]

On Tue, Jul 16, 2019 at 11:10:39AM +0200, Jiri Olsa wrote:
> On Wed, Jul 10, 2019 at 03:16:20PM -0700, Numfor Mbiziwo-Tiapo wrote:
> > The tsan build of perf gives a data race warning in
> 
> hi,
> what's 'tsan build of perf' ?

Thread Sanitizer, from the group that brought us KASAN and UBSAN.

I'd much rather see the below use READ_ONCE() and WRITE_ONCE(). Using
atomic_t without RmW ops is confusing.

> > mmap-thread-lookup.c when perf test is run on our local machines.
> > This is because there is no lock around the "go_away" variable in
> > "mmap-thread-lookup.c".
> > 
> > The warning is difficult to reproduce from the tip branch and we
> > suspect it has something to do with tsan working differently
> > depending on the clang version.
> > 
> > The warning can be silenced by adding locks around the variable
> > but it is better practice to make the variable atomic and use
> > the atomic_set and atomic_read functions. This does not actually
> > silence the warning because tsan doesn't recognize the atomic
> > functions as thread safe, but in actuality it should prevent a
> > data race.
> > 
> > Signed-off-by: Numfor Mbiziwo-Tiapo <nums@google.com>
> > ---
> >  tools/perf/tests/mmap-thread-lookup.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tools/perf/tests/mmap-thread-lookup.c b/tools/perf/tests/mmap-thread-lookup.c
> > index 5ede9b561d32..f4cbb3d92a46 100644
> > --- a/tools/perf/tests/mmap-thread-lookup.c
> > +++ b/tools/perf/tests/mmap-thread-lookup.c
> > @@ -17,7 +17,7 @@
> >  
> >  #define THREADS 4
> >  
> > -static int go_away;
> > +static atomic_t go_away;
> >  
> >  struct thread_data {
> >  	pthread_t	pt;
> > @@ -64,7 +64,7 @@ static void *thread_fn(void *arg)
> >  		return NULL;
> >  	}
> >  
> > -	while (!go_away) {
> > +	while (!atomic_read(&go_away)) {
> >  		/* Waiting for main thread to kill us. */
> >  		usleep(100);
> >  	}
> > @@ -98,7 +98,7 @@ static int threads_create(void)
> >  	struct thread_data *td0 = &threads[0];
> >  	int i, err = 0;
> >  
> > -	go_away = 0;
> > +	atomic_set(&go_away, 0);
> >  
> >  	/* 0 is main thread */
> >  	if (thread_init(td0))
> > @@ -118,7 +118,7 @@ static int threads_destroy(void)
> >  	/* cleanup the main thread */
> >  	munmap(td0->map, page_size);
> >  
> > -	go_away = 1;
> > +	atomic_set(&go_away, 1);
> >  
> >  	for (i = 1; !err && i < THREADS; i++)
> >  		err = pthread_join(threads[i].pt, NULL);
> > -- 
> > 2.22.0.410.gd8fdbe21b5-goog
> >