linux-kernel - Re: [PATCH v2 4/4] rseq/selftests: Add test for mm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <199437fe21ad5bb47ce81c560fbeb0495b5f9ce3.camel@redhat.com>
Date: Fri, 13 Dec 2024 16:03:15 +0100
From: Gabriele Monaco <gmonaco@...hat.com>
To: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, 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, Ingo Molnar
	 <mingo@...hat.com>, Peter Zijlstra <peterz@...radead.org>, Andrew Morton
	 <akpm@...ux-foundation.org>
Subject: Re: [PATCH v2 4/4] rseq/selftests: Add test for mm_cid compaction


On Fri, 2024-12-13 at 09:29 -0500, Mathieu Desnoyers wrote:
> 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.
> > 
> > + /*
> > + * 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.

Right, good point.

> > +     !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 ?
> > 
> > +void test_mm_cid_compaction(void)
> > +{
> > + 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.

Good idea, that would get two birds with one stone.
I just forgot to pass it but it seems the main thread is perfectly
joinable (just checked), so that should work fairly easily.

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

Sure, will do!

Thanks for the comments, working on V3.
Gabriele