[PATCH 0/2] x86/numa: introduce per-node flush_lock

[PATCH 0/2] x86/numa: introduce per-node flush_lock

[Roger Pau Monne]

Hello,

I've had this patches pending for quite some time, but never sent them.
Kind of RFC because I'm unsure whether the approach is the best, there's
however a need to split the flush_lock, as on hosts with a high number
of CUPs there's there's a non-trivial amount of contention around it.

First patch is a preparatory change to allow using per-NUMA node locks,
second patch introduces a per-node flush_lock.

Thanks, Roger.

Roger Pau Monne (2):
  x86/numa: add per-node lock profile objects
  x86/numa: introduce per-NUMA node flush locks

 tools/misc/xenlockprof.c               |  5 ++
 xen/arch/x86/include/asm/irq-vectors.h |  9 ++-
 xen/arch/x86/include/asm/numa.h        |  3 +
 xen/arch/x86/include/asm/smp.h         |  3 +
 xen/arch/x86/numa.c                    | 95 ++++++++++++++++++++++++++
 xen/arch/x86/smp.c                     | 16 +++--
 xen/arch/x86/smpboot.c                 |  3 +
 xen/common/numa.c                      | 23 +++++++
 xen/common/spinlock.c                  |  1 +
 xen/include/public/sysctl.h            |  3 +-
 xen/include/xen/numa.h                 | 13 ++++
 11 files changed, 168 insertions(+), 6 deletions(-)

-- 
2.49.0

[PATCH 1/2] x86/numa: add per-node lock profile objects

[Roger Pau Monne]

Add some basic infrastructure to be able to use lockprofile with per NUMA
node locks.  This patch just introduces the required types, plus the
printing of the data for the newly introduced type.  There's no user of
per NUMA node locks introduced here.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/misc/xenlockprof.c    | 5 +++++
 xen/common/spinlock.c       | 1 +
 xen/include/public/sysctl.h | 3 ++-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/misc/xenlockprof.c b/tools/misc/xenlockprof.c
index 11f43a35e3c0..3fc91e832ae5 100644
--- a/tools/misc/xenlockprof.c
+++ b/tools/misc/xenlockprof.c
@@ -96,6 +96,11 @@ int main(int argc, char *argv[])
         case LOCKPROF_TYPE_PERDOM:
             sprintf(name, "domain %d lock %s", data[j].idx, data[j].name);
             break;
