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

[Tao Su <tao1.su@linux.intel.com>]

On Thu, Jan 18, 2024 at 09:33:29AM -0800, Sean Christopherson wrote:
> +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()

Got it.

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

I see, I will update the ASSERT conditions, thanks for your guidance.

Thanks,
Tao