Re: [PATCH v4 4/4] KVM: selftests: Run dirty_log_perf_test on specific CPUs

[Vipin Sharma]

On Thu, Oct 6, 2022 at 12:50 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Thu, Oct 06, 2022, Vipin Sharma wrote:
> ...
>
> > +static void pin_me_to_pcpu
>
> Maybe s/me/this_task ?

Sure.

>
> > (int pcpu)
>
> Unless we're using -1 as "don't pin" or "invalid", this should be an unsigned value.
>
> > +{
> > +     cpu_set_t cpuset;
> > +     int err;
> > +
> > +     CPU_ZERO(&cpuset);
> > +     CPU_SET(pcpu, &cpuset);
>
> To save user pain:
>
>         r = sched_getaffinity(0, sizeof(allowed_mask), &allowed_mask);
>         TEST_ASSERT(!r, "sched_getaffinity failed, errno = %d (%s)", errno,
>                     strerror(errno));
>
>         TEST_ASSERT(CPU_ISSET(pcpu, &allowed_mask),
>                     "Task '%d' not allowed to run on pCPU '%d'\n");
>
>         CPU_ZERO(&allowed_mask);
>         CPU_SET(cpu, &allowed_mask);
>
> that way the user will get an explicit error message if they try to pin a vCPU/task
> that has already been affined by something else.  And then, in theory,
> sched_setaffinity() should never fail.
>
> Or you could have two cpu_set_t objects and use CPU_AND(), but that seems
> unnecessarily complex.
>

sched_setaffinity() doesn't fail when we assign more than one task to
the pCPU, it allows multiple tasks to be on the same pCPU. One of the
reasons it fails is if it is provided a cpu number which is bigger
than what is actually available on the host.

I am not convinced that pinning vCPUs to the same pCPU should throw an
error. We should allow if someone wants to try and compare performance
by over subscribing or any valid combination they want to test.

...

> > +static int pcpu_num(const char *cpu_str)
> > +{
> > +     int pcpu = atoi_paranoid(cpu_str);
>
> newline after declaration.  Though maybe just omit this helper entirely?  As a
> somewhat belated thought, it's trivial to let "-1" mean "don't pin this vCPU".
> No idea if there's a use case for that, but it's not any more work to support.
>
> Even if <0 is invalid, what about just having pin_task_to_pcu() do all the
> sanity checking?  That way it's more obvious that that helper isn't failing to
> sanity check the incoming value.
>

This will go away with atoi_non_negative() API I will write in v5. I
won't even need this function then.

...

> > +     while (cpu && i < nr_vcpus) {
> > +             perf_test_args.vcpu_args[i++].pcpu = pcpu_num(cpu);
> > +             cpu = strtok(NULL, delim);
> > +     }
> > +
> > +     TEST_ASSERT(i == nr_vcpus,
> > +                 "Number of pcpus (%d) not sufficient for the number of vcpus (%d).",
> > +                 i, nr_vcpus);
>
> Rather than assert after the fact, use a for-loop:
>
>         for (i = 0; i < nr_vcpus; i++ {
>                 TEST_ASSERT(cpu, "pCPU not provided for vCPU%d\n", i);
>                 perf_test_args.vcpu_args[i++].pcpu = atoi_paranoid(cpu);
>                 cpu = strtok(NULL, delim);
>         }
>
> so as to avoid having to consume the loop control variable before and after the
> loop.  Or even
>
>         for (i = 0, cpu = strtok(cpu_list, delim);
>              i < nr_vcpus;
>              i++, cpu = strtok(NULL, delim)) {
>                 TEST_ASSERT(cpu, "pCPU not provided for vCPU%d\n", i);
>                 perf_test_args.vcpu_args[i++].pcpu = atoi_paranoid(cpu);
>         }
>
> Though IMO the latter is gratuitous and hard to read.
>

I will use the former one.

> > +
> > +     perf_test_args.pin_vcpus = true;
> > +
> > +     // 2. Check if main worker is provided
> > +     if (cpu)
> > +             pin_me_to_pcpu(pcpu_num(cpu));
>
> Verify the string is now empty?  I.e. that there isn't trailing garbage.
>

Okay, I will add the verification.

All other suggestions to which I haven't responded, I agree with them
and will make the changes in v5.