Re: [PATCH] KVM: selftests: Use TEST_REQUIRE() in nx_huge_pages_test

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux