[PATCH 00/11] Add support for exact-node memory claims

[PATCH 00/11] Add support for exact-node memory claims

[Alejandro Vallejo]

The Xen toolstack supports claiming memory ahead of allocating it with the
intent of assisting bootstorms of massive VMs. This works ok for general VMs,
but falls appart miserably whenever the toolstack attempts to force placement
of specific NUMA nodes.

This series extends the in-hypervisor claim infrastructure to support per-node
claims, and propagates the knob all the way to xl.cfg.

A worthy future goal is the support of multi-node claims per domain, but the
objective of this series is far more modest and merely intends to enable
claiming memory on a single node for specific domains. This solves a
real-world use case of bundling many small VMs in a single NUMA machine.

The feature specifically allows combining domains with exact-node claims,
general claims and no claims at all in a consistent fashion.

Alejandro Vallejo (11):
  xen/memory: Mask XENMEMF_node() to 8 bits
  xen/page_alloc: Remove `claim` from domain_set_outstanding_pages()
  xen/page_alloc: Add static per-node counts of free pages
  xen: Add node argument to
    domain_{adjust_tot_pages,set_outstanding_pages}()
  xen: Create per-node outstanding claims
  xen/page_alloc: Hook per-node claims to alloc_heap_pages()
  xen/page_alloc: Set node affinity when claiming pages from an exact
    node
  xen/memory: Enable parsing NUMA node argument in XENMEM_claim_pages
  tools/xc: Add `node` argument to xc_domain_claim_pages()
  tools/xl: Expose a "claim_on_node" setting in xl.cfg
  docs/man: Document the new claim_on_node option

 docs/man/xl-numa-placement.7.pod     |   8 ++
 docs/man/xl.1.pod.in                 |   2 +-
 docs/man/xl.cfg.5.pod.in             |  14 +++
 tools/golang/xenlight/helpers.gen.go |   2 +
 tools/golang/xenlight/types.gen.go   |   1 +
 tools/include/xenctrl.h              |   1 +
 tools/include/xenguest.h             |   7 ++
 tools/libs/ctrl/xc_domain.c          |  13 ++-
 tools/libs/guest/xg_dom_core.c       |   1 +
 tools/libs/guest/xg_dom_x86.c        |  22 ++---
 tools/libs/light/libxl_dom.c         |   2 +
 tools/libs/light/libxl_types.idl     |   3 +-
 tools/xl/xl_parse.c                  |  11 +++
 xen/arch/x86/mm.c                    |   3 +-
 xen/arch/x86/mm/mem_sharing.c        |   4 +-
 xen/common/domain.c                  |   2 +-
 xen/common/grant_table.c             |   4 +-
 xen/common/memory.c                  |  25 +++++-
 xen/common/page_alloc.c              | 125 ++++++++++++++++++++++-----
 xen/include/public/memory.h          |   5 +-
 xen/include/xen/mm.h                 |   6 +-
 xen/include/xen/sched.h              |   3 +
 22 files changed, 216 insertions(+), 48 deletions(-)

-- 
2.48.1

[PATCH 01/11] xen/memory: Mask XENMEMF_node() to 8 bits

[Alejandro Vallejo]

As it is, it's incredibly easy for a buggy call to XENMEMF_node() to
unintentionally overflow into bit 17 and beyond. Prevent it by masking,
just like MEMF_* does.

While at it, turn "x" into "n" and "f" to better indicate whether the
macro takes nodes or mem_flags (just like their MEMF_* counterparts).

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 xen/include/public/memory.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index bd9fc37b5297..077eef48c60d 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -32,8 +32,9 @@
 #define XENMEMF_address_bits(x)     (x)
 #define XENMEMF_get_address_bits(x) ((x) & 0xffu)
 /* NUMA node to allocate from. */
-#define XENMEMF_node(x)     (((x) + 1) << 8)
-#define XENMEMF_get_node(x) ((((x) >> 8) - 1) & 0xffu)
+#define XENMEMF_node_mask   (0xffu)
+#define XENMEMF_node(n)     ((((n) + 1) & XENMEMF_node_mask) << 8)
+#define XENMEMF_get_node(f) ((((f) >> 8) - 1) & XENMEMF_node_mask)
 /* Flag to populate physmap with populate-on-demand entries */
 #define XENMEMF_populate_on_demand (1<<16)
 /* Flag to request allocation only from the node specified */
-- 
2.48.1

[PATCH 02/11] xen/page_alloc: Remove `claim` from domain_set_outstanding_pages()

[Alejandro Vallejo]

With a single global count for the claims it's easy to substract
domain_tot_pages() from the claim so the number given in the hypercall
is the real reservation of the domain. This is the current behaviour.

However, a later patch introduces exact-node claims and those interact
very poorly with such a scheme. Since accounting domain_tot_pages() in
one case but not the other seems strictly worse than not accounting them
at all (which is at least consistent), this patch stops substracting
tot_pages from the claim and instead checks that claimed memory +
allocated memory don't exceed max_mem.

