Re: [PATCH v2 4/4] KVM: selftests: use allowed NUMA nodes in guest_memfd_test

["Garg, Shivank" <shivankg@amd.com>]

On Tue, 2026-09-01 at 10:28 -0700, Sean Christopherson wrote:
> On Tue, Sep 01, 2026, Shivank Garg wrote:
> > guest_memfd_test assumes that nodes 0 and 1 exist and have memory.
> > is_multi_numa_node_system() only checks that the maximum node ID is
> > nonzero, which is not enough for sparse or memoryless nodes.
> > 
> > Select the required nodes from MPOL_F_MEMS_ALLOWED instead. Use the full
> > nodemask width plus one to mbind(), and let test_mbind() run when only
> > one memory node is available.
> 
> Please split this into at least three patches.
> 
>   1. Refactor guest_memfd_test.c to prepare for using nodes other than 0 and 1.
>   2. Fix test_mbind().
>   3. Fix test_numa_allocation().
>   4. If necessary, do additional cleanups in numaif.h
> 
> As is, this is extremely difficult to review, e.g. without staring intently, I
> can't tell what's refactoring and what's actually a functional change.
> 
Agreed, will split this patch.

> Actually, looking at the xAPIC IPI test more, what you proposed in patch 3 is in
> the general direction of what we want, but needs to be more than just a wrapper
> for get_mempolicy() to be useful.  Specificaly, if it fills the mask *and* returns
> the number of nodes found, then it's more generically useful.  And if we also add
> an API to get the next (exclusive) node, then we can cut down on the amount of
> copy+paste without forcing tests to use the "array of one-bit nodemasks" approach
> that the xAPIC test uses.
> 
> E.g. the below get_numa_node_ids() is basically just copy+paste from the xAPIC
> test, except that it returns an array of nodes instead of an array of nodemasks.
> The fact that you felt compelled to copy+paste instead of adding an API is quite
> telling: using an array of nodemasks/nodes is inflexible and really only works if
> a test wants to target exactly one node.  It's also annoying to extract to a generic
> API without ending up with a brittle API.  E.g. if the API where to take the a mask
> and an array, it would either have to be a macro or take a struct to ensure the
> array can hold all possible masks.
> 
> I'm planning on adding these in the series to also add MAXNODE_FOR_MASK().
> 
> static inline int kvm_get_numa_memory_nodes(unsigned long *nodemask)
> {
> 	int r;
> 
> 	*nodemask = 0;
> 
> 	r = get_mempolicy(NULL, nodemask, MAXNODE_FOR_MASK(*nodemask), 0,
> 			  MPOL_F_MEMS_ALLOWED);
> 	TEST_ASSERT(!r || errno == ENOSYS || errno == EPERM,
> 		    "Unexpected get_mempolicy() failure");
> 	return __builtin_popcountl(*nodemask);
> }
> 
> /*
>  * Return the node ID of the next NUMA node in the mask, starting at @from+1.
>  * Guarantees a node is found, and that the found node is not @from.  Pass -1
>  * to find the first node in the mask.
>  */
> static inline int kvm_get_next_numa_node(unsigned long nodemask, int from)
> {
> 	const unsigned long nr_bits = BITS_PER_TYPE(nodemask);
> 	int to;
> 
> 	to = find_next_bit(&nodemask, nr_bits, from + 1);
> 	if (to == nr_bits)
> 		to = find_next_bit(&nodemask, nr_bits, 0);
> 
> 	TEST_ASSERT(to != nr_bits && to != from,
> 		    "Unabled to find second NUMA node (from = %d, to = %d)", from, to);
> 	return to;
> }
> 
> > The sysfs helpers for finding maxnode are no longer needed.
> 
> This is an observation, not a proper changelog sentence.
> 
> > Signed-off-by: Shivank Garg <shivankg@amd.com>
> > ---
> >  tools/testing/selftests/kvm/guest_memfd_test.c | 86 +++++++++++++++++---------
> >  tools/testing/selftests/kvm/include/numaif.h   | 52 ----------------
> >  2 files changed, 58 insertions(+), 80 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/kvm/guest_memfd_test.c b/tools/testing/selftests/kvm/guest_memfd_test.c
> > index 2233d871a38f..aee80dda6229 100644
> > --- a/tools/testing/selftests/kvm/guest_memfd_test.c
> > +++ b/tools/testing/selftests/kvm/guest_memfd_test.c
> > @@ -76,33 +76,53 @@ static void test_mmap_supported(int fd, size_t total_size)
> >  	kvm_munmap(mem, total_size);
> >  }
> >  
> > +/*
> > + * Fill @nids with the first @nr_nids nodes in the allowed mask.
> > + * Return false if the mask contains fewer than @nr_nids nodes.
> > + */
> > +static bool get_numa_node_ids(int *nids, int nr_nids)
> > +{
> > +	unsigned long nodemask = get_numa_mem_nodes();
> > +	unsigned long nid;
> > +	int nr_found = 0;
> > +
> > +	for_each_set_bit(nid, &nodemask, BITS_PER_TYPE(nodemask)) {
> > +		nids[nr_found++] = nid;
> > +		if (nr_found == nr_nids)
> > +			return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >  static void test_mbind(int fd, size_t total_size)
> >  {
> > -	const unsigned long nodemask_0 = 1; /* nid: 0 */
> > -	unsigned long nodemask = 0;
> > -	unsigned long maxnode = BITS_PER_TYPE(nodemask);
> > +	unsigned long nodemask, bind_nodemask;
> > +	unsigned long maxnode = BITS_PER_TYPE(nodemask) + 1;
> >  	int policy;
> >  	char *mem;
> > +	int nid;
> >  	int ret;
> >  
> > -	if (!is_multi_numa_node_system())
> > +	if (!get_numa_node_ids(&nid, 1))
> 
> This is not functionally equivalent.  The existing test requires multiple NUMA
> nodes, whereas this will now succeed if there's exactly one node.  That could be
> totally fine, but it needs to be isolated and explained in its own patch.

Sure.

Thanks,
Shivank