Re: [PATCH v3 3/3] rseq/selftests: Add test for mm_cid compaction

[Mathieu Desnoyers]

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