+
+        case LOCKPROF_TYPE_PERNODE:
+            sprintf(name, "NUMA node %d lock %s", data[j].idx, data[j].name);
+            break;
+
         default:
             sprintf(name, "unknown type(%d) %d lock %s", data[j].type,
                     data[j].idx, data[j].name);
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 38caa10a2ea2..d86e32bd67aa 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -612,6 +612,7 @@ static s_time_t lock_profile_start;
 static struct lock_profile_anc lock_profile_ancs[] = {
     [LOCKPROF_TYPE_GLOBAL] = { .name = "Global" },
     [LOCKPROF_TYPE_PERDOM] = { .name = "Domain" },
+    [LOCKPROF_TYPE_PERNODE] = { .name = "NUMA node" },
 };
 static struct lock_profile_qhead lock_profile_glb_q;
 static spinlock_t lock_profile_lock = SPIN_LOCK_UNLOCKED;
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 9eca72865b87..bcf3dd3b1f31 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -582,7 +582,8 @@ struct xen_sysctl_page_offline_op {
 /* Record-type: */
 #define LOCKPROF_TYPE_GLOBAL      0   /* global lock, idx meaningless */
 #define LOCKPROF_TYPE_PERDOM      1   /* per-domain lock, idx is domid */
-#define LOCKPROF_TYPE_N           2   /* number of types */
+#define LOCKPROF_TYPE_PERNODE     2   /* pNUMA per-node lock, idx is node ID */
+#define LOCKPROF_TYPE_N           3   /* number of types */
 struct xen_sysctl_lockprof_data {
     char     name[40];     /* lock name (may include up to 2 %d specifiers) */
     int32_t  type;         /* LOCKPROF_TYPE_??? */
-- 
2.49.0

Re: [PATCH 1/2] x86/numa: add per-node lock profile objects

[Jan Beulich]

On 22.05.2025 10:48, Roger Pau Monne wrote:
> Add some basic infrastructure to be able to use lockprofile with per NUMA
> node locks.  This patch just introduces the required types, plus the
> printing of the data for the newly introduced type.  There's no user of
> per NUMA node locks introduced here.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  tools/misc/xenlockprof.c    | 5 +++++
>  xen/common/spinlock.c       | 1 +
>  xen/include/public/sysctl.h | 3 ++-
>  3 files changed, 8 insertions(+), 1 deletion(-)

Just one nit: Nothing here is x86-specific, so the subject prefix may want
adjusting.

Jan

[PATCH 2/2] x86/numa: introduce per-NUMA node flush locks

[Roger Pau Monne]

Contention around the global flush_lock increases as the amount of physical
CPUs on the host also increases.  Sadly this doesn't scale on big boxes.
However most of the time Xen doesn't require broadcasting flushes to all
CPUs on the system, and hence more fine grained (ie: covering less CPUs)
locks could be used.

A natural boundary to use when splitting the locks are NUMA nodes.  Most
domains will be limited to running on a single node, specifically the one
where the domain memory has been allocated from.  Flushes related to
domains are most likely to be limited to a single NUMA node, and hence
being able to execute per-node flushes allows to reduce the contention
around the global flush_lock, while also allowing to perform concurrent
flushes on different nodes.

This however doesn't come for free.  A new vector must be allocated to be
used for the per-NUMA flushes, and more logic is required in the flush
dispatcher to figure out whether a flush is limited to a single node.

The figures on a 2-node NUMA system are as follows, after having been
running the same XenRT boot storm workload for 90 minutes.

Without the per-NUMA node flush:

Global flush_lock: addr=ffff82d040837340, lockval=d8ded8de, not locked
  lock:21878876(98178042228), block:1603338(6043805110)

So a total block time of ~6s, and average block time of 3.7us.

With the per-node locks:

Global flush_lock: addr=ffff82d040837360, lockval=78e678e6, not locked
  lock:6781028(41032945811), block:583712(2025657239)
NUMA node 1 flush_lock: addr=ffff832fd085b110, lockval=5cd65cd6, not locked
  lock:220374(766500536), block:4091(9933129)
NUMA node 0 flush_lock: addr=ffff8310336a7110, lockval=5c715c71, not locked
  lock:547953(1658170241), block:23856(51266297)

The total block time goes down to ~2s, and the average block time is 3.4us.
The total block time of the per-node locks is much lower compared to the
global flush_lock, 9ms and 51ms respectively.

Note the example here is possibly the system where such split locks don't
make a lot of difference, being a 2 node system, but still there's a
non-trivial difference between the block times.  On a 4 or 9 node system
the figures should likely be even better.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Kind of RFC, had this patch pending for some time on my queue.  Before
working further on this I would like to make sure the approach is OK.
---
 xen/arch/x86/include/asm/irq-vectors.h |  9 ++-
 xen/arch/x86/include/asm/numa.h        |  3 +
 xen/arch/x86/include/asm/smp.h         |  3 +
 xen/arch/x86/numa.c                    | 95 ++++++++++++++++++++++++++
 xen/arch/x86/smp.c                     | 16 +++--
 xen/arch/x86/smpboot.c                 |  3 +
 xen/common/numa.c                      | 23 +++++++
 xen/include/xen/numa.h                 | 13 ++++
 8 files changed, 160 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/include/asm/irq-vectors.h b/xen/arch/x86/include/asm/irq-vectors.h
index f546aedd87de..3b8fee7e22cf 100644
--- a/xen/arch/x86/include/asm/irq-vectors.h
+++ b/xen/arch/x86/include/asm/irq-vectors.h
@@ -9,12 +9,19 @@
 #define CALL_FUNCTION_VECTOR	0xfb
 #define LOCAL_TIMER_VECTOR	0xfa
 #define PMU_APIC_VECTOR 	0xf9
+
+#ifdef CONFIG_NUMA
+# define INVALIDATE_NUMA_VECTOR  0xf8
+# define LAST_HIPRIORITY_VECTOR  0xf7
+#else /* !CONFIG_NUMA */
+# define LAST_HIPRIORITY_VECTOR  0xf8
+#endif /* !CONFIG_NUMA */
+
 /*
  * High-priority dynamically-allocated vectors. For interrupts that
  * must be higher priority than any guest-bound interrupt.
  */
 #define FIRST_HIPRIORITY_VECTOR	0xf1
-#define LAST_HIPRIORITY_VECTOR  0xf8
 /* IRQ0 (timer) is statically allocated but must be high priority. */
 #define IRQ0_VECTOR             0xf0
 
diff --git a/xen/arch/x86/include/asm/numa.h b/xen/arch/x86/include/asm/numa.h
index 7866afa40860..e68c2d9d9bea 100644
--- a/xen/arch/x86/include/asm/numa.h
+++ b/xen/arch/x86/include/asm/numa.h
@@ -25,4 +25,7 @@ void srat_parse_regions(paddr_t addr);
 extern u8 __node_distance(nodeid_t a, nodeid_t b);
 unsigned int arch_get_dma_bitsize(void);
 
+bool flush_numa_node(const cpumask_t *mask, const void *va, unsigned int flags);
+void cf_check invalidate_tbl_numa(void);
+
 #endif
diff --git a/xen/arch/x86/include/asm/smp.h b/xen/arch/x86/include/asm/smp.h
index c8c79601343d..1e6972fd7b39 100644
--- a/xen/arch/x86/include/asm/smp.h
+++ b/xen/arch/x86/include/asm/smp.h
@@ -79,6 +79,9 @@ extern bool unaccounted_cpus;
 
 void *cpu_alloc_stack(unsigned int cpu);
 
+void invalidate_tlb_handler(const void *va, unsigned int flags,
+                            cpumask_t *mask);
+
 #endif /* !__ASSEMBLY__ */
 
 #endif
diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
index ae3cc7a8d060..0139fe5377ba 100644
--- a/xen/arch/x86/numa.c
+++ b/xen/arch/x86/numa.c
@@ -8,8 +8,11 @@
 #include <xen/mm.h>
 #include <xen/nodemask.h>
 #include <xen/numa.h>
+#include <xen/xvmalloc.h>
+
 #include <asm/acpi.h>
 #include <asm/e820.h>
+#include <asm/irq-vectors.h>
 
 #ifndef Dprintk
 #define Dprintk(x...)
@@ -126,3 +129,95 @@ int __init arch_get_ram_range(unsigned int idx, paddr_t *start, paddr_t *end)
 
     return 0;
 }
+
+static struct arch_numa_node {
+    const void *flush_va;
+    unsigned int flush_flags;
+    cpumask_t flush_mask;
+    spinlock_t flush_lock;
+    struct lock_profile_qhead profile_head;
+} *node_info[MAX_NUMNODES];
+
+static int __init cf_check arch_numa_init(void)
+{
+    unsigned int i;
+
+    if ( num_online_nodes() == 1 )
+        return 0;
+
+    for_each_online_node ( i )
+    {
+        struct arch_numa_node *node =
+            alloc_xenheap_pages(get_order_from_bytes(sizeof(*node)),
+                                                     MEMF_node(i));
+
+        if ( node )
+            clear_page(node);
+        else
+            node = xvzalloc(typeof(*node));
+
+        if ( !node )
+        {
+            printk(XENLOG_WARNING
+                   "failed to allocate NUMA info struct for node %u\n", i);
+            return 0;
+        }
+
+        spin_lock_init(&node->flush_lock);
+        lock_profile_register_struct(LOCKPROF_TYPE_PERNODE, node, i);
+        spin_lock_init_prof(node, flush_lock);
+        node_info[i] = node;
+    }
+
+    return 0;
+}
+__initcall(arch_numa_init);
+
+void cf_check invalidate_tbl_numa(void)
+{
+    unsigned int cpu = smp_processor_id();
+    nodeid_t node = cpu_to_node[cpu];
+    struct arch_numa_node *info = node_info[node];
+
+    ASSERT(info);
+
+    invalidate_tlb_handler(info->flush_va, info->flush_flags,
+                           &info->flush_mask);
+}
+
+bool flush_numa_node(const cpumask_t *mask, const void *va, unsigned int flags)
+{
+    nodeid_t node = num_online_nodes() > 1 ? cpumask_to_node(mask)
+                                           : NUMA_NO_NODE;
+    struct arch_numa_node *info;
+
+    if ( node == NUMA_NO_NODE )
+        return false;
+
+    info = node_info[node];
+
+    if ( !info )
+        return false;
+
+    spin_lock(&info->flush_lock);
+    cpumask_and(&info->flush_mask, mask, &cpu_online_map);
+    cpumask_clear_cpu(smp_processor_id(), &info->flush_mask);
+    info->flush_va = va;
+    info->flush_flags = flags;
+    send_IPI_mask(&info->flush_mask, INVALIDATE_NUMA_VECTOR);
+    while ( !cpumask_empty(&info->flush_mask) )
+        cpu_relax();
+    spin_unlock(&info->flush_lock);
+
+    return true;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/smp.c b/xen/arch/x86/smp.c
index 516dab5528c1..06acc5843f9a 100644
--- a/xen/arch/x86/smp.c
+++ b/xen/arch/x86/smp.c
@@ -247,16 +247,21 @@ static cpumask_t flush_cpumask;
 static const void *flush_va;
 static unsigned int flush_flags;
 
-void cf_check invalidate_interrupt(void)
+void invalidate_tlb_handler(const void *va, unsigned int flags,
+                            cpumask_t *mask)
 {
-    unsigned int flags = flush_flags;
     ack_APIC_irq();
     perfc_incr(ipis);
     if ( (flags & FLUSH_VCPU_STATE) && __sync_local_execstate() )
         flags &= ~(FLUSH_TLB | FLUSH_TLB_GLOBAL | FLUSH_ROOT_PGTBL);
     if ( flags & ~(FLUSH_VCPU_STATE | FLUSH_ORDER_MASK) )
-        flush_area_local(flush_va, flags);
-    cpumask_clear_cpu(smp_processor_id(), &flush_cpumask);
+        flush_area_local(va, flags);
+    cpumask_clear_cpu(smp_processor_id(), mask);
+}
+
+void cf_check invalidate_interrupt(void)
+{
+    invalidate_tlb_handler(flush_va, flush_flags, &flush_cpumask);
 }
 
 void flush_area_mask(const cpumask_t *mask, const void *va, unsigned int flags)
@@ -281,6 +286,9 @@ void flush_area_mask(const cpumask_t *mask, const void *va, unsigned int flags)
              !hypervisor_flush_tlb(mask, va, flags) )
             return;
 
+        if ( flush_numa_node(mask, va, flags) )
+            return;
+
         spin_lock(&flush_lock);
         cpumask_and(&flush_cpumask, mask, &cpu_online_map);
         cpumask_clear_cpu(cpu, &flush_cpumask);
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 50c5674555e4..eb8600eb008f 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -1451,4 +1451,7 @@ void __init smp_intr_init(void)
     set_direct_apic_vector(EVENT_CHECK_VECTOR, event_check_interrupt);
     set_direct_apic_vector(INVALIDATE_TLB_VECTOR, invalidate_interrupt);
     set_direct_apic_vector(CALL_FUNCTION_VECTOR, call_function_interrupt);
+#ifdef CONFIG_NUMA
+    set_direct_apic_vector(INVALIDATE_NUMA_VECTOR, invalidate_tbl_numa);
+#endif
 }
