Re: [PATCH v2 4/4] rseq/selftests: Add test for mm_cid compaction

[Gabriele Monaco <gmonaco@redhat.com>]

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