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 <[email protected]>
> > ---
> > 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
