Re: [PATCH] perf bench: Use condition variables in numa.

[Jiri Olsa <jolsa@redhat.com>]

On Fri, Oct 09, 2020 at 11:40:56PM -0700, Ian Rogers wrote:
> The existing approach to synchronization between threads in the numa
> benchmark is unbalanced mutexes. This synchronization causes thread
> sanitizer to warn of locks being taken twice on a thread without an
> unlock, as well as unlocks with no corresponding locks.
> This change replaces the synchronization with more regular condition
> variables. While this fixes one class of thread sanitizer warnings,
> there still remain warnings of data races due to threads reading and
> writing shared memory without any atomics.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>

hi,
I'm getting:

  BUILD:   Doing 'make -j8' parallel build
Warning: Kernel ABI header at 'tools/include/uapi/linux/perf_event.h' differs from latest version at 'include/uapi/linux/perf_event.h'
diff -u tools/include/uapi/linux/perf_event.h include/uapi/linux/perf_event.h
  CC       bench/numa.o
bench/numa.c: In function ‘init’:
bench/numa.c:1461:24: error: ‘struct global_info’ has no member named ‘startup_done_mutex’; did you mean ‘startup_mutex’?
 1461 |  init_global_mutex(&g->startup_done_mutex);
      |                        ^~~~~~~~~~~~~~~~~~
      |                        startup_mutex
bench/numa.c:1462:23: error: ‘struct global_info’ has no member named ‘startup_done_cond’; did you mean ‘startup_cond’?
 1462 |  init_global_cond(&g->startup_done_cond);
      |                       ^~~~~~~~~~~~~~~~~
      |                       startup_cond

jirka


> ---
>  tools/perf/bench/numa.c | 67 ++++++++++++++++++++++++++++-------------
>  1 file changed, 46 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/perf/bench/numa.c b/tools/perf/bench/numa.c
> index f85bceccc459..d8c5a674203e 100644
> --- a/tools/perf/bench/numa.c
> +++ b/tools/perf/bench/numa.c
> @@ -137,12 +137,13 @@ struct global_info {
>  	u8			*data;
>  
>  	pthread_mutex_t		startup_mutex;
> +	pthread_cond_t		startup_cond;
>  	int			nr_tasks_started;
>  
> -	pthread_mutex_t		startup_done_mutex;
> -
>  	pthread_mutex_t		start_work_mutex;
> +	pthread_cond_t		start_work_cond;
>  	int			nr_tasks_working;
> +	bool			start_work;
>  
>  	pthread_mutex_t		stop_work_mutex;
>  	u64			bytes_done;
> @@ -483,6 +484,18 @@ static void init_global_mutex(pthread_mutex_t *mutex)
>  	pthread_mutex_init(mutex, &attr);
>  }
>  
> +/*
> + * Return a process-shared (global) condition variable:
> + */
> +static void init_global_cond(pthread_cond_t *cond)
> +{
> +	pthread_condattr_t attr;
> +
> +	pthread_condattr_init(&attr);
> +	pthread_condattr_setpshared(&attr, PTHREAD_PROCESS_SHARED);
> +	pthread_cond_init(cond, &attr);
> +}
> +
>  static int parse_cpu_list(const char *arg)
>  {
>  	p0.cpu_list_str = strdup(arg);
> @@ -1136,15 +1149,18 @@ static void *worker_thread(void *__tdata)
>  	if (g->p.serialize_startup) {
>  		pthread_mutex_lock(&g->startup_mutex);
>  		g->nr_tasks_started++;
> +		/* The last thread wakes the main process. */
> +		if (g->nr_tasks_started == g->p.nr_tasks)
> +			pthread_cond_signal(&g->startup_cond);
> +
>  		pthread_mutex_unlock(&g->startup_mutex);
>  
>  		/* Here we will wait for the main process to start us all at once: */
>  		pthread_mutex_lock(&g->start_work_mutex);
> +		g->start_work = false;
>  		g->nr_tasks_working++;
> -
> -		/* Last one wake the main process: */
> -		if (g->nr_tasks_working == g->p.nr_tasks)
> -			pthread_mutex_unlock(&g->startup_done_mutex);
> +		while (!g->start_work)
> +			pthread_cond_wait(&g->start_work_cond, &g->start_work_mutex);
>  
>  		pthread_mutex_unlock(&g->start_work_mutex);
>  	}
> @@ -1441,8 +1457,9 @@ static int init(void)
>  
>  	/* Startup serialization: */
>  	init_global_mutex(&g->start_work_mutex);
> -	init_global_mutex(&g->startup_mutex);
> +	init_global_cond(&g->start_work_cond);
>  	init_global_mutex(&g->startup_done_mutex);
> +	init_global_cond(&g->startup_done_cond);
>  	init_global_mutex(&g->stop_work_mutex);
>  
>  	init_thread_data();
> @@ -1502,9 +1519,6 @@ static int __bench_numa(const char *name)
>  	pids = zalloc(g->p.nr_proc * sizeof(*pids));
>  	pid = -1;
>  
> -	/* All threads try to acquire it, this way we can wait for them to start up: */
> -	pthread_mutex_lock(&g->start_work_mutex);
> -
>  	if (g->p.serialize_startup) {
>  		tprintf(" #\n");
>  		tprintf(" # Startup synchronization: ..."); fflush(stdout);
> @@ -1526,22 +1540,29 @@ static int __bench_numa(const char *name)
>  		pids[i] = pid;
>  
>  	}
> -	/* Wait for all the threads to start up: */
> -	while (g->nr_tasks_started != g->p.nr_tasks)
> -		usleep(USEC_PER_MSEC);
> -
> -	BUG_ON(g->nr_tasks_started != g->p.nr_tasks);
>  
>  	if (g->p.serialize_startup) {
> +		bool threads_ready = false;
>  		double startup_sec;
>  
> -		pthread_mutex_lock(&g->startup_done_mutex);
> +		/*
> +		 * Wait for all the threads to start up. The last thread will
> +		 * signal this process.
> +		 */
> +		pthread_mutex_lock(&g->startup_mutex);
> +		while (g->nr_tasks_started != g->p.nr_tasks)
> +			pthread_cond_wait(&g->startup_cond, &g->startup_mutex);
>  
> -		/* This will start all threads: */
> -		pthread_mutex_unlock(&g->start_work_mutex);
> +		pthread_mutex_unlock(&g->startup_mutex);
>  
> -		/* This mutex is locked - the last started thread will wake us: */
> -		pthread_mutex_lock(&g->startup_done_mutex);
> +		/* Wait for all threads to be at the start_work_cond. */
> +		while (!threads_ready) {
> +			pthread_mutex_lock(&g->start_work_mutex);
> +			threads_ready = (g->nr_tasks_working == g->p.nr_tasks);
> +			pthread_mutex_unlock(&g->start_work_mutex);
> +			if (!threads_ready)
> +				usleep(1);
> +		}
>  
>  		gettimeofday(&stop, NULL);
>  
> @@ -1555,7 +1576,11 @@ static int __bench_numa(const char *name)
>  		tprintf(" #\n");
>  
>  		start = stop;
> -		pthread_mutex_unlock(&g->startup_done_mutex);
> +		/* Start all threads running. */
> +		pthread_mutex_lock(&g->start_work_mutex);
> +		g->start_work = true;
> +		pthread_mutex_unlock(&g->start_work_mutex);
> +		pthread_cond_broadcast(&g->start_work_cond);
>  	} else {
>  		gettimeofday(&start, NULL);
>  	}
> -- 
> 2.28.0.1011.ga647a8990f-goog
>