Re: [PATCH] KVM: selftests: Add a requirement for disabling numa balancing

[Sean Christopherson <seanjc@google.com>]

+David

On Thu, Jan 18, 2024, Tao Su wrote:
> On Wed, Jan 17, 2024 at 07:12:08AM -0800, Sean Christopherson wrote:
> 
> [...]
> 
> > > diff --git a/tools/testing/selftests/kvm/x86_64/dirty_log_page_splitting_test.c b/tools/testing/selftests/kvm/x86_64/dirty_log_page_splitting_test.c
> > > index 634c6bfcd572..f2c796111d83 100644
> > > --- a/tools/testing/selftests/kvm/x86_64/dirty_log_page_splitting_test.c
> > > +++ b/tools/testing/selftests/kvm/x86_64/dirty_log_page_splitting_test.c
> > > @@ -212,10 +212,21 @@ static void help(char *name)
> > >  
> > >  int main(int argc, char *argv[])
> > >  {
> > > +	FILE *f;
> > >  	int opt;
> > > +	int ret, numa_balancing;
> > >  
> > >  	TEST_REQUIRE(get_kvm_param_bool("eager_page_split"));
> > >  	TEST_REQUIRE(get_kvm_param_bool("tdp_mmu"));
> > > +	f = fopen("/proc/sys/kernel/numa_balancing", "r");
> > > +	if (f) {
> > > +		ret = fscanf(f, "%d", &numa_balancing);
> > > +		TEST_ASSERT(ret == 1, "Error reading numa_balancing");
> > > +		TEST_ASSERT(!numa_balancing, "please run "
> > > +			    "'echo 0 > /proc/sys/kernel/numa_balancing'");
> > 
> > If we go this route, this should be a TEST_REQUIRE(), not a TEST_ASSERT().  The
> > test hasn't failed, rather it has detected an incompatible setup.
> 
> Yes, previously I wanted to print a more user-friendly prompt, but TEST_REQUIRE()
> can’t customize the output…

__TEST_REQUIRE()

> > Something isn't right though.  The test defaults to HugeTLB, and the invocation
> > in the changelog doesn't override the backing source.  That suggests that NUMA
> > auto-balancing is zapping HugeTLB VMAs, which AFAIK shouldn't happen, e.g. this
> > code in task_numa_work() should cause such VMAs to be skipped:
> > 
> > 		if (!vma_migratable(vma) || !vma_policy_mof(vma) ||
> > 			is_vm_hugetlb_page(vma) || (vma->vm_flags & VM_MIXEDMAP)) {
> > 			trace_sched_skip_vma_numa(mm, vma, NUMAB_SKIP_UNSUITABLE);
> > 			continue;
> > 		}
> > 
> > And the test already warns the user if they opt to use something other than
> > HugeTLB.
> > 
> > 	if (!is_backing_src_hugetlb(backing_src)) {
> > 		pr_info("This test will only work reliably with HugeTLB memory. "
> > 			"It can work with THP, but that is best effort.\n");
> > 	}
> > 
> > If the test is defaulting to something other than HugeTLB, then we should fix
> > that in the test.  If the kernel is doing NUMA balancing on HugeTLB VMAs, then
> > we should fix that in the kernel.
> 
> HugeTLB VMAs are not affected by NUMA auto-balancing through my observation, but
> the backing sources of the test code and per-vCPU stacks are not Huge TLB, e.g.
> __vm_create() invokes
> 
>         vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, 0, 0, nr_pages, 0);
> 
> So, some pages are possible to be migrated.

Ah, hmm.  Requiring NUMA balancing be disabled isn't going to fix the underlying
issue, it's just guarding against one of the more likely culprits.  The best fix
is likely to have the test precisely validate _only_ the test data pages.  E.g.
if we double down on requiring HugeTLB, then the test should be able to assert
that it has at least N hugepages when dirty logging is disabled, and at least M
4KiB pages when dirty logging is enabled.