| Message ID | 20220812175301.3915004-1-oliver.upton@linux.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: selftests: Use TEST_REQUIRE() in nx_huge_pages_test | expand |
On Fri, Aug 12, 2022 at 05:53:01PM +0000, Oliver Upton wrote: > Avoid boilerplate for checking test preconditions by using > TEST_REQUIRE(). While at it, add a precondition for > KVM_CAP_VM_DISABLE_NX_HUGE_PAGES to skip (instead of silently pass) on > older kernels. > > Signed-off-by: Oliver Upton <oliver.upton@linux.dev> > --- > .../selftests/kvm/x86_64/nx_huge_pages_test.c | 24 +++++-------------- > 1 file changed, 6 insertions(+), 18 deletions(-) > > diff --git a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c > index cc6421716400..e19933ea34ca 100644 > --- a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c > +++ b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c > @@ -118,13 +118,6 @@ void run_test(int reclaim_period_ms, bool disable_nx_huge_pages, > vm = vm_create(1); > > if (disable_nx_huge_pages) { > - /* > - * Cannot run the test without NX huge pages if the kernel > - * does not support it. > - */ > - if (!kvm_check_cap(KVM_CAP_VM_DISABLE_NX_HUGE_PAGES)) > - return; > - > r = __vm_disable_nx_huge_pages(vm); > if (reboot_permissions) { > TEST_ASSERT(!r, "Disabling NX huge pages should succeed if process has reboot permissions"); > @@ -248,18 +241,13 @@ int main(int argc, char **argv) > } > } > > - if (token != MAGIC_TOKEN) { > - print_skip("This test must be run with the magic token %d.\n" > - "This is done by nx_huge_pages_test.sh, which\n" > - "also handles environment setup for the test.", > - MAGIC_TOKEN); > - exit(KSFT_SKIP); > - } > + TEST_REQUIRE(kvm_has_cap(KVM_CAP_VM_DISABLE_NX_HUGE_PAGES)); This cap is only needed for run_test(..., true, ...) below so I don't think we should require it for the entire test. That being said, it still might be good to inform the user that the test is being skipped. So perhaps something like this: ... run_test(reclaim_period_ms, false, reboot_permissions); if (kvm_has_cap(KVM_CAP_VM_DISABLE_NX_HUGE_PAGES)) run_test(reclaim_period_ms, true, reboot_permissions); else print_skip("KVM_CAP_VM_DISABLE_NX_HUGE_PAGES not supported"); ... > + TEST_REQUIRE(reclaim_period_ms > 0); > > - if (!reclaim_period_ms) { > - print_skip("The NX reclaim period must be specified and non-zero"); > - exit(KSFT_SKIP); > - } > + __TEST_REQUIRE(token == MAGIC_TOKEN, > + "This test must be run with the magic token %d.\n" > + "This is done by nx_huge_pages_test.sh, which\n" > + "also handles environment setup for the test."); > > run_test(reclaim_period_ms, false, reboot_permissions); > run_test(reclaim_period_ms, true, reboot_permissions); > > base-commit: 93472b79715378a2386598d6632c654a2223267b > -- > 2.37.1.595.g718a3a8f04-goog >
On Fri, Aug 12, 2022 at 11:04:25AM -0700, David Matlack wrote: > On Fri, Aug 12, 2022 at 05:53:01PM +0000, Oliver Upton wrote: > > Avoid boilerplate for checking test preconditions by using > > TEST_REQUIRE(). While at it, add a precondition for > > KVM_CAP_VM_DISABLE_NX_HUGE_PAGES to skip (instead of silently pass) on > > older kernels. > > > > Signed-off-by: Oliver Upton <oliver.upton@linux.dev> > > --- > > .../selftests/kvm/x86_64/nx_huge_pages_test.c | 24 +++++-------------- > > 1 file changed, 6 insertions(+), 18 deletions(-) > > > > diff --git a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c > > index cc6421716400..e19933ea34ca 100644 > > --- a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c > > +++ b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c > > @@ -118,13 +118,6 @@ void run_test(int reclaim_period_ms, bool disable_nx_huge_pages, > > vm = vm_create(1); > > > > if (disable_nx_huge_pages) { > > - /* > > - * Cannot run the test without NX huge pages if the kernel > > - * does not support it. > > - */ > > - if (!kvm_check_cap(KVM_CAP_VM_DISABLE_NX_HUGE_PAGES)) > > - return; > > - > > r = __vm_disable_nx_huge_pages(vm); > > if (reboot_permissions) { > > TEST_ASSERT(!r, "Disabling NX huge pages should succeed if process has reboot permissions"); > > @@ -248,18 +241,13 @@ int main(int argc, char **argv) > > } > > } > > > > - if (token != MAGIC_TOKEN) { > > - print_skip("This test must be run with the magic token %d.\n" > > - "This is done by nx_huge_pages_test.sh, which\n" > > - "also handles environment setup for the test.", > > - MAGIC_TOKEN); > > - exit(KSFT_SKIP); > > - } > > + TEST_REQUIRE(kvm_has_cap(KVM_CAP_VM_DISABLE_NX_HUGE_PAGES)); > > This cap is only needed for run_test(..., true, ...) below so I don't think we should require it for the entire test. It has always seemed that the test preconditions are a way to pretty-print a failure/skip instead of having some random ioctl fail deeper in the test. If we really see value in adding predicates for individual test cases then IMO it deserves first-class support in our framework. Otherwise the next test that comes along is bound to open-code the same thing. Can't folks just update their kernel? :-) -- Thanks, Oliver > That being said, it still might be good to inform the user that the test is being skipped. So perhaps something like this: > > ... > run_test(reclaim_period_ms, false, reboot_permissions); > > if (kvm_has_cap(KVM_CAP_VM_DISABLE_NX_HUGE_PAGES)) > run_test(reclaim_period_ms, true, reboot_permissions); > else > print_skip("KVM_CAP_VM_DISABLE_NX_HUGE_PAGES not supported"); > ... > > > + TEST_REQUIRE(reclaim_period_ms > 0); > > > > - if (!reclaim_period_ms) { > > - print_skip("The NX reclaim period must be specified and non-zero"); > > - exit(KSFT_SKIP); > > - } > > + __TEST_REQUIRE(token == MAGIC_TOKEN, > > + "This test must be run with the magic token %d.\n" > > + "This is done by nx_huge_pages_test.sh, which\n" > > + "also handles environment setup for the test."); > > > > run_test(reclaim_period_ms, false, reboot_permissions); > > run_test(reclaim_period_ms, true, reboot_permissions); > > > > base-commit: 93472b79715378a2386598d6632c654a2223267b > > -- > > 2.37.1.595.g718a3a8f04-goog > >
On Fri, Aug 12, 2022 at 07:16:49PM +0000, Oliver Upton wrote: > On Fri, Aug 12, 2022 at 11:04:25AM -0700, David Matlack wrote: > > On Fri, Aug 12, 2022 at 05:53:01PM +0000, Oliver Upton wrote: > > > Avoid boilerplate for checking test preconditions by using > > > TEST_REQUIRE(). While at it, add a precondition for > > > KVM_CAP_VM_DISABLE_NX_HUGE_PAGES to skip (instead of silently pass) on > > > older kernels. > > > > > > Signed-off-by: Oliver Upton <oliver.upton@linux.dev> > > > --- > > > .../selftests/kvm/x86_64/nx_huge_pages_test.c | 24 +++++-------------- > > > 1 file changed, 6 insertions(+), 18 deletions(-) > > > > > > diff --git a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c > > > index cc6421716400..e19933ea34ca 100644 > > > --- a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c > > > +++ b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c > > > @@ -118,13 +118,6 @@ void run_test(int reclaim_period_ms, bool disable_nx_huge_pages, > > > vm = vm_create(1); > > > > > > if (disable_nx_huge_pages) { > > > - /* > > > - * Cannot run the test without NX huge pages if the kernel > > > - * does not support it. > > > - */ > > > - if (!kvm_check_cap(KVM_CAP_VM_DISABLE_NX_HUGE_PAGES)) > > > - return; > > > - > > > r = __vm_disable_nx_huge_pages(vm); > > > if (reboot_permissions) { > > > TEST_ASSERT(!r, "Disabling NX huge pages should succeed if process has reboot permissions"); > > > @@ -248,18 +241,13 @@ int main(int argc, char **argv) > > > } > > > } > > > > > > - if (token != MAGIC_TOKEN) { > > > - print_skip("This test must be run with the magic token %d.\n" > > > - "This is done by nx_huge_pages_test.sh, which\n" > > > - "also handles environment setup for the test.", > > > - MAGIC_TOKEN); > > > - exit(KSFT_SKIP); > > > - } > > > + TEST_REQUIRE(kvm_has_cap(KVM_CAP_VM_DISABLE_NX_HUGE_PAGES)); > > > > This cap is only needed for run_test(..., true, ...) below so I don't think we should require it for the entire test. > > It has always seemed that the test preconditions are a way to pretty-print > a failure/skip instead of having some random ioctl fail deeper in the > test. > > If we really see value in adding predicates for individual test cases > then IMO it deserves first-class support in our framework. Otherwise > the next test that comes along is bound to open-code the same thing. Fair point. > > Can't folks just update their kernel? :-) Consider my suggestion optional. If anyone is backporting this test to their kernel they'll also probably backport KVM_CAP_VM_DISABLE_NX_HUGE_PAGES ;). So I don't think there will be a huge benefit of making the test more flexible. That aside, Reviewed-by: David Matlack <dmatlack@google.com> > > -- > Thanks, > Oliver > > > That being said, it still might be good to inform the user that the test is being skipped. So perhaps something like this: > > > > ... > > run_test(reclaim_period_ms, false, reboot_permissions); > > > > if (kvm_has_cap(KVM_CAP_VM_DISABLE_NX_HUGE_PAGES)) > > run_test(reclaim_period_ms, true, reboot_permissions); > > else > > print_skip("KVM_CAP_VM_DISABLE_NX_HUGE_PAGES not supported"); > > ... > > > > > + TEST_REQUIRE(reclaim_period_ms > 0); > > > > > > - if (!reclaim_period_ms) { > > > - print_skip("The NX reclaim period must be specified and non-zero"); > > > - exit(KSFT_SKIP); > > > - } > > > + __TEST_REQUIRE(token == MAGIC_TOKEN, > > > + "This test must be run with the magic token %d.\n" > > > + "This is done by nx_huge_pages_test.sh, which\n" > > > + "also handles environment setup for the test."); > > > > > > run_test(reclaim_period_ms, false, reboot_permissions); > > > run_test(reclaim_period_ms, true, reboot_permissions); > > > > > > base-commit: 93472b79715378a2386598d6632c654a2223267b > > > -- > > > 2.37.1.595.g718a3a8f04-goog > > >
On Mon, Aug 15, 2022, David Matlack wrote: > On Fri, Aug 12, 2022 at 07:16:49PM +0000, Oliver Upton wrote: > > On Fri, Aug 12, 2022 at 11:04:25AM -0700, David Matlack wrote: > > > On Fri, Aug 12, 2022 at 05:53:01PM +0000, Oliver Upton wrote: > > > > Avoid boilerplate for checking test preconditions by using > > > > TEST_REQUIRE(). While at it, add a precondition for > > > > KVM_CAP_VM_DISABLE_NX_HUGE_PAGES to skip (instead of silently pass) on > > > > older kernels. ... > > > > + TEST_REQUIRE(kvm_has_cap(KVM_CAP_VM_DISABLE_NX_HUGE_PAGES)); > > > > > > This cap is only needed for run_test(..., true, ...) below so I don't think we should require it for the entire test. > > > > It has always seemed that the test preconditions are a way to pretty-print > > a failure/skip instead of having some random ioctl fail deeper in the > > test. > > > > If we really see value in adding predicates for individual test cases > > then IMO it deserves first-class support in our framework. Otherwise > > the next test that comes along is bound to open-code the same thing. > > Fair point. > > > > > Can't folks just update their kernel? :-) > > Consider my suggestion optional. If anyone is backporting this test to > their kernel they'll also probably backport > KVM_CAP_VM_DISABLE_NX_HUGE_PAGES ;). So I don't think there will be a > huge benefit of making the test more flexible. Yeah, I'm somewhat ambivalent as well. All things considered, I think it makes sense to skip the entire test. Like Oliver said, without first-class support, this will become a mess. And I'm not convinced that adding first-class support is a good idea, as that will inevitably lead to tests ballooning to include a big pile of subtests, a la KUT's VMX test. I would much rather steer selftests in a "one test per binary" direction; IMO it's easier to do filtering via scripts, and it also minimizes the chances of creating subtle dependencies between (sub)tests. So, pushed to branch `for_paolo/6.1` at: https://github.com/sean-jc/linux.git but with a rewritten shortlog+changelog to capture this conversation. And that's a good lesson for the future as well: when piggybacking a patch, making functional changes as the "opportunistic cleanup" is rarely the right thing to do. This is what I ended up with, holler if anything about it bothers you. KVM: selftests: Require DISABLE_NX_HUGE_PAGES cap for NX hugepage test Require KVM_CAP_VM_DISABLE_NX_HUGE_PAGES for the entire NX hugepage test instead of skipping the "disable" subtest if the capability isn't supported by the host kernel. While the "enable" subtest does provide value when the capability isn't supported, silently providing only half the promised coveraged is undesirable, i.e. it's better to skip the test so that the user knows something. Alternatively, the test could print something to alert the user instead of silently skipping the subtest, but that would encourage other tests to follow suit, and it's not clear that it's desirable to take selftests in that direction. And if selftests do head down the path of skipping subtests, such behavior needs first-class support in the framework. Opportunistically convert other test preconditions to TEST_REQUIRE(). Signed-off-by: Oliver Upton <oliver.upton@linux.dev> Reviewed-by: David Matlack <dmatlack@google.com> Link: https://lore.kernel.org/r/20220812175301.3915004-1-oliver.upton@linux.dev [sean: rewrote changelog to capture discussion about skipping the test] Signed-off-by: Sean Christopherson <seanjc@google.com>
diff --git a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c index cc6421716400..e19933ea34ca 100644 --- a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c +++ b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c @@ -118,13 +118,6 @@ void run_test(int reclaim_period_ms, bool disable_nx_huge_pages, vm = vm_create(1); if (disable_nx_huge_pages) { - /* - * Cannot run the test without NX huge pages if the kernel - * does not support it. - */ - if (!kvm_check_cap(KVM_CAP_VM_DISABLE_NX_HUGE_PAGES)) - return; - r = __vm_disable_nx_huge_pages(vm); if (reboot_permissions) { TEST_ASSERT(!r, "Disabling NX huge pages should succeed if process has reboot permissions"); @@ -248,18 +241,13 @@ int main(int argc, char **argv) } } - if (token != MAGIC_TOKEN) { - print_skip("This test must be run with the magic token %d.\n" - "This is done by nx_huge_pages_test.sh, which\n" - "also handles environment setup for the test.", - MAGIC_TOKEN); - exit(KSFT_SKIP); - } + TEST_REQUIRE(kvm_has_cap(KVM_CAP_VM_DISABLE_NX_HUGE_PAGES)); + TEST_REQUIRE(reclaim_period_ms > 0); - if (!reclaim_period_ms) { - print_skip("The NX reclaim period must be specified and non-zero"); - exit(KSFT_SKIP); - } + __TEST_REQUIRE(token == MAGIC_TOKEN, + "This test must be run with the magic token %d.\n" + "This is done by nx_huge_pages_test.sh, which\n" + "also handles environment setup for the test."); run_test(reclaim_period_ms, false, reboot_permissions); run_test(reclaim_period_ms, true, reboot_permissions);
Avoid boilerplate for checking test preconditions by using TEST_REQUIRE(). While at it, add a precondition for KVM_CAP_VM_DISABLE_NX_HUGE_PAGES to skip (instead of silently pass) on older kernels. Signed-off-by: Oliver Upton <oliver.upton@linux.dev> --- .../selftests/kvm/x86_64/nx_huge_pages_test.c | 24 +++++-------------- 1 file changed, 6 insertions(+), 18 deletions(-) base-commit: 93472b79715378a2386598d6632c654a2223267b