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

From: Ian Rogers
Date: Mon Oct 12 2020 - 12:17:55 EST

Next message: kernel test robot: "Re: [PATCH v6 2/3] dmaengine: ptdma: register PTDMA controller as a DMA resource"
Previous message: Ian Rogers: "[PATCH v2] perf bench: Use condition variables in numa."
In reply to: Jiri Olsa: "Re: [PATCH] perf bench: Use condition variables in numa."
Next in thread: kernel test robot: "[perf bench] 3a25d42fc8: make perf failed"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Oct 12, 2020 at 2:45 AM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx>
>
> 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

Sorry was trying to work with two trees, and thought this had
built/tested obviously not given the typo. Fixed in:
https://lore.kernel.org/lkml/20201012161611.366482-1-irogers@xxxxxxxxxx/T/#u

Thanks,
Ian

> > ---
> > 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
> >
>

Next message: kernel test robot: "Re: [PATCH v6 2/3] dmaengine: ptdma: register PTDMA controller as a DMA resource"
Previous message: Ian Rogers: "[PATCH v2] perf bench: Use condition variables in numa."
In reply to: Jiri Olsa: "Re: [PATCH] perf bench: Use condition variables in numa."
Next in thread: kernel test robot: "[perf bench] 3a25d42fc8: make perf failed"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]