diff --git a/xen/common/numa.c b/xen/common/numa.c
index ad75955a1622..b9bb8628fb6b 100644
--- a/xen/common/numa.c
+++ b/xen/common/numa.c
@@ -689,6 +689,29 @@ static int __init cf_check numa_setup(const char *opt)
 }
 custom_param("numa", numa_setup);
 
+/*
+ * Return the NUMA node index if all CPUs in the mask belong to the same node,
+ * otherwise return NUMA_NO_NODE.
+ */
+nodeid_t cpumask_to_node(const cpumask_t *mask)
+{
+    unsigned int cpu;
+    nodeid_t node = NUMA_NO_NODE;
+
+    if ( num_online_nodes() == 1 )
+        return cpu_to_node[0];
+
+    for_each_cpu ( cpu, mask )
+    {
+        if ( node == NUMA_NO_NODE )
+            node = cpu_to_node[cpu];
+        else if ( node != cpu_to_node[cpu] )
+            return NUMA_NO_NODE;
+    }
+
+    return node;
+}
+
 static void cf_check dump_numa(unsigned char key)
 {
     s_time_t now = NOW();
diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
index f6c1f27ca105..256dfbcf85f8 100644
--- a/xen/include/xen/numa.h
+++ b/xen/include/xen/numa.h
@@ -128,6 +128,8 @@ extern bool numa_update_node_memblks(nodeid_t node, unsigned int arch_nid,
                                      paddr_t start, paddr_t size, bool hotplug);
 extern void numa_set_processor_nodes_parsed(nodeid_t node);
 
+extern nodeid_t cpumask_to_node(const cpumask_t *mask);
+
 #else
 
 /* Fake one node for now. See also node_online_map. */
@@ -148,6 +150,17 @@ static inline nodeid_t mfn_to_nid(mfn_t mfn)
     return 0;
 }
 
