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 22:59:27 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     David Matlack <dmatlack@google.com>
Cc:     Vipin Sharma <vipinsh@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: <YwAVzzF2dZ2tKOUh@google.com>
References: <20220819210737.763135-1-vipinsh@google.com>
 <YwAC1f5wTYpTdeh+@google.com>
 <CAHVum0ecr7S9QS4+3kS3Yd-eQJ5ZY_GicQWurVFnAif6oOYhOg@mail.gmail.com>
 <YwAP2dM/9vfjlAMb@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YwAP2dM/9vfjlAMb@google.com>
Precedence: bulk

On Fri, Aug 19, 2022, David Matlack wrote:
> 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:
> > > 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).

...

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

The "how many workers you got?" conundrum can be solved in the same way, e.g. just
have the caller pass in the number of workers it will create.

	perf_test_setup_pinning(pin_string, nr_vcpus, NR_WORKERS);

The only question is what semantics we should support for workers, e.g. do we
want to force an all-or-none approach or can the user pin a subset.  All-or-none
seems like it'd be the simplest to maintain and understand.  I.e. if -c is used,
then all vCPUs must be pinned, and either all workers or no workers are pinned.