Message-ID: <d1e64ae2-9a16-44af-afca-a1940f27d4ef@efficios.com>
Date: Fri, 13 Dec 2024 09:29:31 -0500
From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To: Gabriele Monaco <gmonaco@...hat.com>, Ingo Molnar <mingo@...hat.com>,
 Peter Zijlstra <peterz@...radead.org>,
 Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org
Cc: Juri Lelli <juri.lelli@...hat.com>,
 Vincent Guittot <vincent.guittot@...aro.org>, Mel Gorman <mgorman@...e.de>,
 Shuah Khan <shuah@...nel.org>, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v2 4/4] rseq/selftests: Add test for mm_cid compaction

On 2024-12-13 04:54, Gabriele Monaco wrote:
> A task in the kernel (task_mm_cid_work) runs somewhat periodically to
> compact the mm_cid for each process, this test tries to validate that
> it runs correctly and timely.
> 
> The test spawns 1 thread pinned to each CPU, then each thread, including
> the main one, run in short bursts for some time. During this period, the
> mm_cids should be spanning all numbers between 0 and nproc.
> 
> At the end of this phase, a thread with high enough mm_cid (> nproc/2)
> is selected to be the new leader, all other threads terminate.
> 
> After some time, the only running thread should see 0 as mm_cid, if that
> doesn't happen, the compaction mechanism didn't work and the test fails.
> 
> The test never fails if only 1 core is available, in which case, we
> cannot test anything as the only available mm_cid is 0.
> 
> Signed-off-by: Gabriele Monaco <gmonaco@...hat.com>
> ---
>   tools/testing/selftests/rseq/.gitignore       |   1 +
>   tools/testing/selftests/rseq/Makefile         |   2 +-
>   .../selftests/rseq/mm_cid_compaction_test.c   | 157 ++++++++++++++++++
>   3 files changed, 159 insertions(+), 1 deletion(-)
>   create mode 100644 tools/testing/selftests/rseq/mm_cid_compaction_test.c
> 
> diff --git a/tools/testing/selftests/rseq/.gitignore b/tools/testing/selftests/rseq/.gitignore
> index 16496de5f6ce..2c89f97e4f73 100644
> --- a/tools/testing/selftests/rseq/.gitignore
> +++ b/tools/testing/selftests/rseq/.gitignore
> @@ -3,6 +3,7 @@ basic_percpu_ops_test
>   basic_percpu_ops_mm_cid_test
>   basic_test
>   basic_rseq_op_test
> +mm_cid_compaction_test
>   param_test
>   param_test_benchmark
>   param_test_compare_twice
> diff --git a/tools/testing/selftests/rseq/Makefile b/tools/testing/selftests/rseq/Makefile
> index 5a3432fceb58..ce1b38f46a35 100644
> --- a/tools/testing/selftests/rseq/Makefile
> +++ b/tools/testing/selftests/rseq/Makefile
> @@ -16,7 +16,7 @@ OVERRIDE_TARGETS = 1
>   
>   TEST_GEN_PROGS = basic_test basic_percpu_ops_test basic_percpu_ops_mm_cid_test param_test \
>   		param_test_benchmark param_test_compare_twice param_test_mm_cid \
> -		param_test_mm_cid_benchmark param_test_mm_cid_compare_twice
> +		param_test_mm_cid_benchmark param_test_mm_cid_compare_twice mm_cid_compaction_test
>   
>   TEST_GEN_PROGS_EXTENDED = librseq.so
>   
> diff --git a/tools/testing/selftests/rseq/mm_cid_compaction_test.c b/tools/testing/selftests/rseq/mm_cid_compaction_test.c
> new file mode 100644
> index 000000000000..9bc7310c3cb5
> --- /dev/null
> +++ b/tools/testing/selftests/rseq/mm_cid_compaction_test.c
> @@ -0,0 +1,157 @@
> +// SPDX-License-Identifier: LGPL-2.1
> +#define _GNU_SOURCE
> +#include <assert.h>
> +#include <pthread.h>
> +#include <sched.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <stddef.h>
> +
> +#include "../kselftest.h"
> +#include "rseq.h"
> +
> +#define VERBOSE 0
> +#define printf_verbose(fmt, ...)                    \
> +	do {                                        \
> +		if (VERBOSE)                        \
> +			printf(fmt, ##__VA_ARGS__); \
> +	} while (0)
> +
> +/* 0.5 s */
> +#define RUNNER_PERIOD 500000
> +/* Number of runs before we terminate or get the token */
> +#define THREAD_RUNS 5
> +
> +/*
> + * Number of times we check that the mm_cid were compacted.
> + * Checks are repeated every RUNNER_PERIOD
> + */
> +#define MM_CID_CLEANUP_TIMEOUT 10
> +
> +struct thread_args {
> +	int num_cpus;
> +	pthread_mutex_t token;
> +	pthread_t *tinfo;
> +};
> +
> +static void *thread_runner(void *arg)
> +{
> +	struct thread_args *args = arg;
> +	int i, ret, curr_mm_cid;
> +
> +	for (i = 0; i < THREAD_RUNS; i++)
> +		usleep(RUNNER_PERIOD);
> +	curr_mm_cid = rseq_current_mm_cid();
> +	/*
> +	 * We select one thread with high enough mm_cid to be the new leader
> +	 * all other threads (including the main thread) will terminate
> +	 * After some time, the mm_cid of the only remaining thread should
> +	 * converge to 0, if not, the test fails
> +	 */
> +	if (curr_mm_cid > args->num_cpus / 2 &&

I think we want  curr_mm_cid >= args->num_cpus / 2   here,
otherwise the case with 2 cpus would not match.

> +	    !pthread_mutex_trylock(&args->token)) {
> +		printf_verbose("cpu%d has %d and will be the new leader\n",
> +			       sched_getcpu(), curr_mm_cid);
> +		for (i = 0; i < args->num_cpus; i++) {
> +			if (args->tinfo[i] == pthread_self())
> +				continue;
> +			ret = pthread_join(args->tinfo[i], NULL);

We'd want a synchronization point to join the main thread. I'm not sure
if the main thread is joinable.

Perhaps we could try calling pthread_self() from the main thread, and
store that in the main thread struct thread_args, and use it to join
the main thread afterwards ?

> +			if (ret) {
> +				fprintf(stderr,
> +					"Error: failed to join thread %d (%d): %s\n",
> +					i, ret, strerror(ret));
> +				assert(ret == 0);
> +			}
> +		}
> +		free(args->tinfo);
> +
> +		for (i = 0; i < MM_CID_CLEANUP_TIMEOUT; i++) {
> +			curr_mm_cid = rseq_current_mm_cid();
> +			printf_verbose("run %d: mm_cid %d on cpu%d\n", i,
> +				       curr_mm_cid, sched_getcpu());
> +			if (curr_mm_cid == 0) {
> +				printf_verbose(
> +					"mm_cids successfully compacted, exiting\n");
> +				pthread_exit(NULL);
> +			}
> +			usleep(RUNNER_PERIOD);
> +		}
> +		assert(false);
> +	}
> +	printf_verbose("cpu%d has %d and is going to terminate\n",
> +		       sched_getcpu(), curr_mm_cid);
> +	pthread_exit(NULL);
> +}
> +
> +void test_mm_cid_compaction(void)
> +{
> +	cpu_set_t affinity, test_affinity;
> +	int i, j, ret, num_threads;
> +	pthread_t *tinfo;
> +	struct thread_args args = { .token = PTHREAD_MUTEX_INITIALIZER };
> +
> +	sched_getaffinity(0, sizeof(affinity), &affinity);
> +	CPU_ZERO(&test_affinity);
> +	num_threads = CPU_COUNT(&affinity);
> +	tinfo = calloc(num_threads, sizeof(*tinfo));
> +	if (!tinfo) {
> +		fprintf(stderr, "Error: failed to allocate tinfo(%d): %s\n",
> +			errno, strerror(errno));
> +		assert(ret == 0);
> +	}
> +	args.num_cpus = num_threads;
> +	args.tinfo = tinfo;
> +	if (num_threads == 1) {
> +		printf_verbose(
> +			"Running on a single cpu, cannot test anything\n");
> +		return;
> +	}
> +	for (i = 0, j = 0; i < CPU_SETSIZE && j < num_threads; i++) {
> +		if (CPU_ISSET(i, &affinity)) {

Including the main thread, we end up creating nr_cpus + 1 threads.
I suspect we want to take the main thread into account here, and create
one less thread.

We could use tinfo[0] to store the main thread info.

> +			ret = pthread_create(&tinfo[j], NULL, thread_runner,
> +					     &args);
> +			if (ret) {
> +				fprintf(stderr,
> +					"Error: failed to create thread(%d): %s\n",
> +					ret, strerror(ret));
> +				assert(ret == 0);
> +			}
> +			CPU_SET(i, &test_affinity);
> +			pthread_setaffinity_np(tinfo[j], sizeof(test_affinity),
> +					       &test_affinity);

It would be better that each thread set their own affinity when
they start rather than having the main thread set each created thread
affinity while they are already running. Otherwise it's racy and
timing-dependent.

And don't forget to set the main thread's affinity.

Thanks,

Mathieu

> +			CPU_CLR(i, &test_affinity);
> +			++j;
> +		}
> +	}
> +	printf_verbose("Started %d threads\n", num_threads);
> +
> +	/* Also main thread will terminate if it is not selected as leader */
> +	thread_runner(&args);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	if (rseq_register_current_thread()) {
> +		fprintf(stderr,
> +			"Error: rseq_register_current_thread(...) failed(%d): %s\n",
> +			errno, strerror(errno));
> +		goto error;
> +	}
> +	if (!rseq_mm_cid_available()) {
> +		fprintf(stderr, "Error: rseq_mm_cid unavailable\n");
> +		goto error;
> +	}
> +	test_mm_cid_compaction();
> +	if (rseq_unregister_current_thread()) {
> +		fprintf(stderr,
> +			"Error: rseq_unregister_current_thread(...) failed(%d): %s\n",
> +			errno, strerror(errno));
> +		goto error;
> +	}
> +	return 0;
> +
> +error:
> +	return -1;
> +}

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Powered by blists - more mailing lists