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