Arguably it's also clearer for the caller to align the amount of claimed
memory with that of the requested claim.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 xen/common/page_alloc.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index bd4538c28d82..49c3258169db 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -523,7 +523,7 @@ out:
 int domain_set_outstanding_pages(struct domain *d, unsigned long pages)
 {
     int ret = -ENOMEM;
-    unsigned long claim, avail_pages;
+    unsigned long avail_pages;
 
     /*
      * take the domain's page_alloc_lock, else all d->tot_page adjustments
@@ -549,28 +549,21 @@ int domain_set_outstanding_pages(struct domain *d, unsigned long pages)
         goto out;
     }
 
-    /* disallow a claim not exceeding domain_tot_pages() or above max_pages */
-    if ( (pages <= domain_tot_pages(d)) || (pages > d->max_pages) )
+    /* Don't claim past max_pages */
+    if ( (domain_tot_pages(d) + pages) > d->max_pages )
     {
         ret = -EINVAL;
         goto out;
     }
 
     /* how much memory is available? */
-    avail_pages = total_avail_pages;
+    avail_pages = total_avail_pages - outstanding_claims;
 
-    avail_pages -= outstanding_claims;
-
-    /*
-     * Note, if domain has already allocated memory before making a claim
-     * then the claim must take domain_tot_pages() into account
-     */
-    claim = pages - domain_tot_pages(d);
-    if ( claim > avail_pages )
+    if ( pages > avail_pages )
         goto out;
 
     /* yay, claim fits in available memory, stake the claim, success! */
-    d->outstanding_pages = claim;
+    d->outstanding_pages = pages;
     outstanding_claims += d->outstanding_pages;
     ret = 0;
 
-- 
2.48.1

[PATCH 03/11] xen/page_alloc: Add static per-node counts of free pages

[Alejandro Vallejo]

These are effectively the sum of free memory in all zones of each node.
It's an optimization to avoid doing that operation frequently in
following patches that introduce exact-node claims.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 xen/common/page_alloc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 49c3258169db..733b0300a767 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -485,6 +485,9 @@ static unsigned long node_need_scrub[MAX_NUMNODES];
 static unsigned long *avail[MAX_NUMNODES];
 static long total_avail_pages;
 
+/* Per-node counts of free pages */
+static unsigned long pernode_avail_pages[MAX_NUMNODES];
+
 static DEFINE_SPINLOCK(heap_lock);
 static long outstanding_claims; /* total outstanding claims by all domains */
 
@@ -1033,6 +1036,7 @@ static struct page_info *alloc_heap_pages(
 
     ASSERT(avail[node][zone] >= request);
     avail[node][zone] -= request;
+    pernode_avail_pages[node] -= request;
     total_avail_pages -= request;
     ASSERT(total_avail_pages >= 0);
 
@@ -1191,6 +1195,8 @@ static int reserve_offlined_page(struct page_info *head)
             continue;
 
         avail[node][zone]--;
+        ASSERT(pernode_avail_pages[node] > 0);
+        pernode_avail_pages[node]--;
         total_avail_pages--;
         ASSERT(total_avail_pages >= 0);
 
@@ -1515,6 +1521,7 @@ static void free_heap_pages(
     }
 
     avail[node][zone] += 1 << order;
+    pernode_avail_pages[node] += 1 << order;
     total_avail_pages += 1 << order;
     if ( need_scrub )
     {
-- 
2.48.1

[PATCH 04/11] xen: Add node argument to domain_{adjust_tot_pages,set_outstanding_pages}()

[Alejandro Vallejo]

domain_adjust_tot_pages() decreases the outstanding claims of a domain
as pages are allocated, so that'll need to take into account the node in
which an allocation is done. Deallocations just pass NUMA_NO_NODE.

domain_set_outstanding_pages() takes the node on which to to stake an
exact-node claim, or NUMA_NO_NODE if it's a non-exact claim.

Not a functional change, as neither function uses the arguments for
anything yet. It's a prerequisite to simplify for the following patch
that introduces per-node claim counts.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 xen/arch/x86/mm.c             |  3 ++-
 xen/arch/x86/mm/mem_sharing.c |  4 ++--
 xen/common/domain.c           |  2 +-
 xen/common/grant_table.c      |  4 ++--
 xen/common/memory.c           |  6 ++++--
 xen/common/page_alloc.c       | 17 ++++++++++++-----
 xen/include/xen/mm.h          |  6 ++++--
 7 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index bfdc8fb01949..89f87d013099 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4458,7 +4458,8 @@ int steal_page(
     page_list_del(page, &d->page_list);
 
     /* Unlink from original owner. */
-    if ( !(memflags & MEMF_no_refcount) && !domain_adjust_tot_pages(d, -1) )
+    if ( !(memflags & MEMF_no_refcount) &&
+         !domain_adjust_tot_pages(d, NUMA_NO_NODE, -1) )
         drop_dom_ref = true;
 
     nrspin_unlock(&d->page_alloc_lock);
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index da28266ef076..2551c0d86e80 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -720,7 +720,7 @@ static int page_make_sharable(struct domain *d,
     if ( !validate_only )
     {
         page_set_owner(page, dom_cow);
-        drop_dom_ref = !domain_adjust_tot_pages(d, -1);
+        drop_dom_ref = !domain_adjust_tot_pages(d, NUMA_NO_NODE, -1);
         page_list_del(page, &d->page_list);
     }
 
@@ -766,7 +766,7 @@ static int page_make_private(struct domain *d, struct page_info *page)
     ASSERT(page_get_owner(page) == dom_cow);
     page_set_owner(page, d);
 
-    if ( domain_adjust_tot_pages(d, 1) == 1 )
+    if ( domain_adjust_tot_pages(d, page_to_nid(page), 1) == 1 )
         get_knownalive_domain(d);
     page_list_add_tail(page, &d->page_list);
     nrspin_unlock(&d->page_alloc_lock);
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 585fd726a941..72d8d62bc1e8 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1205,7 +1205,7 @@ int domain_kill(struct domain *d)
         rspin_barrier(&d->domain_lock);
         argo_destroy(d);
         vnuma_destroy(d->vnuma);
-        domain_set_outstanding_pages(d, 0);
+        domain_set_outstanding_pages(d, NUMA_NO_NODE, 0);
         /* fallthrough */
     case DOMDYING_dying:
         rc = domain_teardown(d);
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 6c77867f8cdd..d8c5321185c6 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -2403,7 +2403,7 @@ gnttab_transfer(
         }
 
         /* Okay, add the page to 'e'. */
-        if ( unlikely(domain_adjust_tot_pages(e, 1) == 1) )
+        if ( unlikely(domain_adjust_tot_pages(e, page_to_nid(page), 1) == 1) )
             get_knownalive_domain(e);
 
         /*
@@ -2429,7 +2429,7 @@ gnttab_transfer(
              * page in the page total
              */
             nrspin_lock(&e->page_alloc_lock);
-            drop_dom_ref = !domain_adjust_tot_pages(e, -1);
+            drop_dom_ref = !domain_adjust_tot_pages(e, NUMA_NO_NODE, -1);
             nrspin_unlock(&e->page_alloc_lock);
 
             if ( okay /* i.e. e->is_dying due to the surrounding if() */ )
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 8ca4e1a8425b..1ab0bac4e7da 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -773,7 +773,8 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
 
                 nrspin_lock(&d->page_alloc_lock);
                 drop_dom_ref = (dec_count &&
-                                !domain_adjust_tot_pages(d, -dec_count));
+                                !domain_adjust_tot_pages(d, NUMA_NO_NODE,
+                                -dec_count));
                 nrspin_unlock(&d->page_alloc_lock);
 
                 if ( drop_dom_ref )
@@ -1680,7 +1681,8 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         rc = xsm_claim_pages(XSM_PRIV, d);
 
         if ( !rc )
-            rc = domain_set_outstanding_pages(d, reservation.nr_extents);
+            rc = domain_set_outstanding_pages(d, NUMA_NO_NODE,
+                                              reservation.nr_extents);
 
         rcu_unlock_domain(d);
 
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 733b0300a767..9243c4f51370 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -491,7 +491,8 @@ static unsigned long pernode_avail_pages[MAX_NUMNODES];
 static DEFINE_SPINLOCK(heap_lock);
 static long outstanding_claims; /* total outstanding claims by all domains */
 
-unsigned long domain_adjust_tot_pages(struct domain *d, long pages)
+unsigned long domain_adjust_tot_pages(struct domain *d, nodeid_t node,
+                                      long pages)
 {
     ASSERT(rspin_is_locked(&d->page_alloc_lock));
     d->tot_pages += pages;
@@ -523,7 +524,8 @@ out:
     return d->tot_pages;
 }
 
-int domain_set_outstanding_pages(struct domain *d, unsigned long pages)
+int domain_set_outstanding_pages(struct domain *d, nodeid_t node,
+                                 unsigned long pages)
 {
     int ret = -ENOMEM;
     unsigned long avail_pages;
@@ -2600,6 +2602,8 @@ int assign_pages(
 
     if ( !(memflags & MEMF_no_refcount) )
     {
+        nodeid_t node = page_to_nid(&pg[0]);
+
         if ( unlikely(d->tot_pages + nr < nr) )
         {
             gprintk(XENLOG_INFO,
@@ -2611,7 +2615,9 @@ int assign_pages(
             goto out;
         }
 
-        if ( unlikely(domain_adjust_tot_pages(d, nr) == nr) )
+        ASSERT(node == page_to_nid(&pg[nr - 1]));
+
+        if ( unlikely(domain_adjust_tot_pages(d, node, nr) == nr) )
             get_knownalive_domain(d);
     }
 
@@ -2744,7 +2750,8 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
                 }
             }
 
-            drop_dom_ref = !domain_adjust_tot_pages(d, -(1 << order));
+            drop_dom_ref = !domain_adjust_tot_pages(d, NUMA_NO_NODE,
+                                                    -(1 << order));
 
             rspin_unlock(&d->page_alloc_lock);
 
@@ -2957,7 +2964,7 @@ void free_domstatic_page(struct page_info *page)
 
     arch_free_heap_page(d, page);
 
-    drop_dom_ref = !domain_adjust_tot_pages(d, -1);
+    drop_dom_ref = !domain_adjust_tot_pages(d, NUMA_NO_NODE, -1);
 
     unprepare_staticmem_pages(page, 1, scrub_debug);
 
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index ae1c48a61545..e577a450681c 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -64,6 +64,7 @@
 #include <xen/bug.h>
 #include <xen/compiler.h>
 #include <xen/mm-frame.h>
+#include <xen/numa.h>
 #include <xen/types.h>
 #include <xen/list.h>
 #include <xen/spinlock.h>
@@ -129,8 +130,9 @@ mfn_t xen_map_to_mfn(unsigned long va);
 int populate_pt_range(unsigned long virt, unsigned long nr_mfns);
 /* Claim handling */
 unsigned long __must_check domain_adjust_tot_pages(struct domain *d,
-    long pages);
-int domain_set_outstanding_pages(struct domain *d, unsigned long pages);
+    nodeid_t node, long pages);
+int domain_set_outstanding_pages(struct domain *d, nodeid_t node,
+                                 unsigned long pages);
 void get_outstanding_claims(uint64_t *free_pages, uint64_t *outstanding_pages);
 
 /* Domain suballocator. These functions are *not* interrupt-safe.*/
-- 
2.48.1

[PATCH 05/11] xen: Create per-node outstanding claims

[Alejandro Vallejo]

Extends domain_set_outstanding_claims() to allow staking claims on an
exact node. Also creates global per-node claim counts analogous to
`outstanding_claims`. Note that the per-node counts can't replace the
global one if we want exact-node claims to coexist with non-exact
claims.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 xen/common/page_alloc.c | 32 +++++++++++++++++++++++++++++++-
 xen/include/xen/sched.h |  3 +++
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 9243c4f51370..7fe574b29407 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -490,6 +490,7 @@ static unsigned long pernode_avail_pages[MAX_NUMNODES];
 
 static DEFINE_SPINLOCK(heap_lock);
 static long outstanding_claims; /* total outstanding claims by all domains */
+static unsigned long pernode_oc[MAX_NUMNODES]; /* per-node outstanding claims */
 
 unsigned long domain_adjust_tot_pages(struct domain *d, nodeid_t node,
                                       long pages)
@@ -501,20 +502,31 @@ unsigned long domain_adjust_tot_pages(struct domain *d, nodeid_t node,
      * can test d->outstanding_pages race-free because it can only change
      * if d->page_alloc_lock and heap_lock are both held, see also
      * domain_set_outstanding_pages below
+     *
+     * If `d` has an exact-node claim, we must exit early if this is an
+     * adjustment attributed to another node.
      */
-    if ( !d->outstanding_pages || pages <= 0 )
+    if ( !d->outstanding_pages || pages <= 0 ||
+         (d->claim_node != NUMA_NO_NODE && d->claim_node != node) )
         goto out;
 
+
     spin_lock(&heap_lock);
     BUG_ON(outstanding_claims < d->outstanding_pages);
     if ( d->outstanding_pages < pages )
     {
         /* `pages` exceeds the domain's outstanding count. Zero it out. */
+        if ( d->claim_node != NUMA_NO_NODE )
+            pernode_oc[d->claim_node] -= d->outstanding_pages;
+
         outstanding_claims -= d->outstanding_pages;
         d->outstanding_pages = 0;
     }
     else
     {
+        if ( d->claim_node != NUMA_NO_NODE )
+            pernode_oc[d->claim_node] -= pages;
+
         outstanding_claims -= pages;
         d->outstanding_pages -= pages;
     }
@@ -542,6 +554,10 @@ int domain_set_outstanding_pages(struct domain *d, nodeid_t node,
     if ( pages == 0 )
     {
         outstanding_claims -= d->outstanding_pages;
+
+        if ( d->claim_node != NUMA_NO_NODE )
+            pernode_oc[d->claim_node] -= d->outstanding_pages;
+
         d->outstanding_pages = 0;
         ret = 0;
         goto out;
@@ -564,12 +580,26 @@ int domain_set_outstanding_pages(struct domain *d, nodeid_t node,
     /* how much memory is available? */
     avail_pages = total_avail_pages - outstanding_claims;
 
+    /* This check can't be skipped for the NUMA case, or we may overclaim */
     if ( pages > avail_pages )
         goto out;
 
+    if ( node != NUMA_NO_NODE )
+    {
+        avail_pages = pernode_avail_pages[node] - pernode_oc[node];
+
+        if ( pages > avail_pages )
+            goto out;
+    }
+
     /* yay, claim fits in available memory, stake the claim, success! */
     d->outstanding_pages = pages;
     outstanding_claims += d->outstanding_pages;
+    d->claim_node = node;
+
+    if ( node != NUMA_NO_NODE )
+        pernode_oc[node] += pages;
+
     ret = 0;
 
 out:
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 559d201e0c7e..307a9d749f5d 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -406,6 +406,9 @@ struct domain
     unsigned int     max_pages;         /* maximum value for domain_tot_pages() */
     unsigned int     extra_pages;       /* pages not included in domain_tot_pages() */
 
+    /* NUMA node from which outstanding pages have been reserved */
+    unsigned int     claim_node;
+
 #ifdef CONFIG_MEM_SHARING
     atomic_t         shr_pages;         /* shared pages */
 #endif
-- 
2.48.1

[PATCH 06/11] xen/page_alloc: Hook per-node claims to alloc_heap_pages()

[Alejandro Vallejo]

Extend the claim checks in alloc_heap_pages() to exact-node claims. The
logic is slightly more complicated, so the patch moves it all to an
auxiliary function.

exact-node claims also follow global claims in order to ensure both can
coexist in the same system.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 xen/common/page_alloc.c | 44 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 41 insertions(+), 3 deletions(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 7fe574b29407..cfaa64d3b858 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -991,6 +991,46 @@ static void init_free_page_fields(struct page_info *pg)
     page_set_owner(pg, NULL);
 }
 
+/*
+ * Determine whether a heap allocation is allowed after considering all
+ * outstanding claims in the system.
+ *
+ * Exact-node allocations must also take into account global claims!
+ *
+ * e.g:
+ *   Consider a domain for which toolstack issued a non-exact claim of 75% of
+ *   host memory and another domain for which toolstack tries to issue an
+ *   exact-node claim of 50% of host memory. If the exact claim didn't consider
+ *   non-exact claims too we would overallocate, which is exactly what claims
+ *   are trying to prevent.
+ */
+static bool can_alloc(struct domain *d, unsigned int memflags,
+                      unsigned long request)
+{
+    nodeid_t node = (memflags & MEMF_exact_node) ? MEMF_get_node(memflags) :
+                                                   NUMA_NO_NODE;
+
+    if ( outstanding_claims + request <= total_avail_pages )
+    {
+        if ( node == NUMA_NO_NODE )
+            return true;
+
+        if ( pernode_oc[node] + request <= pernode_avail_pages[node] )
+            return true;
+    }
+
+    /*
+     * Not enough unclaimed memory. Only allow if it's already claimed on the
+     * right node. d->claim_node == NUMA_NO_NODE if the claim isn't on an
+     * exact node.
+     *
+     * Only refcounted allocs attributed to domains may have been claimed
+     */
+
+    return d && d->claim_node == node && d->outstanding_pages >= request &&
+           !(memflags & MEMF_no_refcount);
+}
+
 /* Allocate 2^@order contiguous pages. */
 static struct page_info *alloc_heap_pages(
     unsigned int zone_lo, unsigned int zone_hi,
@@ -1021,9 +1061,7 @@ static struct page_info *alloc_heap_pages(
      * Claimed memory is considered unavailable unless the request
      * is made by a domain with sufficient unclaimed pages.
      */
-    if ( (outstanding_claims + request > total_avail_pages) &&
-          ((memflags & MEMF_no_refcount) ||
-           !d || d->outstanding_pages < request) )
+    if ( !can_alloc(d, memflags, request) )
     {
         spin_unlock(&heap_lock);
         return NULL;
-- 
2.48.1

[PATCH 07/11] xen/page_alloc: Set node affinity when claiming pages from an exact node

[Alejandro Vallejo]

Set the domain's node affinity to the claimed node if the claim
specified an exact node. Do it immediately before making any changes in
case setting the affinity fails (even though it shouldn't).

This allows preferentially allocating from the closest NUMA node when
"exact" is not specified (e.g: p2m tables, etc).

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
Toolstacks can just do it themselves, but it's more error prone. If it
claimed pages from an exact node (and remember we can only hold a single
claim at a time) it makes no sense for the domain to be intentionally
allocating from NUMA nodes other than its home node.
---
 xen/common/page_alloc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index cfaa64d3b858..e69a5fcc8d31 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -586,10 +586,16 @@ int domain_set_outstanding_pages(struct domain *d, nodeid_t node,
 
     if ( node != NUMA_NO_NODE )
     {
-        avail_pages = pernode_avail_pages[node] - pernode_oc[node];
+        nodemask_t affinity = NODE_MASK_NONE;
 
+        avail_pages = pernode_avail_pages[node] - pernode_oc[node];
         if ( pages > avail_pages )
             goto out;
+
+        node_set(node, affinity);
+        ret = domain_set_node_affinity(d, &affinity);
+        if ( ret )
+            goto out;
     }
 
     /* yay, claim fits in available memory, stake the claim, success! */
-- 
2.48.1

[PATCH 08/11] xen/memory: Enable parsing NUMA node argument in XENMEM_claim_pages

[Alejandro Vallejo]

Extract a NUMA node from mem_flags. This _must_ be an exact node, and
has the semantics of making a claim on a specific node.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 xen/common/memory.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 1ab0bac4e7da..28f9db89ba62 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1659,6 +1659,9 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         break;
 
     case XENMEM_claim_pages:
+    {
+        nodeid_t node;
+
         if ( unlikely(start_extent) )
             return -EINVAL;
 
@@ -1671,9 +1674,23 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( reservation.extent_order != 0 )
             return -EINVAL;
 
-        if ( reservation.mem_flags != 0 )
+        /* Only allow NUMA-related memory flags to claim on exact nodes */
+        if ( (reservation.mem_flags &
+              !XENMEMF_exact_node(XENMEMF_node_mask - 1)) != 0 )
             return -EINVAL;
 
+        node = XENMEMF_get_node(reservation.mem_flags);
+        if ( node != NUMA_NO_NODE )
+        {
+            /* Disallow advisory nodes. If present, must be exact */
+            if ( !(reservation.mem_flags & XENMEMF_exact_node_request) )
+                return -EINVAL;
+
+            /* Disallow nodes that would overflow the in-hypervisor arrays */
+            if ( node >= MAX_NUMNODES )
+                return -EINVAL;
+        }
+
         d = rcu_lock_domain_by_id(reservation.domid);
         if ( d == NULL )
             return -EINVAL;
@@ -1681,13 +1698,13 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         rc = xsm_claim_pages(XSM_PRIV, d);
 
         if ( !rc )
-            rc = domain_set_outstanding_pages(d, NUMA_NO_NODE,
-                                              reservation.nr_extents);
+            rc = domain_set_outstanding_pages(d, node, reservation.nr_extents);
 
         rcu_unlock_domain(d);
 
         break;
 
+    }
     case XENMEM_get_vnumainfo:
     {
         struct xen_vnuma_topology_info topology;
-- 
2.48.1

[PATCH 09/11] tools/xc: Add `node` argument to xc_domain_claim_pages()

[Alejandro Vallejo]

Add a node argument to make an exact-node claim. NUMA_NO_NODE means to
make a regular any-node claim.

No functional change.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 tools/include/xenctrl.h       |  1 +
 tools/libs/ctrl/xc_domain.c   | 13 ++++++++++++-
 tools/libs/guest/xg_dom_x86.c |  8 +++++---
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 495598123133..2b63992e1246 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1322,6 +1322,7 @@ int xc_domain_populate_physmap_exact(xc_interface *xch,
 
 int xc_domain_claim_pages(xc_interface *xch,
                                uint32_t domid,
+                               unsigned int node,
                                unsigned long nr_pages);
 
 int xc_domain_memory_exchange_pages(xc_interface *xch,
diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
index 2ddc3f4f426d..f2c6eda875dc 100644
--- a/tools/libs/ctrl/xc_domain.c
+++ b/tools/libs/ctrl/xc_domain.c
@@ -20,6 +20,7 @@
  */
 
 #include "xc_private.h"
+#include "xenguest.h"
 #include <xen/memory.h>
 #include <xen/hvm/hvm_op.h>
 
@@ -1072,13 +1073,23 @@ int xc_domain_remove_from_physmap(xc_interface *xch,
 
 int xc_domain_claim_pages(xc_interface *xch,
                                uint32_t domid,
+                               unsigned int node,
                                unsigned long nr_pages)
 {
     int err;
+    unsigned int mem_flags = 0;
+
+    if ( node != XC_NUMA_NO_NODE )
+    {
+        if ( node >= 0xFF )
+            return -EINVAL;
+        mem_flags = XENMEMF_exact_node(node);
+    }
+
     struct xen_memory_reservation reservation = {
         .nr_extents   = nr_pages,
         .extent_order = 0,
-        .mem_flags    = 0, /* no flags */
+        .mem_flags    = mem_flags,
         .domid        = domid
     };
 
diff --git a/tools/libs/guest/xg_dom_x86.c b/tools/libs/guest/xg_dom_x86.c
index cba01384ae75..ac05106a8c1c 100644
--- a/tools/libs/guest/xg_dom_x86.c
+++ b/tools/libs/guest/xg_dom_x86.c
@@ -1199,6 +1199,7 @@ static int meminit_pv(struct xc_dom_image *dom)
     if ( dom->claim_enabled )
     {
         rc = xc_domain_claim_pages(dom->xch, dom->guest_domid,
+                                   XC_NUMA_NO_NODE,
                                    dom->total_pages);
         if ( rc )
             return rc;
@@ -1327,7 +1328,8 @@ static int meminit_pv(struct xc_dom_image *dom)
 
     /* Ensure no unclaimed pages are left unused.
      * OK to call if hadn't done the earlier claim call. */
-    xc_domain_claim_pages(dom->xch, dom->guest_domid, 0 /* cancel claim */);
+    xc_domain_claim_pages(dom->xch, dom->guest_domid, XC_NUMA_NO_NODE,
+                          0 /* cancel claim */);
 
     return rc;
 }
@@ -1442,7 +1444,7 @@ static int meminit_hvm(struct xc_dom_image *dom)
      * allocated is pointless.
      */
     if ( claim_enabled ) {
-        rc = xc_domain_claim_pages(xch, domid,
+        rc = xc_domain_claim_pages(xch, domid, XC_NUMA_NO_NODE,
                                    target_pages - dom->vga_hole_size);
         if ( rc != 0 )
         {
@@ -1642,7 +1644,7 @@ static int meminit_hvm(struct xc_dom_image *dom)
  out:
 
     /* ensure no unclaimed pages are left unused */
-    xc_domain_claim_pages(xch, domid, 0 /* cancels the claim */);
+    xc_domain_claim_pages(xch, domid, XC_NUMA_NO_NODE, 0 /* cancel claim */);
 
     return rc;
 }
-- 
2.48.1

[PATCH 10/11] tools/xl: Expose a "claim_on_node" setting in xl.cfg

[Alejandro Vallejo]

Expose a setting to explicitly select a NUMA node for created domains

If the hypervisor can't reserve enough memory in the relevant NUMA node
it fails the claim early. Also, disable automatic NUMA placement when
this new option is enabled.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 tools/golang/xenlight/helpers.gen.go |  2 ++
 tools/golang/xenlight/types.gen.go   |  1 +
 tools/include/xenguest.h             |  7 +++++++
 tools/libs/guest/xg_dom_core.c       |  1 +
 tools/libs/guest/xg_dom_x86.c        | 18 +++++++++---------
 tools/libs/light/libxl_dom.c         |  2 ++
 tools/libs/light/libxl_types.idl     |  3 ++-
 tools/xl/xl_parse.c                  | 11 +++++++++++
 8 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
index 90846ea8e882..9204020224d5 100644
--- a/tools/golang/xenlight/helpers.gen.go
+++ b/tools/golang/xenlight/helpers.gen.go
@@ -1108,6 +1108,7 @@ x.LlcColors[i] = uint32(v)
 if err := x.ClaimMode.fromC(&xc.claim_mode);err != nil {
 return fmt.Errorf("converting field ClaimMode: %v", err)
 }
+x.ClaimOnNode = uint32(xc.claim_on_node)
 x.EventChannels = uint32(xc.event_channels)
 x.Kernel = C.GoString(xc.kernel)
 x.Cmdline = C.GoString(xc.cmdline)
@@ -1472,6 +1473,7 @@ cLlcColors[i] = C.uint32_t(v)
 if err := x.ClaimMode.toC(&xc.claim_mode); err != nil {
 return fmt.Errorf("converting field ClaimMode: %v", err)
 }
+xc.claim_on_node = C.uint32_t(x.ClaimOnNode)
 xc.event_channels = C.uint32_t(x.EventChannels)
 if x.Kernel != "" {
 xc.kernel = C.CString(x.Kernel)}
diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
index e7667f1ce3a3..1c6319200411 100644
--- a/tools/golang/xenlight/types.gen.go
+++ b/tools/golang/xenlight/types.gen.go
@@ -577,6 +577,7 @@ Irqs []uint32
 Iomem []IomemRange
 LlcColors []uint32
 ClaimMode Defbool
+ClaimOnNode uint32
 EventChannels uint32
 Kernel string
 Cmdline string
diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index e01f494b772a..08aae1e24e4c 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -185,6 +185,13 @@ struct xc_dom_image {
     uint32_t guest_domid;
     int claim_enabled; /* 0 by default, 1 enables it */
 
+    /*
+     * Exact NUMA node on which to allocate memory from.
+     *
+     * XC_NUMA_NO_NODE by default.
+     */
+    unsigned int claim_on_node;
+
     int xen_version;
     xen_capabilities_info_t xen_caps;
 
diff --git a/tools/libs/guest/xg_dom_core.c b/tools/libs/guest/xg_dom_core.c
index 595b0a667c03..152ec8ea6591 100644
--- a/tools/libs/guest/xg_dom_core.c
+++ b/tools/libs/guest/xg_dom_core.c
@@ -775,6 +775,7 @@ struct xc_dom_image *xc_dom_allocate(xc_interface *xch,
     dom->parms->p2m_base = UNSET_ADDR;
 
     dom->flags = SIF_VIRT_P2M_4TOOLS;
+    dom->claim_on_node = XC_NUMA_NO_NODE;
 
     dom->alloc_malloc += sizeof(*dom);
     return dom;
diff --git a/tools/libs/guest/xg_dom_x86.c b/tools/libs/guest/xg_dom_x86.c
index ac05106a8c1c..4fe816b9bcb4 100644
--- a/tools/libs/guest/xg_dom_x86.c
+++ b/tools/libs/guest/xg_dom_x86.c
@@ -1199,7 +1199,7 @@ static int meminit_pv(struct xc_dom_image *dom)
     if ( dom->claim_enabled )
     {
         rc = xc_domain_claim_pages(dom->xch, dom->guest_domid,
-                                   XC_NUMA_NO_NODE,
+                                   dom->claim_on_node,
                                    dom->total_pages);
         if ( rc )
             return rc;
@@ -1209,9 +1209,10 @@ static int meminit_pv(struct xc_dom_image *dom)
      * that this is a valid state if libxl doesn't provide any
      * vNUMA information.
      *
-     * The dummy values make libxc allocate all pages from
-     * arbitrary physical nodes. This is the expected behaviour if
-     * no vNUMA configuration is provided to libxc.
+     * If there's an outstanding claim on a node, memory is allocated from that
+     * node. Otherwise the dummy values make libxc allocate all pages from
+     * arbitrary physical nodes. This is the expected behaviour if no vNUMA
+     * configuration is provided to libxc and.
      *
      * Note that the following hunk is just for the convenience of
      * allocation code. No defaulting happens in libxc.
@@ -1227,7 +1228,7 @@ static int meminit_pv(struct xc_dom_image *dom)
 
         nr_vnodes = 1;
         vnode_to_pnode = dummy_vnode_to_pnode;
-        vnode_to_pnode[0] = XC_NUMA_NO_NODE;
+        vnode_to_pnode[0] = dom->claim_on_node;
     }
     else
     {
@@ -1357,7 +1358,6 @@ static int meminit_hvm(struct xc_dom_image *dom)
     unsigned long stat_normal_pages = 0, stat_2mb_pages = 0,
         stat_1gb_pages = 0;
     unsigned int memflags = 0;
-    int claim_enabled = dom->claim_enabled;
     uint64_t total_pages;
     xen_vmemrange_t dummy_vmemrange[2];
     unsigned int dummy_vnode_to_pnode[1];
@@ -1397,7 +1397,7 @@ static int meminit_hvm(struct xc_dom_image *dom)
             nr_vmemranges++;
         }
 
-        dummy_vnode_to_pnode[0] = XC_NUMA_NO_NODE;
+        dummy_vnode_to_pnode[0] = dom->claim_on_node;
         nr_vnodes = 1;
         vmemranges = dummy_vmemrange;
         vnode_to_pnode = dummy_vnode_to_pnode;
@@ -1443,8 +1443,8 @@ static int meminit_hvm(struct xc_dom_image *dom)
      * actually allocates memory for the guest. Claiming after memory has been
      * allocated is pointless.
      */
-    if ( claim_enabled ) {
-        rc = xc_domain_claim_pages(xch, domid, XC_NUMA_NO_NODE,
+    if ( dom->claim_enabled ) {
+        rc = xc_domain_claim_pages(xch, domid, dom->claim_on_node,
                                    target_pages - dom->vga_hole_size);
         if ( rc != 0 )
         {
diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
index 94fef374014e..1a5500702239 100644
--- a/tools/libs/light/libxl_dom.c
+++ b/tools/libs/light/libxl_dom.c
@@ -663,6 +663,7 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
     dom->xenstore_evtchn = state->store_port;
     dom->xenstore_domid = state->store_domid;
     dom->claim_enabled = libxl_defbool_val(info->claim_mode);
+    dom->claim_on_node = info->claim_on_node;
     dom->max_vcpus = info->max_vcpus;
 
     if (info->num_vnuma_nodes != 0) {
@@ -1090,6 +1091,7 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
     mem_size = (uint64_t)(info->max_memkb - info->video_memkb) << 10;
     dom->target_pages = (uint64_t)(info->target_memkb - info->video_memkb) >> 2;
     dom->claim_enabled = libxl_defbool_val(info->claim_mode);
+    dom->claim_on_node = info->claim_on_node;
     if (info->u.hvm.mmio_hole_memkb) {
         uint64_t max_ram_below_4g = (1ULL << 32) -
             (info->u.hvm.mmio_hole_memkb << 10);
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index bd4b8721ff19..a252b36c2b5d 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -617,7 +617,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("irqs",             Array(uint32, "num_irqs")),
     ("iomem",            Array(libxl_iomem_range, "num_iomem")),
     ("llc_colors",       Array(uint32, "num_llc_colors")),
-    ("claim_mode",	     libxl_defbool),
+    ("claim_mode",       libxl_defbool),
+    ("claim_on_node",    uint32),
     ("event_channels",   uint32),
     ("kernel",           string),
     ("cmdline",          string),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 089a88935aff..5880a032e6d6 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1558,6 +1558,17 @@ void parse_config_data(const char *config_source,
         exit(1);
 
     libxl_defbool_set(&b_info->claim_mode, claim_mode);
+    e = xlu_cfg_get_bounded_long (config, "claim_on_node", 0,
+                                  254, &l, 1);
+    if (e == ESRCH) /* not specified */
+        b_info->claim_on_node = ~0U;
+    else if (!e) {
+        libxl_defbool_set(&b_info->numa_placement, false);
+        libxl_defbool_set(&b_info->claim_mode, true);
+        b_info->claim_on_node = l;
+    }
+    else
+        exit(1);
 
     if (xlu_cfg_get_string (config, "on_poweroff", &buf, 0))
         buf = "destroy";
-- 
2.48.1

[PATCH 11/11] docs/man: Document the new claim_on_node option

[Alejandro Vallejo]

... and while at it, add missing relevant links to xl-numa-placement(7)
in xl.1.pod.in and xl.cfg.5.pod.in, which describes libxl's behaviour on
NUMA placement.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 docs/man/xl-numa-placement.7.pod |  8 ++++++++
 docs/man/xl.1.pod.in             |  2 +-
 docs/man/xl.cfg.5.pod.in         | 14 ++++++++++++++
 3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/docs/man/xl-numa-placement.7.pod b/docs/man/xl-numa-placement.7.pod
index 4d83f26d412e..287ad41e5071 100644
--- a/docs/man/xl-numa-placement.7.pod
+++ b/docs/man/xl-numa-placement.7.pod
@@ -173,6 +173,14 @@ soft affinity belong.
 
 =back
 
+It's possible to force memory to be allocated from a specific NUMA node via the
+"claim_on_node" option (See B<claim_mode> in L<xl.conf(5)> for more details on
+claims). "claim_on_node" associates the domain with a single NUMA node. Domain
+creation fails if not enough memory can be reserved in the node and memory is
+preferentially allocated from that node at runtime. The downside is that
+claiming memory on a node via "claim_on_node" doesn't automatically set
+soft-affinity for the vCPUs, so that must still be done manually for a fully
+optimised single-node domain.
 
 =head2 Placing the guest automatically
 
diff --git a/docs/man/xl.1.pod.in b/docs/man/xl.1.pod.in
index fe38724b2b82..27a972486296 100644
--- a/docs/man/xl.1.pod.in
+++ b/docs/man/xl.1.pod.in
@@ -2012,7 +2012,7 @@ otherwise behavior is undefined.  Setting to 0 disables the timeout.
 The following man pages:
 
 L<xl.cfg(5)>, L<xlcpupool.cfg(5)>, L<xentop(1)>, L<xl-disk-configuration(5)>
-L<xl-network-configuration(5)>
+L<xl-network-configuration(5)>, L<xl-numa-placement(7)>
 
 And the following documents on the xenproject.org website:
 
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index 7339c44efd54..c1ffc29d312a 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -278,6 +278,18 @@ memory=8096 will report significantly less memory available for use
 than a system with maxmem=8096 memory=8096 due to the memory overhead
 of having to track the unused pages.
 
+=item B<claim_on_node=NUMBER>
+
+Binds guest memory to a particular host NUMA node. Creation only starts if
+enough memory on this NUMA node can be reserved beforehand on the hypervisor.
+Failure to claim memory aborts creation early.
+
+See B<claim_mode> in L<xl.conf(5)> for further details on memory claims.
+
+Overriding B<claim_on_node> forces B<claim_mode> to be set on this guest and
+disables automatic NUMA placement (See L<xl-numa-placement(7)> for further
+details on NUMA placement and the effects of this option.)
+
 =back
 
 =head3 Guest Virtual NUMA Configuration
@@ -3143,6 +3155,8 @@ If using this option is necessary to fix an issue, please report a bug.
 
 =item L<xl-network-configuration(5)>
 
+=item L<xl-numa-placement(7)>
+
 =item L<xen-tscmode(7)>
 
 =back
-- 
2.48.1

Re: [PATCH 01/11] xen/memory: Mask XENMEMF_node() to 8 bits

[Jan Beulich]

On 14.03.2025 18:24, Alejandro Vallejo wrote:
> As it is, it's incredibly easy for a buggy call to XENMEMF_node() to
> unintentionally overflow into bit 17 and beyond. Prevent it by masking,
> just like MEMF_* does.

Yet then ...

> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -32,8 +32,9 @@
>  #define XENMEMF_address_bits(x)     (x)
>  #define XENMEMF_get_address_bits(x) ((x) & 0xffu)
>  /* NUMA node to allocate from. */
> -#define XENMEMF_node(x)     (((x) + 1) << 8)
> -#define XENMEMF_get_node(x) ((((x) >> 8) - 1) & 0xffu)
> +#define XENMEMF_node_mask   (0xffu)
> +#define XENMEMF_node(n)     ((((n) + 1) & XENMEMF_node_mask) << 8)

... this still won't have the intended effect: Rather than spilling into
higher bits (with a certain chance of causing an error) you now truncate
the node number, thus making the misbehavior almost certainly silent.
Further, if you do this for the node, why not also for the address bits?
(Rhetorical question; I don't really want you to do that.)

Jan

> +#define XENMEMF_get_node(f) ((((f) >> 8) - 1) & XENMEMF_node_mask)
>  /* Flag to populate physmap with populate-on-demand entries */
>  #define XENMEMF_populate_on_demand (1<<16)
>  /* Flag to request allocation only from the node specified */

Re: [PATCH 01/11] xen/memory: Mask XENMEMF_node() to 8 bits

[Alejandro Vallejo]

For the record, the rest of the series doesn't require this patch. I just
thought it was a strictly net-positive improvement on the current behaviour.

On Mon Mar 17, 2025 at 4:33 PM GMT, Jan Beulich wrote:
> On 14.03.2025 18:24, Alejandro Vallejo wrote:
> > As it is, it's incredibly easy for a buggy call to XENMEMF_node() to
> > unintentionally overflow into bit 17 and beyond. Prevent it by masking,
> > just like MEMF_* does.
>
> Yet then ...
>
> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -32,8 +32,9 @@
> >  #define XENMEMF_address_bits(x)     (x)
> >  #define XENMEMF_get_address_bits(x) ((x) & 0xffu)
> >  /* NUMA node to allocate from. */
> > -#define XENMEMF_node(x)     (((x) + 1) << 8)
> > -#define XENMEMF_get_node(x) ((((x) >> 8) - 1) & 0xffu)
> > +#define XENMEMF_node_mask   (0xffu)
> > +#define XENMEMF_node(n)     ((((n) + 1) & XENMEMF_node_mask) << 8)
>
> ... this still won't have the intended effect: Rather than spilling into
> higher bits (with a certain chance of causing an error) you now truncate
> the node number, thus making the misbehavior almost certainly silent.

It has the intended effect of containing the effects of XENMEMF_node(n) to the
bits representing such mask.

There's an error either way, and either way you'll notice quite late too. One
of them has fully undefined consequences (possibly worth an XSA for systems
with separate xenstore or driver domains). This one contains the effects of
invalid data. A later patch in the series returns EINVAL in xc_claim_pages() if
node >= 0xff to catch problematic inputs early, but that's a toolstack matter,
the API should be self-defending.

Note that this exact code is present in MEMF_node(n), in xen/mm.h Likely to
avoid the same sort of problem inside the hypervisor.

> Further, if you do this for the node, why not also for the address bits?
> (Rhetorical question; I don't really want you to do that.)
>
> Jan

Mostly because the series deals with memory management rather than anything
else. I do think address_bits should be subject to the same treatment for
identical reasons.

>
> > +#define XENMEMF_get_node(f) ((((f) >> 8) - 1) & XENMEMF_node_mask)
> >  /* Flag to populate physmap with populate-on-demand entries */
> >  #define XENMEMF_populate_on_demand (1<<16)
> >  /* Flag to request allocation only from the node specified */

Cheers,
Alejandro

Re: [PATCH 02/11] xen/page_alloc: Remove `claim` from domain_set_outstanding_pages()

[Roger Pau Monné]

On Fri, Mar 14, 2025 at 05:24:53PM +0000, Alejandro Vallejo wrote:
> With a single global count for the claims it's easy to substract
> domain_tot_pages() from the claim so the number given in the hypercall
> is the real reservation of the domain. This is the current behaviour.
> 
> However, a later patch introduces exact-node claims and those interact
> very poorly with such a scheme. Since accounting domain_tot_pages() in
> one case but not the other seems strictly worse than not accounting them
> at all (which is at least consistent), this patch stops substracting
> tot_pages from the claim and instead checks that claimed memory +
> allocated memory don't exceed max_mem.

Hm, while I don't have any specific interest in keeping the current
behavior, XENMEM_claim_pages is part of the stable ABI (it's not a
domctl), and hence should be stable.  Note also the comment above the
definition of XENMEM_claim_pages how it states the specific behavior
that you are trying to change (and which should have been adjusted as
part of this change).

I have no idea why this was made a xenmem rather than a domctl
hypercall, but if you want to change the semantics I think the only
option is introducing a new hypercall.

Thanks, Roger.

Re: [PATCH 03/11] xen/page_alloc: Add static per-node counts of free pages

[Roger Pau Monné]

On Fri, Mar 14, 2025 at 05:24:54PM +0000, Alejandro Vallejo wrote:
> These are effectively the sum of free memory in all zones of each node.
> It's an optimization to avoid doing that operation frequently in
> following patches that introduce exact-node claims.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
>  xen/common/page_alloc.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 49c3258169db..733b0300a767 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -485,6 +485,9 @@ static unsigned long node_need_scrub[MAX_NUMNODES];
>  static unsigned long *avail[MAX_NUMNODES];
>  static long total_avail_pages;
>  
> +/* Per-node counts of free pages */
> +static unsigned long pernode_avail_pages[MAX_NUMNODES];
> +
>  static DEFINE_SPINLOCK(heap_lock);
>  static long outstanding_claims; /* total outstanding claims by all domains */
>  
> @@ -1033,6 +1036,7 @@ static struct page_info *alloc_heap_pages(
>  
>      ASSERT(avail[node][zone] >= request);
>      avail[node][zone] -= request;
> +    pernode_avail_pages[node] -= request;

Since it's done for the per-zone tracking, you might as well add:

ASSERT(pernode_avail_pages[node] >= request);

here?

Thanks, Roger.

Re: [PATCH 04/11] xen: Add node argument to domain_{adjust_tot_pages,set_outstanding_pages}()

[Roger Pau Monné]

On Fri, Mar 14, 2025 at 05:24:55PM +0000, Alejandro Vallejo wrote:
> domain_adjust_tot_pages() decreases the outstanding claims of a domain
> as pages are allocated, so that'll need to take into account the node in
> which an allocation is done. Deallocations just pass NUMA_NO_NODE.
> 
> domain_set_outstanding_pages() takes the node on which to to stake an
> exact-node claim, or NUMA_NO_NODE if it's a non-exact claim.
> 
> Not a functional change, as neither function uses the arguments for
> anything yet. It's a prerequisite to simplify for the following patch
> that introduces per-node claim counts.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
>  xen/arch/x86/mm.c             |  3 ++-
>  xen/arch/x86/mm/mem_sharing.c |  4 ++--
>  xen/common/domain.c           |  2 +-
>  xen/common/grant_table.c      |  4 ++--
>  xen/common/memory.c           |  6 ++++--
>  xen/common/page_alloc.c       | 17 ++++++++++++-----
>  xen/include/xen/mm.h          |  6 ++++--
>  7 files changed, 27 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index bfdc8fb01949..89f87d013099 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4458,7 +4458,8 @@ int steal_page(
>      page_list_del(page, &d->page_list);
>  
>      /* Unlink from original owner. */
> -    if ( !(memflags & MEMF_no_refcount) && !domain_adjust_tot_pages(d, -1) )
> +    if ( !(memflags & MEMF_no_refcount) &&
> +         !domain_adjust_tot_pages(d, NUMA_NO_NODE, -1) )
>          drop_dom_ref = true;
>  
>      nrspin_unlock(&d->page_alloc_lock);
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index da28266ef076..2551c0d86e80 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -720,7 +720,7 @@ static int page_make_sharable(struct domain *d,
>      if ( !validate_only )
>      {
>          page_set_owner(page, dom_cow);
> -        drop_dom_ref = !domain_adjust_tot_pages(d, -1);
> +        drop_dom_ref = !domain_adjust_tot_pages(d, NUMA_NO_NODE, -1);
>          page_list_del(page, &d->page_list);
>      }
>  
> @@ -766,7 +766,7 @@ static int page_make_private(struct domain *d, struct page_info *page)
>      ASSERT(page_get_owner(page) == dom_cow);
>      page_set_owner(page, d);
>  
> -    if ( domain_adjust_tot_pages(d, 1) == 1 )
> +    if ( domain_adjust_tot_pages(d, page_to_nid(page), 1) == 1 )
>          get_knownalive_domain(d);
>      page_list_add_tail(page, &d->page_list);
>      nrspin_unlock(&d->page_alloc_lock);
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 585fd726a941..72d8d62bc1e8 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1205,7 +1205,7 @@ int domain_kill(struct domain *d)
>          rspin_barrier(&d->domain_lock);
>          argo_destroy(d);
>          vnuma_destroy(d->vnuma);
> -        domain_set_outstanding_pages(d, 0);
> +        domain_set_outstanding_pages(d, NUMA_NO_NODE, 0);
>          /* fallthrough */
>      case DOMDYING_dying:
>          rc = domain_teardown(d);
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index 6c77867f8cdd..d8c5321185c6 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -2403,7 +2403,7 @@ gnttab_transfer(
>          }
>  
>          /* Okay, add the page to 'e'. */
> -        if ( unlikely(domain_adjust_tot_pages(e, 1) == 1) )
> +        if ( unlikely(domain_adjust_tot_pages(e, page_to_nid(page), 1) == 1) )
>              get_knownalive_domain(e);
>  
>          /*
> @@ -2429,7 +2429,7 @@ gnttab_transfer(
>               * page in the page total
>               */
>              nrspin_lock(&e->page_alloc_lock);
> -            drop_dom_ref = !domain_adjust_tot_pages(e, -1);
> +            drop_dom_ref = !domain_adjust_tot_pages(e, NUMA_NO_NODE, -1);
>              nrspin_unlock(&e->page_alloc_lock);
>  
>              if ( okay /* i.e. e->is_dying due to the surrounding if() */ )
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 8ca4e1a8425b..1ab0bac4e7da 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -773,7 +773,8 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
>  
>                  nrspin_lock(&d->page_alloc_lock);
>                  drop_dom_ref = (dec_count &&
> -                                !domain_adjust_tot_pages(d, -dec_count));
> +                                !domain_adjust_tot_pages(d, NUMA_NO_NODE,
> +                                -dec_count));

Nit: it would be best if dec_count is aligned with the parenthesis:

                                !domain_adjust_tot_pages(d, NUMA_NO_NODE,
                                                         -dec_count));

Thanks, Roger.

Re: [PATCH 05/11] xen: Create per-node outstanding claims

[Roger Pau Monné]

On Fri, Mar 14, 2025 at 05:24:56PM +0000, Alejandro Vallejo wrote:
> Extends domain_set_outstanding_claims() to allow staking claims on an
> exact node. Also creates global per-node claim counts analogous to
> `outstanding_claims`. Note that the per-node counts can't replace the
> global one if we want exact-node claims to coexist with non-exact
> claims.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
>  xen/common/page_alloc.c | 32 +++++++++++++++++++++++++++++++-
>  xen/include/xen/sched.h |  3 +++
>  2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 9243c4f51370..7fe574b29407 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -490,6 +490,7 @@ static unsigned long pernode_avail_pages[MAX_NUMNODES];
>  
>  static DEFINE_SPINLOCK(heap_lock);
>  static long outstanding_claims; /* total outstanding claims by all domains */
> +static unsigned long pernode_oc[MAX_NUMNODES]; /* per-node outstanding claims */
>  
>  unsigned long domain_adjust_tot_pages(struct domain *d, nodeid_t node,
>                                        long pages)
> @@ -501,20 +502,31 @@ unsigned long domain_adjust_tot_pages(struct domain *d, nodeid_t node,
>       * can test d->outstanding_pages race-free because it can only change
>       * if d->page_alloc_lock and heap_lock are both held, see also
>       * domain_set_outstanding_pages below
> +     *
> +     * If `d` has an exact-node claim, we must exit early if this is an
> +     * adjustment attributed to another node.
>       */
> -    if ( !d->outstanding_pages || pages <= 0 )
> +    if ( !d->outstanding_pages || pages <= 0 ||
> +         (d->claim_node != NUMA_NO_NODE && d->claim_node != node) )
>          goto out;
>  
> +
>      spin_lock(&heap_lock);
>      BUG_ON(outstanding_claims < d->outstanding_pages);
>      if ( d->outstanding_pages < pages )
>      {
>          /* `pages` exceeds the domain's outstanding count. Zero it out. */
> +        if ( d->claim_node != NUMA_NO_NODE )
> +            pernode_oc[d->claim_node] -= d->outstanding_pages;
> +
>          outstanding_claims -= d->outstanding_pages;
>          d->outstanding_pages = 0;
>      }
>      else
>      {
> +        if ( d->claim_node != NUMA_NO_NODE )
> +            pernode_oc[d->claim_node] -= pages;
> +
>          outstanding_claims -= pages;
>          d->outstanding_pages -= pages;
>      }
> @@ -542,6 +554,10 @@ int domain_set_outstanding_pages(struct domain *d, nodeid_t node,
>      if ( pages == 0 )
>      {
>          outstanding_claims -= d->outstanding_pages;
> +
> +        if ( d->claim_node != NUMA_NO_NODE )
> +            pernode_oc[d->claim_node] -= d->outstanding_pages;
> +
>          d->outstanding_pages = 0;
>          ret = 0;
>          goto out;
> @@ -564,12 +580,26 @@ int domain_set_outstanding_pages(struct domain *d, nodeid_t node,
>      /* how much memory is available? */
>      avail_pages = total_avail_pages - outstanding_claims;
>  
> +    /* This check can't be skipped for the NUMA case, or we may overclaim */
>      if ( pages > avail_pages )
>          goto out;
>  
> +    if ( node != NUMA_NO_NODE )
> +    {
> +        avail_pages = pernode_avail_pages[node] - pernode_oc[node];
> +
> +        if ( pages > avail_pages )
> +            goto out;
> +    }
> +
>      /* yay, claim fits in available memory, stake the claim, success! */
>      d->outstanding_pages = pages;
>      outstanding_claims += d->outstanding_pages;
> +    d->claim_node = node;
> +
> +    if ( node != NUMA_NO_NODE )
> +        pernode_oc[node] += pages;
> +
>      ret = 0;
>  
>  out:
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 559d201e0c7e..307a9d749f5d 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -406,6 +406,9 @@ struct domain
>      unsigned int     max_pages;         /* maximum value for domain_tot_pages() */
>      unsigned int     extra_pages;       /* pages not included in domain_tot_pages() */
>  
> +    /* NUMA node from which outstanding pages have been reserved */
> +    unsigned int     claim_node;

This should possibly be nodeid_t rather than unsigned int?

But why is this a single node?  The interface should allow for a
domain to claim memory from multiple different nodes.

The interface here seems to be focused on domains only being allowed
to allocate from a single node, or otherwise you must first allocate
memory from a node before moving to the next one (which defeats the
purpose of claims?).

I think we want to instead convert d->outstanding_pages into a
per-node array, so that a domain can have outstanding claims for
multiple NUMA nodes?

The hypercall interface becomes a bit awkward then, as the toolstack
has to perform a different hypercall for each memory claim from a
different node (and rollback in case of failure).  Ideally we would
need to introduce a new hypercall that allows making claims from
multiple nodes in a single locked region, as to ensure success or
failure in an atomic way.

Thanks, Roger.

Re: [PATCH 06/11] xen/page_alloc: Hook per-node claims to alloc_heap_pages()

[Roger Pau Monné]

On Fri, Mar 14, 2025 at 05:24:57PM +0000, Alejandro Vallejo wrote:
> Extend the claim checks in alloc_heap_pages() to exact-node claims. The
> logic is slightly more complicated, so the patch moves it all to an
> auxiliary function.
> 
> exact-node claims also follow global claims in order to ensure both can
> coexist in the same system.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
>  xen/common/page_alloc.c | 44 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 7fe574b29407..cfaa64d3b858 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -991,6 +991,46 @@ static void init_free_page_fields(struct page_info *pg)
>      page_set_owner(pg, NULL);
>  }
>  
> +/*
> + * Determine whether a heap allocation is allowed after considering all
> + * outstanding claims in the system.
> + *
> + * Exact-node allocations must also take into account global claims!
> + *
> + * e.g:
> + *   Consider a domain for which toolstack issued a non-exact claim of 75% of
> + *   host memory and another domain for which toolstack tries to issue an
> + *   exact-node claim of 50% of host memory. If the exact claim didn't consider
> + *   non-exact claims too we would overallocate, which is exactly what claims

s/overallocate/overclaim/ or maybe "attempt to overallocate".

> + *   are trying to prevent.
> + */
> +static bool can_alloc(struct domain *d, unsigned int memflags,

d can be const here, this helper is just a checker that doesn't modify
anything.

> +                      unsigned long request)
> +{
> +    nodeid_t node = (memflags & MEMF_exact_node) ? MEMF_get_node(memflags) :
> +                                                   NUMA_NO_NODE;
> +
> +    if ( outstanding_claims + request <= total_avail_pages )
> +    {
> +        if ( node == NUMA_NO_NODE )
> +            return true;
> +
> +        if ( pernode_oc[node] + request <= pernode_avail_pages[node] )
> +            return true;
> +    }

You can possibly join all conditions in a single clause?

if ( outstanding_claims + request <= total_avail_pages &&
     (node == NUMA_NO_NODE ||
      pernode_oc[node] + request <= pernode_avail_pages[node]) )
    return true;

> +
> +    /*
> +     * Not enough unclaimed memory. Only allow if it's already claimed on the
> +     * right node. d->claim_node == NUMA_NO_NODE if the claim isn't on an
> +     * exact node.
> +     *
> +     * Only refcounted allocs attributed to domains may have been claimed

Nit: missing full stop at the end of the sentence.

Thanks, Roger.

Re: [PATCH 07/11] xen/page_alloc: Set node affinity when claiming pages from an exact node

[Roger Pau Monné]

On Fri, Mar 14, 2025 at 05:24:58PM +0000, Alejandro Vallejo wrote:
> Set the domain's node affinity to the claimed node if the claim
> specified an exact node. Do it immediately before making any changes in
> case setting the affinity fails (even though it shouldn't).
> 
> This allows preferentially allocating from the closest NUMA node when
> "exact" is not specified (e.g: p2m tables, etc).
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
> Toolstacks can just do it themselves, but it's more error prone. If it
> claimed pages from an exact node (and remember we can only hold a single
> claim at a time) it makes no sense for the domain to be intentionally
> allocating from NUMA nodes other than its home node.
> ---
>  xen/common/page_alloc.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index cfaa64d3b858..e69a5fcc8d31 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -586,10 +586,16 @@ int domain_set_outstanding_pages(struct domain *d, nodeid_t node,
>  
>      if ( node != NUMA_NO_NODE )
>      {
> -        avail_pages = pernode_avail_pages[node] - pernode_oc[node];
> +        nodemask_t affinity = NODE_MASK_NONE;
>  
> +        avail_pages = pernode_avail_pages[node] - pernode_oc[node];
>          if ( pages > avail_pages )
>              goto out;
> +
> +        node_set(node, affinity);
> +        ret = domain_set_node_affinity(d, &affinity);

You can use nodemask_of_node(node) here?

> +        if ( ret )
> +            goto out;

This seems a bit too much, specially failing the claim if the affinity
cannot be fulfilled.

If would maybe print a message if the claim is made against a
non-affine node, but that would be it.

Thanks, Roger.

Re: [PATCH 05/11] xen: Create per-node outstanding claims

[Roger Pau Monné]

On Fri, Mar 14, 2025 at 05:24:56PM +0000, Alejandro Vallejo wrote:
> Extends domain_set_outstanding_claims() to allow staking claims on an
> exact node. Also creates global per-node claim counts analogous to
> `outstanding_claims`. Note that the per-node counts can't replace the
> global one if we want exact-node claims to coexist with non-exact
> claims.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
>  xen/common/page_alloc.c | 32 +++++++++++++++++++++++++++++++-
>  xen/include/xen/sched.h |  3 +++
>  2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 9243c4f51370..7fe574b29407 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -490,6 +490,7 @@ static unsigned long pernode_avail_pages[MAX_NUMNODES];
>  
>  static DEFINE_SPINLOCK(heap_lock);
>  static long outstanding_claims; /* total outstanding claims by all domains */
> +static unsigned long pernode_oc[MAX_NUMNODES]; /* per-node outstanding claims */
>  
>  unsigned long domain_adjust_tot_pages(struct domain *d, nodeid_t node,
>                                        long pages)
> @@ -501,20 +502,31 @@ unsigned long domain_adjust_tot_pages(struct domain *d, nodeid_t node,
>       * can test d->outstanding_pages race-free because it can only change
>       * if d->page_alloc_lock and heap_lock are both held, see also
>       * domain_set_outstanding_pages below
> +     *
> +     * If `d` has an exact-node claim, we must exit early if this is an
> +     * adjustment attributed to another node.
>       */
> -    if ( !d->outstanding_pages || pages <= 0 )
> +    if ( !d->outstanding_pages || pages <= 0 ||
> +         (d->claim_node != NUMA_NO_NODE && d->claim_node != node) )
>          goto out;
>  
> +
>      spin_lock(&heap_lock);
>      BUG_ON(outstanding_claims < d->outstanding_pages);
>      if ( d->outstanding_pages < pages )
>      {
>          /* `pages` exceeds the domain's outstanding count. Zero it out. */
> +        if ( d->claim_node != NUMA_NO_NODE )
> +            pernode_oc[d->claim_node] -= d->outstanding_pages;
> +
>          outstanding_claims -= d->outstanding_pages;
>          d->outstanding_pages = 0;
>      }
>      else
>      {
> +        if ( d->claim_node != NUMA_NO_NODE )
> +            pernode_oc[d->claim_node] -= pages;
> +
>          outstanding_claims -= pages;
>          d->outstanding_pages -= pages;
>      }
> @@ -542,6 +554,10 @@ int domain_set_outstanding_pages(struct domain *d, nodeid_t node,
>      if ( pages == 0 )
>      {
>          outstanding_claims -= d->outstanding_pages;
> +
> +        if ( d->claim_node != NUMA_NO_NODE )
> +            pernode_oc[d->claim_node] -= d->outstanding_pages;
> +
>          d->outstanding_pages = 0;
>          ret = 0;
>          goto out;
> @@ -564,12 +580,26 @@ int domain_set_outstanding_pages(struct domain *d, nodeid_t node,
>      /* how much memory is available? */
>      avail_pages = total_avail_pages - outstanding_claims;
>  
> +    /* This check can't be skipped for the NUMA case, or we may overclaim */
>      if ( pages > avail_pages )
>          goto out;
>  
> +    if ( node != NUMA_NO_NODE )
> +    {
> +        avail_pages = pernode_avail_pages[node] - pernode_oc[node];

Forgot to mention in my previous reply, should there be some sanity
check that node < MAX_NUMNODES?  It's hard to tell whether it should
be an assert or an if check, as there are no callers here that pass
node != NUMA_NO_NODE.

Thanks, Roger.

Re: [PATCH 08/11] xen/memory: Enable parsing NUMA node argument in XENMEM_claim_pages

[Roger Pau Monné]

On Fri, Mar 14, 2025 at 05:24:59PM +0000, Alejandro Vallejo wrote:
> Extract a NUMA node from mem_flags. This _must_ be an exact node, and
> has the semantics of making a claim on a specific node.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
>  xen/common/memory.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/common/memory.c b/xen/common/memory.c
> index 1ab0bac4e7da..28f9db89ba62 100644
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1659,6 +1659,9 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          break;
>  
>      case XENMEM_claim_pages:
> +    {
> +        nodeid_t node;
> +
>          if ( unlikely(start_extent) )
>              return -EINVAL;
>  
> @@ -1671,9 +1674,23 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( reservation.extent_order != 0 )
>              return -EINVAL;
>  
> -        if ( reservation.mem_flags != 0 )
> +        /* Only allow NUMA-related memory flags to claim on exact nodes */
> +        if ( (reservation.mem_flags &
> +              !XENMEMF_exact_node(XENMEMF_node_mask - 1)) != 0 )

Are you sure the above is correct?

!XENMEMF_exact_node(XENMEMF_node_mask - 1) resolves unconditionally to
false, and hence you are doing:

(reservation.mem_flags & 0) != 0

Which can never be true?

I think you want ~ instead of !.

>              return -EINVAL;
>  
> +        node = XENMEMF_get_node(reservation.mem_flags);
> +        if ( node != NUMA_NO_NODE )
> +        {
> +            /* Disallow advisory nodes. If present, must be exact */
> +            if ( !(reservation.mem_flags & XENMEMF_exact_node_request) )
> +                return -EINVAL;
> +
> +            /* Disallow nodes that would overflow the in-hypervisor arrays */
> +            if ( node >= MAX_NUMNODES )
> +                return -EINVAL;

You can likely join all checks in a single condition.

Thanks, Roger.

Re: [PATCH 10/11] tools/xl: Expose a "claim_on_node" setting in xl.cfg

[Roger Pau Monné]

On Fri, Mar 14, 2025 at 05:25:01PM +0000, Alejandro Vallejo wrote:
> Expose a setting to explicitly select a NUMA node for created domains
> 
> If the hypervisor can't reserve enough memory in the relevant NUMA node
> it fails the claim early. Also, disable automatic NUMA placement when
> this new option is enabled.

Overall it would be more helpful for users if soft-affinity was taken
into consideration when doing the claims, as that's the node where the
memory will ultimately be allocated?

Otherwise it's not very helpful IMO, as it's unlikely for the user to
know which NUMA the domain will get assigned to ahead of having
created it.

> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
>  tools/golang/xenlight/helpers.gen.go |  2 ++
>  tools/golang/xenlight/types.gen.go   |  1 +
>  tools/include/xenguest.h             |  7 +++++++
>  tools/libs/guest/xg_dom_core.c       |  1 +
>  tools/libs/guest/xg_dom_x86.c        | 18 +++++++++---------
>  tools/libs/light/libxl_dom.c         |  2 ++
>  tools/libs/light/libxl_types.idl     |  3 ++-
>  tools/xl/xl_parse.c                  | 11 +++++++++++
>  8 files changed, 35 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/golang/xenlight/helpers.gen.go b/tools/golang/xenlight/helpers.gen.go
> index 90846ea8e882..9204020224d5 100644
> --- a/tools/golang/xenlight/helpers.gen.go
> +++ b/tools/golang/xenlight/helpers.gen.go
> @@ -1108,6 +1108,7 @@ x.LlcColors[i] = uint32(v)
>  if err := x.ClaimMode.fromC(&xc.claim_mode);err != nil {
>  return fmt.Errorf("converting field ClaimMode: %v", err)
>  }
> +x.ClaimOnNode = uint32(xc.claim_on_node)
>  x.EventChannels = uint32(xc.event_channels)
>  x.Kernel = C.GoString(xc.kernel)
>  x.Cmdline = C.GoString(xc.cmdline)
> @@ -1472,6 +1473,7 @@ cLlcColors[i] = C.uint32_t(v)
>  if err := x.ClaimMode.toC(&xc.claim_mode); err != nil {
>  return fmt.Errorf("converting field ClaimMode: %v", err)
>  }
> +xc.claim_on_node = C.uint32_t(x.ClaimOnNode)
>  xc.event_channels = C.uint32_t(x.EventChannels)
>  if x.Kernel != "" {
>  xc.kernel = C.CString(x.Kernel)}
> diff --git a/tools/golang/xenlight/types.gen.go b/tools/golang/xenlight/types.gen.go
> index e7667f1ce3a3..1c6319200411 100644
> --- a/tools/golang/xenlight/types.gen.go
> +++ b/tools/golang/xenlight/types.gen.go
> @@ -577,6 +577,7 @@ Irqs []uint32
>  Iomem []IomemRange
>  LlcColors []uint32
>  ClaimMode Defbool
> +ClaimOnNode uint32
>  EventChannels uint32
>  Kernel string
>  Cmdline string
> diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
> index e01f494b772a..08aae1e24e4c 100644
> --- a/tools/include/xenguest.h
> +++ b/tools/include/xenguest.h
> @@ -185,6 +185,13 @@ struct xc_dom_image {
>      uint32_t guest_domid;
>      int claim_enabled; /* 0 by default, 1 enables it */
>  
> +    /*
> +     * Exact NUMA node on which to allocate memory from.
> +     *
> +     * XC_NUMA_NO_NODE by default.
> +     */
> +    unsigned int claim_on_node;
> +
>      int xen_version;
>      xen_capabilities_info_t xen_caps;
>  
> diff --git a/tools/libs/guest/xg_dom_core.c b/tools/libs/guest/xg_dom_core.c
> index 595b0a667c03..152ec8ea6591 100644
> --- a/tools/libs/guest/xg_dom_core.c
> +++ b/tools/libs/guest/xg_dom_core.c
> @@ -775,6 +775,7 @@ struct xc_dom_image *xc_dom_allocate(xc_interface *xch,
>      dom->parms->p2m_base = UNSET_ADDR;
>  
>      dom->flags = SIF_VIRT_P2M_4TOOLS;
> +    dom->claim_on_node = XC_NUMA_NO_NODE;
>  
>      dom->alloc_malloc += sizeof(*dom);
>      return dom;
> diff --git a/tools/libs/guest/xg_dom_x86.c b/tools/libs/guest/xg_dom_x86.c
> index ac05106a8c1c..4fe816b9bcb4 100644
> --- a/tools/libs/guest/xg_dom_x86.c
> +++ b/tools/libs/guest/xg_dom_x86.c
> @@ -1199,7 +1199,7 @@ static int meminit_pv(struct xc_dom_image *dom)
>      if ( dom->claim_enabled )
>      {
>          rc = xc_domain_claim_pages(dom->xch, dom->guest_domid,
> -                                   XC_NUMA_NO_NODE,
> +                                   dom->claim_on_node,
>                                     dom->total_pages);
>          if ( rc )
>              return rc;
> @@ -1209,9 +1209,10 @@ static int meminit_pv(struct xc_dom_image *dom)
>       * that this is a valid state if libxl doesn't provide any
>       * vNUMA information.
>       *
> -     * The dummy values make libxc allocate all pages from
> -     * arbitrary physical nodes. This is the expected behaviour if
> -     * no vNUMA configuration is provided to libxc.
> +     * If there's an outstanding claim on a node, memory is allocated from that
> +     * node. Otherwise the dummy values make libxc allocate all pages from
> +     * arbitrary physical nodes. This is the expected behaviour if no vNUMA
> +     * configuration is provided to libxc and.

Stray 'and' or truncated sentence?

>       *
>       * Note that the following hunk is just for the convenience of
>       * allocation code. No defaulting happens in libxc.
> @@ -1227,7 +1228,7 @@ static int meminit_pv(struct xc_dom_image *dom)
>  
>          nr_vnodes = 1;
>          vnode_to_pnode = dummy_vnode_to_pnode;
> -        vnode_to_pnode[0] = XC_NUMA_NO_NODE;
> +        vnode_to_pnode[0] = dom->claim_on_node;
>      }
>      else
>      {
> @@ -1357,7 +1358,6 @@ static int meminit_hvm(struct xc_dom_image *dom)
>      unsigned long stat_normal_pages = 0, stat_2mb_pages = 0,
>          stat_1gb_pages = 0;
>      unsigned int memflags = 0;
> -    int claim_enabled = dom->claim_enabled;
>      uint64_t total_pages;
>      xen_vmemrange_t dummy_vmemrange[2];
>      unsigned int dummy_vnode_to_pnode[1];
> @@ -1397,7 +1397,7 @@ static int meminit_hvm(struct xc_dom_image *dom)
>              nr_vmemranges++;
>          }
>  
> -        dummy_vnode_to_pnode[0] = XC_NUMA_NO_NODE;
> +        dummy_vnode_to_pnode[0] = dom->claim_on_node;
>          nr_vnodes = 1;
>          vmemranges = dummy_vmemrange;
>          vnode_to_pnode = dummy_vnode_to_pnode;
> @@ -1443,8 +1443,8 @@ static int meminit_hvm(struct xc_dom_image *dom)
>       * actually allocates memory for the guest. Claiming after memory has been
>       * allocated is pointless.
>       */
> -    if ( claim_enabled ) {
> -        rc = xc_domain_claim_pages(xch, domid, XC_NUMA_NO_NODE,
> +    if ( dom->claim_enabled ) {
> +        rc = xc_domain_claim_pages(xch, domid, dom->claim_on_node,
>                                     target_pages - dom->vga_hole_size);
>          if ( rc != 0 )
>          {
> diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
> index 94fef374014e..1a5500702239 100644
> --- a/tools/libs/light/libxl_dom.c
> +++ b/tools/libs/light/libxl_dom.c
> @@ -663,6 +663,7 @@ int libxl__build_pv(libxl__gc *gc, uint32_t domid,
>      dom->xenstore_evtchn = state->store_port;
>      dom->xenstore_domid = state->store_domid;
>      dom->claim_enabled = libxl_defbool_val(info->claim_mode);
> +    dom->claim_on_node = info->claim_on_node;
>      dom->max_vcpus = info->max_vcpus;
>  
>      if (info->num_vnuma_nodes != 0) {
> @@ -1090,6 +1091,7 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid,
>      mem_size = (uint64_t)(info->max_memkb - info->video_memkb) << 10;
>      dom->target_pages = (uint64_t)(info->target_memkb - info->video_memkb) >> 2;
>      dom->claim_enabled = libxl_defbool_val(info->claim_mode);
> +    dom->claim_on_node = info->claim_on_node;
>      if (info->u.hvm.mmio_hole_memkb) {
>          uint64_t max_ram_below_4g = (1ULL << 32) -
>              (info->u.hvm.mmio_hole_memkb << 10);
> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> index bd4b8721ff19..a252b36c2b5d 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -617,7 +617,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
>      ("irqs",             Array(uint32, "num_irqs")),
>      ("iomem",            Array(libxl_iomem_range, "num_iomem")),
>      ("llc_colors",       Array(uint32, "num_llc_colors")),
> -    ("claim_mode",	     libxl_defbool),
> +    ("claim_mode",       libxl_defbool),
> +    ("claim_on_node",    uint32),
>      ("event_channels",   uint32),
>      ("kernel",           string),
>      ("cmdline",          string),
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 089a88935aff..5880a032e6d6 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1558,6 +1558,17 @@ void parse_config_data(const char *config_source,
>          exit(1);
>  
>      libxl_defbool_set(&b_info->claim_mode, claim_mode);
> +    e = xlu_cfg_get_bounded_long (config, "claim_on_node", 0,

You seem to introduce a new xl.cfg option, but there are no man page
changes as part of the patch?

> +                                  254, &l, 1);
> +    if (e == ESRCH) /* not specified */
> +        b_info->claim_on_node = ~0U;
> +    else if (!e) {
> +        libxl_defbool_set(&b_info->numa_placement, false);
> +        libxl_defbool_set(&b_info->claim_mode, true);
> +        b_info->claim_on_node = l;
> +    }

This setting of defaults should better be done in
libxl__domain_build_info_setdefault() IMO.

Thanks, Roger.

Re: [PATCH 09/11] tools/xc: Add `node` argument to xc_domain_claim_pages()

[Roger Pau Monné]

On Fri, Mar 14, 2025 at 05:25:00PM +0000, Alejandro Vallejo wrote:
> Add a node argument to make an exact-node claim. NUMA_NO_NODE means to
> make a regular any-node claim.
> 
> No functional change.
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
>  tools/include/xenctrl.h       |  1 +
>  tools/libs/ctrl/xc_domain.c   | 13 ++++++++++++-
>  tools/libs/guest/xg_dom_x86.c |  8 +++++---
>  3 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index 495598123133..2b63992e1246 100644
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -1322,6 +1322,7 @@ int xc_domain_populate_physmap_exact(xc_interface *xch,
>  
>  int xc_domain_claim_pages(xc_interface *xch,
>                                 uint32_t domid,
> +                               unsigned int node,
>                                 unsigned long nr_pages);
>  
>  int xc_domain_memory_exchange_pages(xc_interface *xch,
> diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
> index 2ddc3f4f426d..f2c6eda875dc 100644
> --- a/tools/libs/ctrl/xc_domain.c
> +++ b/tools/libs/ctrl/xc_domain.c
> @@ -20,6 +20,7 @@
>   */
>  
>  #include "xc_private.h"
> +#include "xenguest.h"
>  #include <xen/memory.h>
>  #include <xen/hvm/hvm_op.h>
>  
> @@ -1072,13 +1073,23 @@ int xc_domain_remove_from_physmap(xc_interface *xch,
>  
>  int xc_domain_claim_pages(xc_interface *xch,
>                                 uint32_t domid,
> +                               unsigned int node,
>                                 unsigned long nr_pages)
>  {
>      int err;
> +    unsigned int mem_flags = 0;
> +
> +    if ( node != XC_NUMA_NO_NODE )
> +    {
> +        if ( node >= 0xFF )

You should have XENMEMF_node_mask available here which is better than
open-coding 0xff.

Thanks, Roger.

Re: [PATCH 11/11] docs/man: Document the new claim_on_node option

[Roger Pau Monné]

On Fri, Mar 14, 2025 at 05:25:02PM +0000, Alejandro Vallejo wrote:
> ... and while at it, add missing relevant links to xl-numa-placement(7)
> in xl.1.pod.in and xl.cfg.5.pod.in, which describes libxl's behaviour on
> NUMA placement.

Oh, here it is.  This should be part of the previous patch.

I still have my concerns of the option being generally helpful, as
said in the previous patch it would be better if this new per-node
claim mode was somehow tied with the soft-affinity placement by
default.

Thanks, Roger.

Re: [PATCH 02/11] xen/page_alloc: Remove `claim` from domain_set_outstanding_pages()

[Jan Beulich]

On 05.06.2025 18:42, Roger Pau Monné wrote:
> On Fri, Mar 14, 2025 at 05:24:53PM +0000, Alejandro Vallejo wrote:
>> With a single global count for the claims it's easy to substract
>> domain_tot_pages() from the claim so the number given in the hypercall
>> is the real reservation of the domain. This is the current behaviour.
>>
>> However, a later patch introduces exact-node claims and those interact
>> very poorly with such a scheme. Since accounting domain_tot_pages() in
>> one case but not the other seems strictly worse than not accounting them
>> at all (which is at least consistent), this patch stops substracting
>> tot_pages from the claim and instead checks that claimed memory +
>> allocated memory don't exceed max_mem.
> 
> Hm, while I don't have any specific interest in keeping the current
> behavior, XENMEM_claim_pages is part of the stable ABI (it's not a
> domctl), and hence should be stable.

Is that true? It sits inside a

#if defined(__XEN__) || defined(__XEN_TOOLS__)

which generally de-marks unstable (tools-only) interfaces.

>  Note also the comment above the
> definition of XENMEM_claim_pages how it states the specific behavior
> that you are trying to change (and which should have been adjusted as
> part of this change).

This is the more important part, imo.

> I have no idea why this was made a xenmem rather than a domctl
> hypercall, but if you want to change the semantics I think the only
> option is introducing a new hypercall.

It _is_ a memory operation, and it re-uses one of the interface structs
there. (Yes, none of these would technically have prevented it from
being a domctl.)

Jan

Re: [PATCH 02/11] xen/page_alloc: Remove `claim` from domain_set_outstanding_pages()

[Jan Beulich]

On 14.03.2025 18:24, Alejandro Vallejo wrote:
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -523,7 +523,7 @@ out:
>  int domain_set_outstanding_pages(struct domain *d, unsigned long pages)
>  {
>      int ret = -ENOMEM;
> -    unsigned long claim, avail_pages;
> +    unsigned long avail_pages;
>  
>      /*
>       * take the domain's page_alloc_lock, else all d->tot_page adjustments
> @@ -549,28 +549,21 @@ int domain_set_outstanding_pages(struct domain *d, unsigned long pages)
>          goto out;
>      }
>  
> -    /* disallow a claim not exceeding domain_tot_pages() or above max_pages */
> -    if ( (pages <= domain_tot_pages(d)) || (pages > d->max_pages) )
> +    /* Don't claim past max_pages */
> +    if ( (domain_tot_pages(d) + pages) > d->max_pages )

In addition to what Roger has said, how can such a behavioral change come without
any caller-side adjustment?

Jan

Re: [PATCH 02/11] xen/page_alloc: Remove `claim` from domain_set_outstanding_pages()

[Roger Pau Monné]

On Tue, Jun 10, 2025 at 02:23:26PM +0200, Jan Beulich wrote:
> On 05.06.2025 18:42, Roger Pau Monné wrote:
> > On Fri, Mar 14, 2025 at 05:24:53PM +0000, Alejandro Vallejo wrote:
> >> With a single global count for the claims it's easy to substract
> >> domain_tot_pages() from the claim so the number given in the hypercall
> >> is the real reservation of the domain. This is the current behaviour.
> >>
> >> However, a later patch introduces exact-node claims and those interact
> >> very poorly with such a scheme. Since accounting domain_tot_pages() in
> >> one case but not the other seems strictly worse than not accounting them
> >> at all (which is at least consistent), this patch stops substracting
> >> tot_pages from the claim and instead checks that claimed memory +
> >> allocated memory don't exceed max_mem.
> > 
> > Hm, while I don't have any specific interest in keeping the current
> > behavior, XENMEM_claim_pages is part of the stable ABI (it's not a
> > domctl), and hence should be stable.
> 
> Is that true? It sits inside a
> 
> #if defined(__XEN__) || defined(__XEN_TOOLS__)
> 
> which generally de-marks unstable (tools-only) interfaces.

Ops, my bad, I didn't realize it was inside such region.

> >  Note also the comment above the
> > definition of XENMEM_claim_pages how it states the specific behavior
> > that you are trying to change (and which should have been adjusted as
> > part of this change).
> 
> This is the more important part, imo.

I see.  Well, it's in a kind of a weird position then, because there's
no equivalent of XEN_DOMCTL_INTERFACE_VERSION that we could use to
signal callers of the changed interface, like we do for domctl.

Thanks, Roger.

Re: [PATCH 02/11] xen/page_alloc: Remove `claim` from domain_set_outstanding_pages()

[Alejandro Vallejo]

On Tue Jun 10, 2025 at 2:23 PM CEST, Jan Beulich wrote:
> On 05.06.2025 18:42, Roger Pau Monné wrote:
>> On Fri, Mar 14, 2025 at 05:24:53PM +0000, Alejandro Vallejo wrote:
>>> With a single global count for the claims it's easy to substract
>>> domain_tot_pages() from the claim so the number given in the hypercall
>>> is the real reservation of the domain. This is the current behaviour.
>>>
>>> However, a later patch introduces exact-node claims and those interact
>>> very poorly with such a scheme. Since accounting domain_tot_pages() in
>>> one case but not the other seems strictly worse than not accounting them
>>> at all (which is at least consistent), this patch stops substracting
>>> tot_pages from the claim and instead checks that claimed memory +
>>> allocated memory don't exceed max_mem.
>> 
>> Hm, while I don't have any specific interest in keeping the current
>> behavior, XENMEM_claim_pages is part of the stable ABI (it's not a
>> domctl), and hence should be stable.
>
> Is that true? It sits inside a
>
> #if defined(__XEN__) || defined(__XEN_TOOLS__)
>
> which generally de-marks unstable (tools-only) interfaces.
>
>>  Note also the comment above the
>> definition of XENMEM_claim_pages how it states the specific behavior
>> that you are trying to change (and which should have been adjusted as
>> part of this change).
>
> This is the more important part, imo.

Ugh. I missed that. Regardless, the current scheme is highly counterintuitive
and works only because toolstack colaborates in removing the claim after dom
creation.

>
>> I have no idea why this was made a xenmem rather than a domctl
>> hypercall, but if you want to change the semantics I think the only
>> option is introducing a new hypercall.
>
> It _is_ a memory operation, and it re-uses one of the interface structs
> there. (Yes, none of these would technically have prevented it from
> being a domctl.)
>
> Jan

As Jan mentions, my understanding was that __XEN_TOOLS__ effectively meant
unstable. It is quite hard to keep the previous semantics for one hypercall
while introducing a new one that behaves slightly differently.

Cheers,
Alejandro

Re: [PATCH 03/11] xen/page_alloc: Add static per-node counts of free pages

[Jan Beulich]

On 14.03.2025 18:24, Alejandro Vallejo wrote:
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -485,6 +485,9 @@ static unsigned long node_need_scrub[MAX_NUMNODES];
>  static unsigned long *avail[MAX_NUMNODES];
>  static long total_avail_pages;
>  
> +/* Per-node counts of free pages */
> +static unsigned long pernode_avail_pages[MAX_NUMNODES];

Performance-wise I find concerning any addition of yet another array like
this one, which - unlike e.g. avail[] - is randomly written from all nodes.
I think we either need to introduce per_node() paralleling per_cpu(), or
(less desirable, but perhaps easier to carry out) data structures need to
be introduced to make sure one particular cache line would only ever be
written from a single node. As it's visible from patch context:
node_need_scrub[] could/should move there, too, for example.

Jan

Re: [PATCH 04/11] xen: Add node argument to domain_{adjust_tot_pages,set_outstanding_pages}()

[Jan Beulich]

On 14.03.2025 18:24, Alejandro Vallejo wrote:
> domain_adjust_tot_pages() decreases the outstanding claims of a domain
> as pages are allocated, so that'll need to take into account the node in
> which an allocation is done. Deallocations just pass NUMA_NO_NODE.
> 
> domain_set_outstanding_pages() takes the node on which to to stake an
> exact-node claim, or NUMA_NO_NODE if it's a non-exact claim.

This doesn't fit the code, where you make both callers pass NUMA_NO_NODE.
With that it's hard to see why ...

> Not a functional change, as neither function uses the arguments for
> anything yet. It's a prerequisite to simplify for the following patch
> that introduces per-node claim counts.

... this part of the change would simplify further changes: Any actual
use of the parameter would be meaningless as long as no caller passes a
valid node ID. IOW it looks to me as if this part of the change wants
moving elsewhere.

Jan

Re: [PATCH 07/11] xen/page_alloc: Set node affinity when claiming pages from an exact node

[Jan Beulich]

On 14.03.2025 18:24, Alejandro Vallejo wrote:
> Set the domain's node affinity to the claimed node if the claim
> specified an exact node. Do it immediately before making any changes in
> case setting the affinity fails (even though it shouldn't).
> 
> This allows preferentially allocating from the closest NUMA node when
> "exact" is not specified (e.g: p2m tables, etc).
> 
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
> Toolstacks can just do it themselves, but it's more error prone. If it
> claimed pages from an exact node (and remember we can only hold a single
> claim at a time) it makes no sense for the domain to be intentionally
> allocating from NUMA nodes other than its home node.

I question this as a global, built-in policy: An admin may intentionally
do things in different ways, for whatever reasons they may have. Even in
the toolstack I'd consider such behavior a valid default, but requiring
a way to override.

Jan