Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220819210737.763135-1-vipinsh@google.com> <YwAC1f5wTYpTdeh+@google.com>
In-Reply-To: <YwAC1f5wTYpTdeh+@google.com>
From:   Vipin Sharma <vipinsh@google.com>
Date:   Fri, 19 Aug 2022 15:20:06 -0700
Message-ID: <CAHVum0ecr7S9QS4+3kS3Yd-eQJ5ZY_GicQWurVFnAif6oOYhOg@mail.gmail.com>
Subject: Re: [PATCH v2] KVM: selftests: Run dirty_log_perf_test on specific cpus
To:     David Matlack <dmatlack@google.com>
Cc:     seanjc@google.com, pbonzini@redhat.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Aug 19, 2022 at 2:38 PM David Matlack <dmatlack@google.com> wrote:
>
> On Fri, Aug 19, 2022 at 02:07:37PM -0700, Vipin Sharma wrote:
> > +static int atoi_paranoid(const char *num_str)
> > +{
> > +     int num;
> > +     char *end_ptr;
> > +
> > +     errno = 0;
> > +     num = (int)strtol(num_str, &end_ptr, 10);
> > +     TEST_ASSERT(errno == 0, "Conversion error: %d\n", errno);
> > +     TEST_ASSERT(num_str != end_ptr && *end_ptr == '\0',
> > +                 "Invalid number string.\n");
> > +
> > +     return num;
> > +}
>
> Introduce atoi_paranoid() and upgrade existing atoi() users in a
> separate commit. Also please put it in e.g. test_util.c so that it can
> be used by other tests (and consider upgrading other tests to use it in
> your commit).
>

Sure, I will add a separate commit.


> > -     while ((opt = getopt(argc, argv, "eghi:p:m:nb:f:v:os:x:")) != -1) {
> > +     while ((opt = getopt(argc, argv, "c:eghi:p:m:nb:f:v:os:x:")) != -1) {
> >               switch (opt) {
> > +             case 'c':
> > +                     nr_lcpus = parse_cpu_list(optarg, lcpu_list, KVM_MAX_VCPUS + 1);
>
> I think we should move all the logic to pin threads to perf_test_util.c.
> The only thing dirty_log_perf_test.c should do is pass optarg into
> perf_test_util.c. This will make it trivial for any other test based on
> pef_test_util.c to also use pinning.
>
> e.g. All a test needs to do to use pinning is add a flag to the optlist
> and add a case statement like:
>
>         case 'c':
>                 perf_test_setup_pinning(optarg);
>                 break;
>
> perf_test_setup_pinning() would:
>  - Parse the list and populate perf_test_vcpu_args with each vCPU's
>    assigned pCPU.
>  - Pin the current thread to it's assigned pCPU if one is provided.
>

This will assume all tests have the same pinning requirement and
format. What if some tests have more than one worker threads after the
vcpus?

Maybe I should:
1. Create a generic function which parses a csv of integers, put it in
test_util.c
2. Provide a function to populate perf_test_vcpus_args in perf_test_util.c
3. Provide a function to migrate self to some cpu in perf_test_util.c.
This will work for now, but in future if there are more than 1 worker
we need to revisit it.

I will also be fine implementing what you suggested and keep working
under the precondition that there will be only one worker thread, if
that is okay to assume.

> Validating that the number of pCPUs == number of vCPUs is a little
> tricky. But that could be done as part of
> perf_test_start_vcpu_threads(). Alternatively, you could set up pinning
> after getting the number of vCPUs. e.g.
>
>         const char *cpu_list = NULL;
>
>         ...
>
>         while ((opt = getopt(...)) != -1) {
>                 switch (opt) {
>                 case 'c':
>                         cpu_list = optarg;  // is grabbing optarg here safe?

I am not sure how it is internally implemented. API doesn't mention
anything. Better to be safe and assume it is not valid later.

>                         break;
>                 }
>                 ...
>         }
>
>         if (cpu_list)
>                 perf_test_setup_pinning(cpu_list, nr_vcpus);
>

Better to have a copy of the argument or just get list of cpus after
parsing and then do the verification. What is the point in doing all
extra parsing work when we are gonna abort the process due to some
invalid condition.