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@xxxxxxxxx>
> > ---
> > .../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
> >
