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;
Date:   Fri, 19 Aug 2022 15:34:01 -0700
From:   David Matlack <dmatlack@google.com>
To:     Vipin Sharma <vipinsh@google.com>
Cc:     seanjc@google.com, pbonzini@redhat.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] KVM: selftests: Run dirty_log_perf_test on specific
 cpus
Message-ID: <YwAP2dM/9vfjlAMb@google.com>
References: <20220819210737.763135-1-vipinsh@google.com>
 <YwAC1f5wTYpTdeh+@google.com>
 <CAHVum0ecr7S9QS4+3kS3Yd-eQJ5ZY_GicQWurVFnAif6oOYhOg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHVum0ecr7S9QS4+3kS3Yd-eQJ5ZY_GicQWurVFnAif6oOYhOg@mail.gmail.com>
Precedence: bulk

On Fri, Aug 19, 2022 at 03:20:06PM -0700, Vipin Sharma wrote:
> 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?

Even if a test has other worker threads, this proposal would still be
logically consistent. The flag is defined to only control the vCPU
threads and the main threads. If some test has some other threads
besides that, this flag will not affect them (which is exactly how it's
defined to behave). If such a test wants to pin those other threads, it
would make sense to have a test-specific flag for that pinning (and we
can figure out the right way to do that if/when we encounter that
situation).

> 
> 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.

I think it should be fine. I assume optarg is pointing into [a copy of?]
argv with null characters inserted, so the pointer will still be valid
after the loop. But I just wanted to call out that I wasn't 100% sure :)

> 
> >                         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.

Yeah and I also realized perf_test_setup_pinning() will need to know how
many vCPUs there are so it can determine which element is the main
thread's pCPU assignment.