| Message ID | 20221010220538.1154054-1-vipinsh@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | dirty_log_perf_test vCPU pinning | expand |
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 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.
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 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 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 Mon, Oct 10, 2022, 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. ... > Vipin Sharma (5): > KVM: selftests: Add missing break between -e and -g option in > dirty_log_perf_test > KVM: selftests: Put command line options in alphabetical order in > dirty_log_perf_test > KVM: selftests: Add atoi_paranoid() to catch errors missed by atoi() > KVM: selftests: Add atoi_positive() and atoi_non_negative() for input > validation > KVM: selftests: Run dirty_log_perf_test on specific CPUs Minor nits on patch 5, but otherwise looks good. Might be worth sending a v6 just so there's no confusion.
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. v5: - Added atoi_postive() and atoi_non_negative() APIs for string parsing. - Using sched_getaffinity() to verify if a pCPU is allowed or not. - Changed Suggested-by to add only person came up with original idea of pinning. - Updated strings and commit messages. v4: https://lore.kernel.org/lkml/20221006171133.372359-1-vipinsh@google.com/ - Moved boolean to check vCPUs pinning from perf_test_vcpu_args to perf_test_args. - Changed assert statements to make error more descriptive. - Modified break statement between 'e' and 'g' option in v3 by not copying dirty_log_manual_caps = 0 to 'e'. v3: https://lore.kernel.org/lkml/20220826184500.1940077-1-vipinsh@google.com - Moved atoi_paranoid() to test_util.c and replaced all atoi() usage with atoi_paranoid() - Sorted command line options alphabetically. - Instead of creating a vcpu thread on a specific pcpu the thread will migrate to the provided pcpu after its creation. - Decoupled -e and -g option. v2: https://lore.kernel.org/lkml/20220819210737.763135-1-vipinsh@google.com/ - Removed -d option. - One cpu list passed as option, cpus for vcpus, followed by application thread cpu. - Added paranoid cousin of atoi(). v1: https://lore.kernel.org/lkml/20220817152956.4056410-1-vipinsh@google.com Vipin Sharma (5): KVM: selftests: Add missing break between -e and -g option in dirty_log_perf_test KVM: selftests: Put command line options in alphabetical order in dirty_log_perf_test KVM: selftests: Add atoi_paranoid() to catch errors missed by atoi() KVM: selftests: Add atoi_positive() and atoi_non_negative() for input validation KVM: selftests: Run dirty_log_perf_test on specific CPUs .../selftests/kvm/aarch64/arch_timer.c | 25 ++----- .../testing/selftests/kvm/aarch64/vgic_irq.c | 6 +- .../selftests/kvm/access_tracking_perf_test.c | 2 +- .../selftests/kvm/demand_paging_test.c | 4 +- .../selftests/kvm/dirty_log_perf_test.c | 62 ++++++++++++------ .../selftests/kvm/include/perf_test_util.h | 6 ++ .../testing/selftests/kvm/include/test_util.h | 4 ++ .../selftests/kvm/kvm_page_table_test.c | 4 +- .../selftests/kvm/lib/perf_test_util.c | 65 ++++++++++++++++++- tools/testing/selftests/kvm/lib/test_util.c | 35 ++++++++++ .../selftests/kvm/max_guest_memory_test.c | 7 +- .../kvm/memslot_modification_stress_test.c | 6 +- .../testing/selftests/kvm/memslot_perf_test.c | 24 ++----- .../selftests/kvm/set_memory_region_test.c | 2 +- .../selftests/kvm/x86_64/nx_huge_pages_test.c | 4 +- 15 files changed, 178 insertions(+), 78 deletions(-)