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

[Vipin Sharma]

On Fri, Oct 7, 2022 at 10:39 AM Vipin Sharma <vipinsh@xxxxxxxxxx> wrote:
>
> On Thu, Oct 6, 2022 at 5:14 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> >
> > On Thu, Oct 06, 2022, Vipin Sharma wrote:
> > > On Thu, Oct 6, 2022 at 12:50 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > > > > +{
> > > > > +     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.
> >
> > Oh, I'm not talking about the user pinning multiple vCPUs to the same pCPU via
> > the test, I'm talking about the user, or more likely something in the users's
> > environment, restricting what pCPUs the user's tasks are allowed on.  E.g. if
> > the test is run in shell that has been restricted to CPU8 via cgroups, then
> > sched_setaffinity() will fail if the user tries to pin vCPUs to any other CPU.
>
> I see, I will add this validation.

I think we should drop this check. Current logic is that the new
function perf_test_setup_pinning() parses the vcpu mappings, stores
them in perf_test_vcpu_args{} struct and moves the main thread to the
provided pcpu. But this causes TEST_ASSERT(CPU_ISSET...) to fail for
vcpu threads when they are created because they inherit task affinity
from the main thread which has the pcpu set during setup.

However, this affinity is not strict, so, if TEST_ASSERT(CPU_ISSET...)
is removed then vcpu threads successfully move to their required pcpu
via sched_setaffinity() even though the main thread has different
affinity. If cpus are restricted via cgroups then sched_setaffinity()
fails as expected no matter what.

Another option will be to split the API, perf_test_setup_pinning()
will return the main thread pcpu and dirty_log_perf_test can call
pin_this_task_to_cpu() with the returned pcpu after vcpus have been
started. I do not like this approach, I also think
TEST_ASSERT(CPU_ISSET...) is not reducing user pain that much because
users can still figure out with returned errno what is happening.