+static inline nodeid_t cpumask_to_node(const cpumask_t *mask)
+{
+    return NUMA_NO_NODE;
+}
+
+#ifdef CONFIG_X86
+static inline bool flush_numa_node(const cpumask_t *mask, const void *va,
+                                   unsigned int flags)
+{ return false; }
+#endif /* CONFIG_X86 */
+
 #endif
 
 #define page_to_nid(pg) mfn_to_nid(page_to_mfn(pg))
-- 
2.49.0

Re: [PATCH 2/2] x86/numa: introduce per-NUMA node flush locks

[Jan Beulich]

On 22.05.2025 10:48, Roger Pau Monne wrote:
> Contention around the global flush_lock increases as the amount of physical
> CPUs on the host also increases.  Sadly this doesn't scale on big boxes.
> However most of the time Xen doesn't require broadcasting flushes to all
> CPUs on the system, and hence more fine grained (ie: covering less CPUs)
> locks could be used.
> 
> A natural boundary to use when splitting the locks are NUMA nodes.  Most
> domains will be limited to running on a single node, specifically the one
> where the domain memory has been allocated from.  Flushes related to
> domains are most likely to be limited to a single NUMA node, and hence
> being able to execute per-node flushes allows to reduce the contention
> around the global flush_lock, while also allowing to perform concurrent
> flushes on different nodes.
> 
> This however doesn't come for free.  A new vector must be allocated to be
> used for the per-NUMA flushes, and more logic is required in the flush
> dispatcher to figure out whether a flush is limited to a single node.
> 
> The figures on a 2-node NUMA system are as follows, after having been
> running the same XenRT boot storm workload for 90 minutes.
> 
> Without the per-NUMA node flush:
> 
> Global flush_lock: addr=ffff82d040837340, lockval=d8ded8de, not locked
>   lock:21878876(98178042228), block:1603338(6043805110)
> 
> So a total block time of ~6s, and average block time of 3.7us.
> 
> With the per-node locks:
> 
> Global flush_lock: addr=ffff82d040837360, lockval=78e678e6, not locked
>   lock:6781028(41032945811), block:583712(2025657239)
> NUMA node 1 flush_lock: addr=ffff832fd085b110, lockval=5cd65cd6, not locked
>   lock:220374(766500536), block:4091(9933129)
> NUMA node 0 flush_lock: addr=ffff8310336a7110, lockval=5c715c71, not locked
>   lock:547953(1658170241), block:23856(51266297)
> 
> The total block time goes down to ~2s, and the average block time is 3.4us.
> The total block time of the per-node locks is much lower compared to the
> global flush_lock, 9ms and 51ms respectively.
> 
> Note the example here is possibly the system where such split locks don't
> make a lot of difference, being a 2 node system, but still there's a
> non-trivial difference between the block times.  On a 4 or 9 node system
> the figures should likely be even better.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

