Re: [PATCH for-4.22 4/5] tests/numa: add unit tests for NUMA setup logic

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Jun 03, 2026 at 10:38:52AM +0200, Jan Beulich wrote:
> On 01.06.2026 17:43, Roger Pau Monne wrote:
> > --- /dev/null
> > +++ b/tools/tests/numa/.gitignore
> > @@ -0,0 +1,2 @@
> > +/numa.h
> > +/test-numa
> 
> Why the leading slashes?

This is the format of the .gitignore that we use in the pdx, numa and
rengeset testing.  The slashes denote that the pattern is relative to
the particular .gitignore itself, but won't match any level below the
.gitignore.

> > --- /dev/null
> > +++ b/tools/tests/numa/Makefile
> > @@ -0,0 +1,47 @@
> > +XEN_ROOT=$(CURDIR)/../../..
> > +include $(XEN_ROOT)/tools/Rules.mk
> > +
> > +TARGETS := test-numa
> > +
> > +.PHONY: all
> > +all: $(TARGETS)
> > +
> > +.PHONY: run
> > +run: $(TARGETS)
> > +ifeq ($(CC),$(HOSTCC))
> > +   set -e;             \
> > +   for test in $? ; do \
> > +           ./$$test ;  \
> > +   done
> > +else
> > +   $(warning HOSTCC != CC, will not run test)
> > +endif
> > +
> > +.PHONY: clean
> > +clean:
> > +   $(RM) -- *.o $(TARGETS) $(DEPS_RM) numa.h
> > +
> > +.PHONY: distclean
> > +distclean: clean
> > +   $(RM) -- *~
> 
> I see we remove *~ elsewhere, but not everywhere. I don't, however, know
> why we have that, and hence I wonder whether it really wants replicating.

Seems like *~ is a backup file created by some editors (Emacs or Vim
for example.  Again this is a verbatim copy of the Makefile that we
use for other unit testing (I think I copied this from the PDX
testing).

> > --- /dev/null
> > +++ b/tools/tests/numa/harness.h
> > @@ -0,0 +1,184 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Unit tests for NUMA setup.
> > + *
> > + * Copyright (C) 2026 Cloud Software Group
> > + */
> > +
> > +#ifndef _TEST_HARNESS_
> > +#define _TEST_HARNESS_
> > +
> > +#include <assert.h>
> > +#include <errno.h>
> > +#include <inttypes.h>
> > +#include <stdbool.h>
> > +#include <stdint.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +
> > +#include <xen-tools/bitops.h>
> > +#include <xen-tools/common-macros.h>
> > +
> > +#define CONFIG_DEBUG
> > +#define CONFIG_NUMA
> > +#define CONFIG_NR_NUMA_NODES 64
> > +#define NR_CPUS 256
> > +#define MAX_RANGES 128
> > +#define PADDR_BITS 52
> > +
> > +#define __init
> > +#define __initdata
> > +#define __ro_after_init
> > +#define __read_mostly
> > +
> > +#define printk printf
> > +#define XENLOG_INFO ""
> > +#define XENLOG_DEBUG ""
> > +#define XENLOG_WARNING ""
> > +#define KERN_INFO ""
> > +#define KERN_ERR ""
> > +#define KERN_WARNING ""
> > +#define KERN_DEBUG ""
> > +
> > +#define PAGE_SHIFT    12
> > +/* Some libcs define PAGE_SIZE in limits.h. */
> > +#undef  PAGE_SIZE
> > +#define PAGE_SIZE     (1L << PAGE_SHIFT)
> > +#define MAX_ORDER     18 /* 2 * PAGETABLE_ORDER (9) */
> > +
> > +#define PFN_DOWN(x)   ((x) >> PAGE_SHIFT)
> > +#define PFN_UP(x)     (((x) + PAGE_SIZE-1) >> PAGE_SHIFT)
> > +
> > +#define paddr_to_pfn(pa)  ((unsigned long)((pa) >> PAGE_SHIFT))
> > +#define mfn_to_pdx(mfn)   (mfn)
> > +#define paddr_to_pdx(pa)  ((pa) >> PAGE_SHIFT)
> > +#define mfn_to_maddr(mfn) ((mfn) << PAGE_SHIFT)
> > +
> > +#define ASSERT assert
> > +#define ASSERT_UNREACHABLE() assert(0)
> > +
> > +/* For the purposes of the testing assume arch NID == Xen NID. */
> > +#define numa_node_to_arch_nid(n) (n)
> > +
> > +typedef uint64_t paddr_t;
> > +#define PRIpaddr "016" PRIx64
> > +
> > +typedef unsigned long mfn_t;
> > +typedef uint8_t nodeid_t;
> > +
> > +#define __set_bit set_bit
> > +#define __clear_bit clear_bit
> > +
> > +static inline unsigned int find_next_bit(
> > +    const unsigned long *addr, unsigned int size, unsigned int off)
> > +{
> > +    unsigned int i;
> > +
> > +    ASSERT(size <= BITS_PER_LONG);
> > +
> > +    for ( i = off; i < size; i++ )
> > +        if ( !!(*addr & (1UL << i)) )
> 
> Why the !! ?

I copied this from the PDX header and simplified the function because
now it only cares about set values, and forgot to drop it.  I can
indeed drop the !!.

> 
> > +            return i;
> > +
> > +    return size;
> > +}
> > +
> > +#define find_first_bit(b, s) find_next_bit(b, s, 0)
> > +
> > +/* Minimal cpumask support. */
> > +typedef struct cpumask{ DECLARE_BITMAP(bits, NR_CPUS); } cpumask_t;
> > +
> > +#define cpumask_clear_cpu(c, m) clear_bit((c), (m)->bits)
> > +
> > +/* Define the nodemask helpers used. */
> > +typedef struct nodemask{ DECLARE_BITMAP(bits, CONFIG_NR_NUMA_NODES); } 
> > nodemask_t;
> > +
> > +#define node_set(node, dst) set_bit((node), (dst).bits)
> 
> To aid readability, omit the parentheses around "node"? (More similar cases
> further down.)

Sure, this is all copied from the nodemask.h header.  I should have
adjusted.

Thanks, Roger.