Message-ID: <f762c6e5-02b8-4a71-b076-1c629c104dc5@efficios.com>
Date: Thu, 26 Dec 2024 09:17:41 -0500
From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To: Gabriele Monaco <gmonaco@...hat.com>,
 Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...nel.org>,
 linux-mm@...ck.org, linux-kernel@...r.kernel.org
Cc: Juri Lelli <juri.lelli@...hat.com>, Shuah Khan <shuah@...nel.org>
Subject: Re: [PATCH v3 3/3] rseq/selftests: Add test for mm_cid compaction

On 2024-12-26 04:04, Gabriele Monaco wrote:
> 
> On Tue, 2024-12-24 at 11:20 -0500, Mathieu Desnoyers wrote:
>> On 2024-12-16 08:09, 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.
>>>
>>> + if (curr_mm_cid == 0) {
>>> + printf_verbose(
>>> + "mm_cids successfully compacted, exiting\n");
>>> + pthread_exit(NULL);
>>> + }
>>> + usleep(RUNNER_PERIOD);
>>> + }
>>> + assert(false);
>>
>> I suspect we'd want an explicit error message here
>> with an abort() rather than an assertion which can be
>> compiled-out with -DNDEBUG.
>>
>>> + }
>>> + 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)
>>
>> This function should return its error to the caller
>> rather than assert.
>>
>>> +{
>>> + cpu_set_t affinity;
>>> + int i, j, ret, num_threads;
>>> + pthread_t *tinfo;
>>> + pthread_mutex_t *token;
>>> + struct thread_args *args;
>>> +
>>> + sched_getaffinity(0, sizeof(affinity), &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 = calloc(num_threads, sizeof(*args));
>>> + if (!args) {
>>> + fprintf(stderr, "Error: failed to allocate args(%d): %s\n",
>>> + errno, strerror(errno));
>>> + assert(ret == 0);
>>> + }
>>> + token = calloc(num_threads, sizeof(*token));
>>> + if (!token) {
>>> + fprintf(stderr, "Error: failed to allocate token(%d): %s\n",
>>> + errno, strerror(errno));
>>> + assert(ret == 0);
>>> + }
>>> + if (num_threads == 1) {
>>> + printf_verbose(
>>> + "Running on a single cpu, cannot test anything\n");
>>> + return;
>>
>> This should return a value telling the caller that
>> the test is skipped (not an error per se).
>>
> 
> Thanks for the review!
> I'm not sure how to properly handle these, but it seems to me the
> cleanest way is to use ksft_* functions to report failures and skipped
> tests. Other tests in rseq don't use the library but it doesn't seem a
> big deal if just one test is using it, for now.

For the moment, we could do like the following test which
does a skip:

void test_membarrier(void)
{
         fprintf(stderr, "rseq_offset_deref_addv is not implemented on this architecture. "
                         "Skipping membarrier test.\n");
}

We can revamp the rest of the tests to use ksft in the future.

Currently everything is driven from run_param_test.sh, and it would
require significant rework to move to ksft.

> 
> It gets a bit complicated to return values since we are exiting from
> the main thread (sure we could join the remaining /winning/ thread but
> we would end up with 2 threads running). The ksft_* functions solve
> this quite nicely using exit codes, though.

Then thread_running should be marked with the noreturn attribute.

test_mm_cid_compaction can indeed return if it fails in the preparation
stages, just not when calling thread_running.

So we want test_mm_cid_compaction to return errors so main can handle
them, and we may want to move the call to thread_running directly
into main after success of test_mm_cid_compaction preparation step.

It's not like we can append any further test after this noreturn call.

> 
>>> + }
>>> + pthread_mutex_init(token, NULL);
>>> + /* The main thread runs on CPU0 */
>>> + for (i = 0, j = 0; i < CPU_SETSIZE && j < num_threads; i++) {
>>> + if (CPU_ISSET(i, &affinity)) {
>>
>> We can save an indent level here by moving this
>> in the for () condition:
>>
>>   for (i = 0, j = 0; i < CPU_SETSIZE &&
>>        CPU_ISSET(i, &affinity) && j < num_threads; i++) {
>>
> 
> Well, if we assume the affinity mask is contiguous, which is likely but
> not always true. A typical setup with isolated CPUs have one
> housekeeping core per NUMA node, let's say 0,32,64,96 out of 127 cpus,
> the test would run only on cpu 0 in that case.

Whooops, yes, you are right. Scratch that. It would become
an end loop condition, which is not what we want here.

then to remove indent level we'd want:

   if (!CPU_ISSET(i, &affinity))
       continue;

in the for loop.

> 
>>> + args[j].num_cpus = num_threads;
>>> + args[j].tinfo = tinfo;
>>> + args[j].token = token;
>>> + args[j].cpu = i;
>>> + args[j].args_head = args;
>>> + if (!j) {
>>> + /* The first thread is the main one */
>>> + tinfo[0] = pthread_self();
>>> + ++j;
>>> + continue;
>>> + }
>>> + ret = pthread_create(&tinfo[j], NULL, thread_runner,
>>> +      &args[j]);
>>> + if (ret) {
>>> + fprintf(stderr,
>>> + "Error: failed to create thread(%d): %s\n",
>>> + ret, strerror(ret));
>>> + assert(ret == 0);
>>> + }
>>> + ++j;
>>> + }
>>> + }
>>> + printf_verbose("Started %d threads\n", num_threads);
>>
>> I think there is a missing rendez-vous point here. Assuming a
>> sufficiently long unexpected delay (think of a guest VM VCPU
>> preempted for a long time), the new leader can start poking
>> into args and other thread's info while we are still creating
>> threads here.
>>
> 
> Yeah, good point, I'm assuming all threads are ready by the time we are
> done waiting but that's not bulletproof. I'll add a barrier.
> 
> Thanks again for the comments, I'll prepare a V4.

Thanks!

Mathieu

> Gabriele
> 

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

Powered by blists - more mailing lists