The savings must be from TLB flushes only? Cache flushes, as we have discussed
recently, need to be global anyway, and hence there are no savings to be had.
This may want reflecting the subject and/or description.

> @@ -126,3 +129,95 @@ int __init arch_get_ram_range(unsigned int idx, paddr_t *start, paddr_t *end)
>  
>      return 0;
>  }
> +
> +static struct arch_numa_node {
> +    const void *flush_va;
> +    unsigned int flush_flags;
> +    cpumask_t flush_mask;
> +    spinlock_t flush_lock;
> +    struct lock_profile_qhead profile_head;
> +} *node_info[MAX_NUMNODES];

__ro_after_init or at least __read_mostly?

> +static int __init cf_check arch_numa_init(void)
> +{
> +    unsigned int i;
> +
> +    if ( num_online_nodes() == 1 )
> +        return 0;
> +
> +    for_each_online_node ( i )
> +    {
> +        struct arch_numa_node *node =
> +            alloc_xenheap_pages(get_order_from_bytes(sizeof(*node)),
> +                                                     MEMF_node(i));

A full page for what may cover just a cacheline or two? I realize ...

> +        if ( node )
> +            clear_page(node);
> +        else
> +            node = xvzalloc(typeof(*node));

... this (sadly) still has no NUMA-capable counterpart, but I'd have expected
at least a brief comment to justify.

> +void cf_check invalidate_tbl_numa(void)

Nit: s/tbl/tlb/ ?

> +bool flush_numa_node(const cpumask_t *mask, const void *va, unsigned int flags)
> +{
> +    nodeid_t node = num_online_nodes() > 1 ? cpumask_to_node(mask)
> +                                           : NUMA_NO_NODE;
> +    struct arch_numa_node *info;
> +
> +    if ( node == NUMA_NO_NODE )
> +        return false;
> +
> +    info = node_info[node];
> +
> +    if ( !info )
> +        return false;
> +
> +    spin_lock(&info->flush_lock);
> +    cpumask_and(&info->flush_mask, mask, &cpu_online_map);
> +    cpumask_clear_cpu(smp_processor_id(), &info->flush_mask);

IOW this function is strictly a helper of flush_area_mask(), relying on
the local CPU to have had its flushing done already. Might it not be
better if this was put as a static in smp.c then? Or, to keep struct
arch_numa_node local to this file, split it into two parts?

> +    info->flush_va = va;
> +    info->flush_flags = flags;
> +    send_IPI_mask(&info->flush_mask, INVALIDATE_NUMA_VECTOR);

This one similarly depends on flush_area_mask() behavior, not calling here
when mask has solely the local CPU set.

> --- a/xen/common/numa.c
> +++ b/xen/common/numa.c
> @@ -689,6 +689,29 @@ static int __init cf_check numa_setup(const char *opt)
>  }
>  custom_param("numa", numa_setup);
>  
> +/*
> + * Return the NUMA node index if all CPUs in the mask belong to the same node,
> + * otherwise return NUMA_NO_NODE.
> + */
> +nodeid_t cpumask_to_node(const cpumask_t *mask)
> +{
> +    unsigned int cpu;
> +    nodeid_t node = NUMA_NO_NODE;
> +
> +    if ( num_online_nodes() == 1 )
> +        return cpu_to_node[0];

The sole caller checks this already, so strictly speaking Misra would consider
this dead / unreachable code.

Similarly Misra may not like this on Arm, where it's unused right now. Then
again looks like NUMA work there was abandoned, so for now all ought to be
fine here.

Jan

Re: [PATCH 2/2] x86/numa: introduce per-NUMA node flush locks

[Roger Pau Monné]

On Wed, Jun 04, 2025 at 02:35:01PM +0200, Jan Beulich wrote:
> On 22.05.2025 10:48, Roger Pau Monne wrote:
> > Contention around the global flush_lock increases as the amount of physical
> > CPUs on the host also increases.  Sadly this doesn't scale on big boxes.
> > However most of the time Xen doesn't require broadcasting flushes to all
> > CPUs on the system, and hence more fine grained (ie: covering less CPUs)
> > locks could be used.
> > 
> > A natural boundary to use when splitting the locks are NUMA nodes.  Most
> > domains will be limited to running on a single node, specifically the one
> > where the domain memory has been allocated from.  Flushes related to
> > domains are most likely to be limited to a single NUMA node, and hence
> > being able to execute per-node flushes allows to reduce the contention
> > around the global flush_lock, while also allowing to perform concurrent
> > flushes on different nodes.
> > 
> > This however doesn't come for free.  A new vector must be allocated to be
> > used for the per-NUMA flushes, and more logic is required in the flush
> > dispatcher to figure out whether a flush is limited to a single node.
> > 
> > The figures on a 2-node NUMA system are as follows, after having been
> > running the same XenRT boot storm workload for 90 minutes.
> > 
> > Without the per-NUMA node flush:
> > 
> > Global flush_lock: addr=ffff82d040837340, lockval=d8ded8de, not locked
> >   lock:21878876(98178042228), block:1603338(6043805110)
> > 
> > So a total block time of ~6s, and average block time of 3.7us.
> > 
> > With the per-node locks:
> > 
> > Global flush_lock: addr=ffff82d040837360, lockval=78e678e6, not locked
> >   lock:6781028(41032945811), block:583712(2025657239)
> > NUMA node 1 flush_lock: addr=ffff832fd085b110, lockval=5cd65cd6, not locked
> >   lock:220374(766500536), block:4091(9933129)
> > NUMA node 0 flush_lock: addr=ffff8310336a7110, lockval=5c715c71, not locked
> >   lock:547953(1658170241), block:23856(51266297)
> > 
> > The total block time goes down to ~2s, and the average block time is 3.4us.
> > The total block time of the per-node locks is much lower compared to the
> > global flush_lock, 9ms and 51ms respectively.
> > 
> > Note the example here is possibly the system where such split locks don't
> > make a lot of difference, being a 2 node system, but still there's a
> > non-trivial difference between the block times.  On a 4 or 9 node system
> > the figures should likely be even better.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> The savings must be from TLB flushes only? Cache flushes, as we have discussed
> recently, need to be global anyway, and hence there are no savings to be had.
> This may want reflecting the subject and/or description.

Yeah, it's only TLB flushes.

> > @@ -126,3 +129,95 @@ int __init arch_get_ram_range(unsigned int idx, paddr_t *start, paddr_t *end)
> >  
> >      return 0;
> >  }
> > +
> > +static struct arch_numa_node {
> > +    const void *flush_va;
> > +    unsigned int flush_flags;
> > +    cpumask_t flush_mask;
> > +    spinlock_t flush_lock;
> > +    struct lock_profile_qhead profile_head;
> > +} *node_info[MAX_NUMNODES];
> 
> __ro_after_init or at least __read_mostly?

__ro_after_init would be OK I think.

> > +static int __init cf_check arch_numa_init(void)
> > +{
> > +    unsigned int i;
> > +
> > +    if ( num_online_nodes() == 1 )
> > +        return 0;
> > +
> > +    for_each_online_node ( i )
> > +    {
> > +        struct arch_numa_node *node =
> > +            alloc_xenheap_pages(get_order_from_bytes(sizeof(*node)),
> > +                                                     MEMF_node(i));
> 
> A full page for what may cover just a cacheline or two? I realize ...
> 
> > +        if ( node )
> > +            clear_page(node);
> > +        else
> > +            node = xvzalloc(typeof(*node));
> 
> ... this (sadly) still has no NUMA-capable counterpart, but I'd have expected
> at least a brief comment to justify.

Oh, yes, it should better have a comment.

> > +void cf_check invalidate_tbl_numa(void)
> 
> Nit: s/tbl/tlb/ ?
> 
> > +bool flush_numa_node(const cpumask_t *mask, const void *va, unsigned int flags)
> > +{
> > +    nodeid_t node = num_online_nodes() > 1 ? cpumask_to_node(mask)
> > +                                           : NUMA_NO_NODE;
> > +    struct arch_numa_node *info;
> > +
> > +    if ( node == NUMA_NO_NODE )
> > +        return false;
> > +
> > +    info = node_info[node];
> > +
> > +    if ( !info )
> > +        return false;
> > +
> > +    spin_lock(&info->flush_lock);
> > +    cpumask_and(&info->flush_mask, mask, &cpu_online_map);
> > +    cpumask_clear_cpu(smp_processor_id(), &info->flush_mask);
> 
> IOW this function is strictly a helper of flush_area_mask(), relying on
> the local CPU to have had its flushing done already. Might it not be
> better if this was put as a static in smp.c then? Or, to keep struct
> arch_numa_node local to this file, split it into two parts?
> 
> > +    info->flush_va = va;
> > +    info->flush_flags = flags;
> > +    send_IPI_mask(&info->flush_mask, INVALIDATE_NUMA_VECTOR);
> 
> This one similarly depends on flush_area_mask() behavior, not calling here
> when mask has solely the local CPU set.
> 
> > --- a/xen/common/numa.c
> > +++ b/xen/common/numa.c
> > @@ -689,6 +689,29 @@ static int __init cf_check numa_setup(const char *opt)
> >  }
> >  custom_param("numa", numa_setup);
> >  
> > +/*
> > + * Return the NUMA node index if all CPUs in the mask belong to the same node,
> > + * otherwise return NUMA_NO_NODE.
> > + */
> > +nodeid_t cpumask_to_node(const cpumask_t *mask)
> > +{
> > +    unsigned int cpu;
> > +    nodeid_t node = NUMA_NO_NODE;
> > +
> > +    if ( num_online_nodes() == 1 )
> > +        return cpu_to_node[0];
> 
> The sole caller checks this already, so strictly speaking Misra would consider
> this dead / unreachable code.
> 
> Similarly Misra may not like this on Arm, where it's unused right now. Then
> again looks like NUMA work there was abandoned, so for now all ought to be
> fine here.

Thanks, given the feedback I think you consider this an improvement
worth pursuing then?

Regards, Roger.

Re: [PATCH 2/2] x86/numa: introduce per-NUMA node flush locks

[Jan Beulich]

On 04.06.2025 17:03, Roger Pau Monné wrote:
> Thanks, given the feedback I think you consider this an improvement
> worth pursuing then?

Given the numbers you provided - yes.

Jan

Re: [PATCH 2/2] x86/numa: introduce per-NUMA node flush locks

[Jan Beulich]

On 22.05.2025 10:48, Roger Pau Monne wrote:
> +bool flush_numa_node(const cpumask_t *mask, const void *va, unsigned int flags)
> +{
> +    nodeid_t node = num_online_nodes() > 1 ? cpumask_to_node(mask)
> +                                           : NUMA_NO_NODE;
> +    struct arch_numa_node *info;
> +
> +    if ( node == NUMA_NO_NODE )
> +        return false;

One further question: Here you limit NUMA flushing to a single node, using
global flushes in all other cases. Did you consider extending this? Savings
ought to be had also when you want to e.g. flush on 2 out of 8 nodes. That
is, would it perhaps make sense to break up the file-scope flush_* variables
in smp.c altogether, using the NUMA approach uniformly?

> +    info = node_info[node];

node_info[0] would then need populating from a static struct instance, of
course, and ...

> +    if ( !info )
> +        return false;

... such conditionals of course would need to go away. Then we'd also get
away without requiring yet another direct-APIC-vector, as ...

> +    spin_lock(&info->flush_lock);
> +    cpumask_and(&info->flush_mask, mask, &cpu_online_map);
> +    cpumask_clear_cpu(smp_processor_id(), &info->flush_mask);
> +    info->flush_va = va;
> +    info->flush_flags = flags;
> +    send_IPI_mask(&info->flush_mask, INVALIDATE_NUMA_VECTOR);

... INVALIDATE_TLB_VECTOR could then be used here.

Jan

> +    while ( !cpumask_empty(&info->flush_mask) )
> +        cpu_relax();
> +    spin_unlock(&info->flush_lock);
> +
> +    return true;
> +}

Re: [PATCH 2/2] x86/numa: introduce per-NUMA node flush locks

[Roger Pau Monné]

On Wed, Jun 04, 2025 at 02:42:45PM +0200, Jan Beulich wrote:
> On 22.05.2025 10:48, Roger Pau Monne wrote:
> > +bool flush_numa_node(const cpumask_t *mask, const void *va, unsigned int flags)
> > +{
> > +    nodeid_t node = num_online_nodes() > 1 ? cpumask_to_node(mask)
> > +                                           : NUMA_NO_NODE;
> > +    struct arch_numa_node *info;
> > +
> > +    if ( node == NUMA_NO_NODE )
> > +        return false;
> 
> One further question: Here you limit NUMA flushing to a single node, using
> global flushes in all other cases. Did you consider extending this?

I did consider it, my first through was that such approach would
require taking multiple per-node locks at the same time, and so was
not worth pursuing.

I now realize however that we can decompose multi-node flushes into
per-node actions, and execute them sequentially, taking just one
per-node lock for each action.  I can see into doing this.  I'm unsure
whether such decomposing into sequential per-node flushes won't add
more latency (even if reducing contention).

As you say below, that way we would avoid having to allocate an extra
vector.

Thanks, Roger.