linux-kernel - Re: [PATCH] KVM: selftests: Run dirty_log_perf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHVum0dJBwtc5yNzK=n2OQn8YZohTxgFST0XBPUWweQ+KuSeWQ@mail.gmail.com>
Date:   Thu, 18 Aug 2022 10:58:03 -0700
From:   Vipin Sharma <vipinsh@...gle.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     dmatlack@...gle.com, pbonzini@...hat.com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: selftests: Run dirty_log_perf_test on specific cpus

On Wed, Aug 17, 2022 at 2:29 PM Sean Christopherson <seanjc@...gle.com> wrote:
>
> On Wed, Aug 17, 2022, Vipin Sharma wrote:
> > On Wed, Aug 17, 2022 at 10:25 AM Sean Christopherson <seanjc@...gle.com> wrote:

> > We need error checking here to make sure that the user really wants
> > cpu 0 and it was not a mistake in typing.
> > I was thinking of using parse_num API for other places as well instead
> > of atoi() in dirty_log_perf_test.
>
> Yes, definitely.  And maybe give it a name like atoi_paranoid()?

Lol. Absolutely, if that's what you want!

>
> > Yeah, it was either my almost duplicate functions or have the one
> > function do two things via if-else.  I am not happy with both
> > approaches.
> >
> > I think I will pass an integer array which this parsing function will
> > fill up and return an int denoting how many elements were filled. The
> > caller then can use the array as they wish, to copy it in
> > vcpu_to_lcpu_map or cpuset.
>
> Eh, I doubt that'll be a net improvement, e.g. the CPUSET case will then need to
> re-loop, which seems silly.  If the exclusive cpuset vs. array is undesirable, we
> could have the API require at least one instead of exactly one, i.e.
>
>         TEST_ASSERT(cpuset || vcpu_map);
>
>         ...
>
>                 cpu = atoi(cpustr);
>                 TEST_ASSERT(cpu >= 0, "Invalid cpu number: %d\n", cpu);
>                 if (vcpu_map)
>                         vcpu_map[i++] = cpu;
>                 if (cpuset)
>                         CPU_SET(cpu, cpuset);
>
> If we somehow end up with a third type of destination, then we can revisit this,
> but that seems unlikely at this point.
>

I am removing the -d option, so this is not needed anymore.


> > > I wonder if we should make -c and -d mutually exclusive.  Tweak -c to include the
> > > application thread, i.e. TEST_ASSERT(nr_lcpus == nr_vcpus+1) and require 1:1 pinning
> > > for all tasks.  E.g. allowing "-c ..., -d 0,1,22" seems unnecessary.
> > >
> >
> > One downside I can think of will be if we add some worker threads
> > which are not vcpus then all of those threads will end up running on a
> > single cpu unless we edit this parsing logic again.
>
> But adding worker threads also requires a code change, i.e. it won't require a
> separate commit/churn.  And if we get to the point where we want multiple workers,
> it should be relatively straightforward to support pinning an arbitrary number of
> workers, e.g.
>
>         enum memtest_worker_type {
>                 MAIN_WORKER,
>                 MINION_1,
>                 MINION_2,
>                 NR_MEMTEST_WORKERS,
>         }
>
>
>         TEST_ASSERT(nr_lcpus == nr_vcpus + NR_MEMTEST_WORKERS);
>
> void spawn_worker(enum memtest_worker_type type, <function pointer>)
> {
>         cpu_set_t cpuset;
>
>         CPU_ZERO(&cpuset);
>         CPU_SET(task_map[nr_vcpus + type], &cpuset);
>
>         <set affinity and spawn>
> }
>
> > Current implementation gives vcpus special treatment via -c and for
> > the whole application via -d. This gives good separation of concerns
> > via flags.
>
> But they aren't separated, e.g. using -d without -c means vCPUs are thrown into
> the same pool as worker threads.  And if we ever do add more workers, -d doesn't
> allow the user to pin workers 1:1 with logical CPUs.
>
> Actually, if -c is extended to pin workers, then adding -d is unnecessary.  If the
> user wants to affine tasks to CPUs but not pin 1:1, it can do that with e.g. taskset.
> What the user can't do is pin 1:1.
>
> If we don't want to _require_ the caller to pin the main worker, then we could do
>
>         TEST_ASSERT(nr_lcpus >= nr_vcpus &&
>                     nr_lcpus <= nr_vcpus + NR_MEMTEST_WORKERS);
>
> to _require_ pinning all vCPUs, and allow but not require pinning non-vCPU tasks.

Okay, I will remove -d and only keep -c. I will extend it to support
pinning the main worker and vcpus. Arguments to -c will be like:
<main woker lcpu>, <vcpu0's lcpu>, <vcpu1's lcpu>, <vcpu2's lcpu>,...
Example:
./dirty_log_perf_test -v 3 -c 1,20,21,22

Main worker will run on 1 and 3 vcpus  will run on logical cpus 20, 21 and 22.

Sounds good?

Thanks
Vipin