On Tuesday, October 11, 2022 6:06 AM, Vipin Sharma wrote:
> Pin vCPUs to a host physical CPUs (pCPUs) in dirty_log_perf_test and optionally
> pin the main application thread to a physical cpu if provided. All tests based on
> perf_test_util framework can take advantage of it if needed.
>
> While at it, I changed atoi() to atoi_paranoid(), atoi_positive,
> atoi_non_negative() in other tests, sorted command line options alphabetically
> in dirty_log_perf_test, and added break between -e and -g which was missed in
> original commit when -e was introduced.
Just curious why not re-using the existing tools (e.g. taskset) to do the pinning?
For example, with below changes:
diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c b/tools/testing/se lftests/kvm/lib/perf_test_util.c
index 9618b37c66f7..aac58d1acb3c 100644
--- a/tools/testing/selftests/kvm/lib/perf_test_util.c
+++ b/tools/testing/selftests/kvm/lib/perf_test_util.c
@@ -264,6 +264,7 @@ void perf_test_start_vcpu_threads(int nr_vcpus,
void (*vcpu_fn)(struct perf_test_vcpu_args *))
{
int i;
+ char vcpu_name[5];
vcpu_thread_fn = vcpu_fn;
WRITE_ONCE(all_vcpu_threads_running, false);
@@ -275,6 +276,8 @@ void perf_test_start_vcpu_threads(int nr_vcpus,
WRITE_ONCE(vcpu->running, false);
pthread_create(&vcpu->thread, NULL, vcpu_thread_main, vcpu);
+ sprintf(vcpu_name, "%s%d", "vcpu", i);
+ pthread_setname_np(vcpu->thread, vcpu_name);
}
and with top we can get
PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND
4464 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.97 dirty_log_perf_
4467 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.93 vcpu0
4469 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.93 vcpu2
4470 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.94 vcpu3
4468 root 20 0 4248684 4.0g 1628 R 99.7 26.2 0:50.93 vcpu1
On Fri, Oct 14, 2022, Wang, Wei W wrote:
> On Tuesday, October 11, 2022 6:06 AM, Vipin Sharma wrote:
> > Pin vCPUs to a host physical CPUs (pCPUs) in dirty_log_perf_test and optionally
> > pin the main application thread to a physical cpu if provided. All tests based on
> > perf_test_util framework can take advantage of it if needed.
> >
> > While at it, I changed atoi() to atoi_paranoid(), atoi_positive,
> > atoi_non_negative() in other tests, sorted command line options alphabetically
> > in dirty_log_perf_test, and added break between -e and -g which was missed in
> > original commit when -e was introduced.
>
> Just curious why not re-using the existing tools (e.g. taskset) to do the pinning?
IIUC, you're suggesting the test give tasks meaningful names so that the user can
do taskset on the appropriate tasks? The goal is to ensure vCPUs are pinned before
they do any meaningful work. I don't see how that can be accomplished with taskset
without some form of hook in the test to effectively pause the test until the user
(or some run script) is ready to continue.
Pinning aside, naming the threads is a great idea! That would definitely help
debug, e.g. if one vCPU gets stuck or is lagging behind.
>
> For example, with below changes:
> diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c b/tools/testing/se lftests/kvm/lib/perf_test_util.c
> index 9618b37c66f7..aac58d1acb3c 100644
> --- a/tools/testing/selftests/kvm/lib/perf_test_util.c
> +++ b/tools/testing/selftests/kvm/lib/perf_test_util.c
> @@ -264,6 +264,7 @@ void perf_test_start_vcpu_threads(int nr_vcpus,
> void (*vcpu_fn)(struct perf_test_vcpu_args *))
> {
> int i;
> + char vcpu_name[5];
>
> vcpu_thread_fn = vcpu_fn;
> WRITE_ONCE(all_vcpu_threads_running, false);
> @@ -275,6 +276,8 @@ void perf_test_start_vcpu_threads(int nr_vcpus,
> WRITE_ONCE(vcpu->running, false);
>
> pthread_create(&vcpu->thread, NULL, vcpu_thread_main, vcpu);
> + sprintf(vcpu_name, "%s%d", "vcpu", i);
> + pthread_setname_np(vcpu->thread, vcpu_name);
> }
>
> and with top we can get
> PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND
> 4464 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.97 dirty_log_perf_
> 4467 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.93 vcpu0
> 4469 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.93 vcpu2
> 4470 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.94 vcpu3
> 4468 root 20 0 4248684 4.0g 1628 R 99.7 26.2 0:50.93 vcpu1
On Fri, Oct 14, 2022 at 9:34 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Fri, Oct 14, 2022, Wang, Wei W wrote:
> > On Tuesday, October 11, 2022 6:06 AM, Vipin Sharma wrote:
> > > Pin vCPUs to a host physical CPUs (pCPUs) in dirty_log_perf_test and optionally
> > > pin the main application thread to a physical cpu if provided. All tests based on
> > > perf_test_util framework can take advantage of it if needed.
> > >
> > > While at it, I changed atoi() to atoi_paranoid(), atoi_positive,
> > > atoi_non_negative() in other tests, sorted command line options alphabetically
> > > in dirty_log_perf_test, and added break between -e and -g which was missed in
> > > original commit when -e was introduced.
> >
> > Just curious why not re-using the existing tools (e.g. taskset) to do the pinning?
>
> IIUC, you're suggesting the test give tasks meaningful names so that the user can
> do taskset on the appropriate tasks? The goal is to ensure vCPUs are pinned before
> they do any meaningful work. I don't see how that can be accomplished with taskset
> without some form of hook in the test to effectively pause the test until the user
> (or some run script) is ready to continue.
A taskset approach would also be more difficult to incorporate into
automated runs of dirty_log_perf_test.
>
> Pinning aside, naming the threads is a great idea! That would definitely help
> debug, e.g. if one vCPU gets stuck or is lagging behind.
+1
>
> >
> > For example, with below changes:
> > diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c b/tools/testing/se lftests/kvm/lib/perf_test_util.c
> > index 9618b37c66f7..aac58d1acb3c 100644
> > --- a/tools/testing/selftests/kvm/lib/perf_test_util.c
> > +++ b/tools/testing/selftests/kvm/lib/perf_test_util.c
> > @@ -264,6 +264,7 @@ void perf_test_start_vcpu_threads(int nr_vcpus,
> > void (*vcpu_fn)(struct perf_test_vcpu_args *))
> > {
> > int i;
> > + char vcpu_name[5];
> >
> > vcpu_thread_fn = vcpu_fn;
> > WRITE_ONCE(all_vcpu_threads_running, false);
> > @@ -275,6 +276,8 @@ void perf_test_start_vcpu_threads(int nr_vcpus,
> > WRITE_ONCE(vcpu->running, false);
> >
> > pthread_create(&vcpu->thread, NULL, vcpu_thread_main, vcpu);
> > + sprintf(vcpu_name, "%s%d", "vcpu", i);
> > + pthread_setname_np(vcpu->thread, vcpu_name);
> > }
> >
> > and with top we can get
> > PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND
> > 4464 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.97 dirty_log_perf_
> > 4467 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.93 vcpu0
> > 4469 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.93 vcpu2
> > 4470 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.94 vcpu3
> > 4468 root 20 0 4248684 4.0g 1628 R 99.7 26.2 0:50.93 vcpu1
On Fri, Oct 14, 2022 at 9:55 AM David Matlack <dmatlack@google.com> wrote: > > On Fri, Oct 14, 2022 at 9:34 AM Sean Christopherson <seanjc@google.com> wrote: > > > > On Fri, Oct 14, 2022, Wang, Wei W wrote: > > > On Tuesday, October 11, 2022 6:06 AM, Vipin Sharma wrote: > > > > Pin vCPUs to a host physical CPUs (pCPUs) in dirty_log_perf_test and optionally > > > > pin the main application thread to a physical cpu if provided. All tests based on > > > > perf_test_util framework can take advantage of it if needed. > > > > > > > > While at it, I changed atoi() to atoi_paranoid(), atoi_positive, > > > > atoi_non_negative() in other tests, sorted command line options alphabetically > > > > in dirty_log_perf_test, and added break between -e and -g which was missed in > > > > original commit when -e was introduced. > > > > > > Just curious why not re-using the existing tools (e.g. taskset) to do the pinning? > > > > IIUC, you're suggesting the test give tasks meaningful names so that the user can > > do taskset on the appropriate tasks? The goal is to ensure vCPUs are pinned before > > they do any meaningful work. I don't see how that can be accomplished with taskset > > without some form of hook in the test to effectively pause the test until the user > > (or some run script) is ready to continue. > > A taskset approach would also be more difficult to incorporate into > automated runs of dirty_log_perf_test. > > > > > Pinning aside, naming the threads is a great idea! That would definitely help > > debug, e.g. if one vCPU gets stuck or is lagging behind. > > +1 I also like the idea. Sean: Do you want a v6 with the naming patch or you will be fine taking v5, if there are no changes needed in v5, and I can send a separate patch for naming?
On Fri, Oct 14, 2022, Vipin Sharma wrote: > On Fri, Oct 14, 2022 at 9:55 AM David Matlack <dmatlack@google.com> wrote: > > > > On Fri, Oct 14, 2022 at 9:34 AM Sean Christopherson <seanjc@google.com> wrote: > > > > > > On Fri, Oct 14, 2022, Wang, Wei W wrote: > > > > Just curious why not re-using the existing tools (e.g. taskset) to do the pinning? > > > > > > IIUC, you're suggesting the test give tasks meaningful names so that the user can > > > do taskset on the appropriate tasks? The goal is to ensure vCPUs are pinned before > > > they do any meaningful work. I don't see how that can be accomplished with taskset > > > without some form of hook in the test to effectively pause the test until the user > > > (or some run script) is ready to continue. > > > > A taskset approach would also be more difficult to incorporate into > > automated runs of dirty_log_perf_test. > > > > > > > > Pinning aside, naming the threads is a great idea! That would definitely help > > > debug, e.g. if one vCPU gets stuck or is lagging behind. > > > > +1 > > I also like the idea. > > Sean: > Do you want a v6 with the naming patch or you will be fine taking v5, > if there are no changes needed in v5, and I can send a separate patch > for naming? Definitely separate, this is an orthogonal change and I don't think there will be any conflict. If there is a conflict, it will be trivial to resolve. But since Wei provided a more or less complete patch, let's let Wei post a formal patch (unless he doesn't want to).
On Saturday, October 15, 2022 3:03 AM, Sean Christopherson wrote: > On Fri, Oct 14, 2022, Vipin Sharma wrote: > > On Fri, Oct 14, 2022 at 9:55 AM David Matlack <dmatlack@google.com> > wrote: > > > > > > On Fri, Oct 14, 2022 at 9:34 AM Sean Christopherson <seanjc@google.com> > wrote: > > > > > > > > On Fri, Oct 14, 2022, Wang, Wei W wrote: > > > > > Just curious why not re-using the existing tools (e.g. taskset) to do the > pinning? > > > > > > > > IIUC, you're suggesting the test give tasks meaningful names so > > > > that the user can do taskset on the appropriate tasks? The goal > > > > is to ensure vCPUs are pinned before they do any meaningful work. > > > > I don't see how that can be accomplished with taskset without some > > > > form of hook in the test to effectively pause the test until the user (or > some run script) is ready to continue. > > > > > > A taskset approach would also be more difficult to incorporate into > > > automated runs of dirty_log_perf_test. > > > > > > > > > > > Pinning aside, naming the threads is a great idea! That would > > > > definitely help debug, e.g. if one vCPU gets stuck or is lagging behind. > > > > > > +1 > > > > I also like the idea. > > > > Sean: > > Do you want a v6 with the naming patch or you will be fine taking v5, > > if there are no changes needed in v5, and I can send a separate patch > > for naming? > > Definitely separate, this is an orthogonal change and I don't think there will be > any conflict. If there is a conflict, it will be trivial to resolve. But since Wei > provided a more or less complete patch, let's let Wei post a formal patch > (unless he doesn't want to). Yeah, I'm glad to take care of this. There are other places (e.g. hardware_disable_test.c) that seem good to have this as well, I'll cover them in one patchset.
On Fri, Oct 14, 2022 at 12:03 PM Sean Christopherson <seanjc@google.com> wrote: > > On Fri, Oct 14, 2022, Vipin Sharma wrote: > > On Fri, Oct 14, 2022 at 9:55 AM David Matlack <dmatlack@google.com> wrote: > > > > > > On Fri, Oct 14, 2022 at 9:34 AM Sean Christopherson <seanjc@google.com> wrote: > > > > > > > > On Fri, Oct 14, 2022, Wang, Wei W wrote: > > > > > Just curious why not re-using the existing tools (e.g. taskset) to do the pinning? > > > > > > > > IIUC, you're suggesting the test give tasks meaningful names so that the user can > > > > do taskset on the appropriate tasks? The goal is to ensure vCPUs are pinned before > > > > they do any meaningful work. I don't see how that can be accomplished with taskset > > > > without some form of hook in the test to effectively pause the test until the user > > > > (or some run script) is ready to continue. > > > > > > A taskset approach would also be more difficult to incorporate into > > > automated runs of dirty_log_perf_test. > > > > > > > > > > > Pinning aside, naming the threads is a great idea! That would definitely help > > > > debug, e.g. if one vCPU gets stuck or is lagging behind. > > > > > > +1 > > > > I also like the idea. > > > > Sean: > > Do you want a v6 with the naming patch or you will be fine taking v5, > > if there are no changes needed in v5, and I can send a separate patch > > for naming? > > Definitely separate, this is an orthogonal change and I don't think there will be > any conflict. If there is a conflict, it will be trivial to resolve. But since > Wei provided a more or less complete patch, let's let Wei post a formal patch > (unless he doesn't want to). Sounds good!
On Friday, October 14, 2022 11:12 PM, Wang, Wei W wrote:
> On Tuesday, October 11, 2022 6:06 AM, Vipin Sharma wrote:
> > Pin vCPUs to a host physical CPUs (pCPUs) in dirty_log_perf_test and
> > optionally pin the main application thread to a physical cpu if
> > provided. All tests based on perf_test_util framework can take advantage of
> it if needed.
> >
> > While at it, I changed atoi() to atoi_paranoid(), atoi_positive,
> > atoi_non_negative() in other tests, sorted command line options
> > alphabetically in dirty_log_perf_test, and added break between -e and
> > -g which was missed in original commit when -e was introduced.
>
> Just curious why not re-using the existing tools (e.g. taskset) to do the pinning?
>
> For example, with below changes:
> diff --git a/tools/testing/selftests/kvm/lib/perf_test_util.c b/tools/testing/se
> lftests/kvm/lib/perf_test_util.c
> index 9618b37c66f7..aac58d1acb3c 100644
> --- a/tools/testing/selftests/kvm/lib/perf_test_util.c
> +++ b/tools/testing/selftests/kvm/lib/perf_test_util.c
> @@ -264,6 +264,7 @@ void perf_test_start_vcpu_threads(int nr_vcpus,
> void (*vcpu_fn)(struct
> perf_test_vcpu_args *)) {
> int i;
> + char vcpu_name[5];
Typo, should be "vcpu_name[6]" here.
© 2016 - 2026 Red Hat, Inc.