[v4,03/17] xen/arm: implement node distance helpers for Arm

Commit Message

Henry Wang April 25, 2023, 7:56 a.m. UTC

From: Wei Chen <[e-mail]>

We will parse NUMA nodes distances from device tree. So we need
a matrix to record the distances between any two nodes we parsed.
Accordingly, we provide this node_set_distance API for device tree
NUMA to set the distance for any two nodes in this patch. When
NUMA initialization failed, __node_distance will return
NUMA_REMOTE_DISTANCE, this will help us avoid doing rollback
for distance maxtrix when NUMA initialization failed.

As both x86 and Arm have implemented __node_distance, so we move
its declaration from asm/numa.h to xen/numa.h. At same time, the
outdated u8 return value of x86 has been changed to unsigned char.

Signed-off-by: Wei Chen <[e-mail]>
Signed-off-by: Henry Wang <[e-mail]>
Acked-by: Jan Beulich <[e-mail]> # non-Arm parts
---
v3 -> v4:
1. s/definition/declaration/ in commit message.
2. Add Acked-by tag from Jan for non-Arm parts.
3. Drop unnecessary initializer for node_distance_map. Pre-set the
   distance map to NUMA_NO_DISTANCE.
4. Drop NUMA_DISTANCE_UDF_MIN and its usage.
5. Drop EXPORT_SYMBOL(__node_distance).
6. Rework __node_distance()'s return value logic.
v2 -> v3:
1. Use __ro_after_init for node_distance_map.
2. Correct format of if condition identation in numa_set_distance().
3. Drop the unnecessary change to the year of copyright.
4. Use ARRAY_SIZE() to determine node_distance_map's row, column size.
v1 -> v2:
1. Use unsigned int/char instead of uint32_t/u8.
2. Re-org the commit message.
---
 xen/arch/arm/Makefile           |  1 +
 xen/arch/arm/include/asm/numa.h | 12 +++++++
 xen/arch/arm/numa.c             | 55 +++++++++++++++++++++++++++++++++
 xen/arch/x86/include/asm/numa.h |  1 -
 xen/arch/x86/srat.c             |  2 +-
 xen/include/xen/numa.h          |  1 +
 6 files changed, 70 insertions(+), 2 deletions(-)

Comments

Jan Beulich April 25, 2023, 8:37 a.m. UTC | #1

On 25.04.2023 09:56, Henry Wang wrote:
> --- a/xen/arch/arm/numa.c
> +++ b/xen/arch/arm/numa.c
> @@ -28,6 +28,12 @@ enum dt_numa_status {
>  
>  static enum dt_numa_status __ro_after_init device_tree_numa = DT_NUMA_DEFAULT;
>  
> +static unsigned char __ro_after_init
> +node_distance_map[MAX_NUMNODES][MAX_NUMNODES] = {
> +    [0 ... MAX_NUMNODES - 1] = { [0 ... MAX_NUMNODES - 1] = NUMA_NO_DISTANCE }
> +};
> +
> +

Nit: A stray 2nd blank line has appeared here.

> @@ -48,3 +54,52 @@ int __init arch_numa_setup(const char *opt)
>  {
>      return -EINVAL;
>  }
> +
> +void __init numa_set_distance(nodeid_t from, nodeid_t to,
> +                              unsigned int distance)
> +{
> +    if ( from >= ARRAY_SIZE(node_distance_map) ||
> +         to >= ARRAY_SIZE(node_distance_map[0]) )
> +    {
> +        printk(KERN_WARNING
> +               "NUMA: invalid nodes: from=%"PRIu8" to=%"PRIu8" MAX=%"PRIu8"\n",
> +               from, to, MAX_NUMNODES);
> +        return;
> +    }
> +
> +    /* NUMA defines NUMA_NO_DISTANCE as unreachable and 0-9 are undefined */
> +    if ( distance >= NUMA_NO_DISTANCE || distance <= NUMA_DISTANCE_UDF_MAX ||
> +         (from == to && distance != NUMA_LOCAL_DISTANCE) )
> +    {
> +        printk(KERN_WARNING
> +               "NUMA: invalid distance: from=%"PRIu8" to=%"PRIu8" distance=%"PRIu32"\n",
> +               from, to, distance);
> +        return;
> +    }

I appreciate the checking that node-local references are NUMA_LOCAL_DISTANCE,
but if they're wrongly passed into here, shouldn't the resulting array still
have NUMA_LOCAL_DISTANCE on its diagonal, at least as far as present nodes
go?

> +    node_distance_map[from][to] = distance;
> +}
> +
> +unsigned char __node_distance(nodeid_t from, nodeid_t to)
> +{
> +    if ( from == to )
> +        return NUMA_LOCAL_DISTANCE;
> +
> +    /*
> +     * When NUMA is off, any distance will be treated as unreachable, so
> +     * directly return NUMA_NO_DISTANCE from here as an optimization.
> +     */
> +    if ( numa_disabled() )
> +        return NUMA_NO_DISTANCE;
> +
> +    /*
> +     * Check whether the nodes are in the matrix range.
> +     * When any node is out of range, except from and to nodes are the
> +     * same, we treat them as unreachable.

I think this "except ..." part is slightly confusing, as it doesn't comment
the subsequent code, but instead refers to the first check in the function.
If you want to keep it, may I suggest to add something like "(see above)"
before the comma?

Jan

Henry Wang April 25, 2023, 9:31 a.m. UTC | #2

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Subject: Re: [PATCH v4 03/17] xen/arm: implement node distance helpers for
> Arm
> 
> On 25.04.2023 09:56, Henry Wang wrote:
> > --- a/xen/arch/arm/numa.c
> > +++ b/xen/arch/arm/numa.c
> > @@ -28,6 +28,12 @@ enum dt_numa_status {
> >
> >  static enum dt_numa_status __ro_after_init device_tree_numa =
> DT_NUMA_DEFAULT;
> >
> > +static unsigned char __ro_after_init
> > +node_distance_map[MAX_NUMNODES][MAX_NUMNODES] = {
> > +    [0 ... MAX_NUMNODES - 1] = { [0 ... MAX_NUMNODES - 1] =
> NUMA_NO_DISTANCE }
> > +};
> > +
> > +
> 
> Nit: A stray 2nd blank line has appeared here.

Will fix this in v5.

> 
> > +    /* NUMA defines NUMA_NO_DISTANCE as unreachable and 0-9 are
> undefined */
> > +    if ( distance >= NUMA_NO_DISTANCE || distance <=
> NUMA_DISTANCE_UDF_MAX ||
> > +         (from == to && distance != NUMA_LOCAL_DISTANCE) )
> > +    {
> > +        printk(KERN_WARNING
> > +               "NUMA: invalid distance: from=%"PRIu8" to=%"PRIu8"
> distance=%"PRIu32"\n",
> > +               from, to, distance);
> > +        return;
> > +    }
> 
> I appreciate the checking that node-local references are
> NUMA_LOCAL_DISTANCE,
> but if they're wrongly passed into here, shouldn't the resulting array still
> have NUMA_LOCAL_DISTANCE on its diagonal, at least as far as present nodes
> go?

Apologies in advance to ask more specific details from you as I am not sure
if I can correctly understand the "if they're wrongly passed into here" case. Do you
mean we are always guaranteed that if from == to, the distance will always be
NUMA_LOCAL_DISTANCE so the (from == to && distance != NUMA_LOCAL_DISTANCE)
check is redundant and can be removed? Thanks.

> 
> > +    node_distance_map[from][to] = distance;
> > +}

[...]

> > +    /*
> > +     * Check whether the nodes are in the matrix range.
> > +     * When any node is out of range, except from and to nodes are the
> > +     * same, we treat them as unreachable.
> 
> I think this "except ..." part is slightly confusing, as it doesn't comment
> the subsequent code, but instead refers to the first check in the function.
> If you want to keep it, may I suggest to add something like "(see above)"
> before the comma?

Sure, I will add "(see above)" in v5.

Kind regards,
Henry

> 
> Jan

Jan Beulich April 25, 2023, 9:43 a.m. UTC | #3

On 25.04.2023 11:31, Henry Wang wrote:
>> -----Original Message-----
>> From: Jan Beulich <[e-mail]>
>>
>> On 25.04.2023 09:56, Henry Wang wrote:
>>> +    /* NUMA defines NUMA_NO_DISTANCE as unreachable and 0-9 are
>> undefined */
>>> +    if ( distance >= NUMA_NO_DISTANCE || distance <=
>> NUMA_DISTANCE_UDF_MAX ||
>>> +         (from == to && distance != NUMA_LOCAL_DISTANCE) )
>>> +    {
>>> +        printk(KERN_WARNING
>>> +               "NUMA: invalid distance: from=%"PRIu8" to=%"PRIu8"
>> distance=%"PRIu32"\n",
>>> +               from, to, distance);
>>> +        return;
>>> +    }
>>
>> I appreciate the checking that node-local references are
>> NUMA_LOCAL_DISTANCE,
>> but if they're wrongly passed into here, shouldn't the resulting array still
>> have NUMA_LOCAL_DISTANCE on its diagonal, at least as far as present nodes
>> go?
> 
> Apologies in advance to ask more specific details from you as I am not sure
> if I can correctly understand the "if they're wrongly passed into here" case. Do you
> mean we are always guaranteed that if from == to, the distance will always be
> NUMA_LOCAL_DISTANCE so the (from == to && distance != NUMA_LOCAL_DISTANCE)
> check is redundant and can be removed? Thanks.

It's a little odd that you ask me: It is your code which insists on
NUMA_LOCAL_DISTANCE when from == to. If you insist on your caller to pass
only this one value, then I think it is only logical to also set the
respective table entries to that value (rather than leaving them at
NUMA_NO_DISTANCE).

Jan

Henry Wang April 25, 2023, 9:48 a.m. UTC | #4

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Subject: Re: [PATCH v4 03/17] xen/arm: implement node distance helpers for
> Arm
> 
> On 25.04.2023 11:31, Henry Wang wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <[e-mail]>
> >>
> >> On 25.04.2023 09:56, Henry Wang wrote:
> >>> +    /* NUMA defines NUMA_NO_DISTANCE as unreachable and 0-9 are
> >> undefined */
> >>> +    if ( distance >= NUMA_NO_DISTANCE || distance <=
> >> NUMA_DISTANCE_UDF_MAX ||
> >>> +         (from == to && distance != NUMA_LOCAL_DISTANCE) )
> >>> +    {
> >>> +        printk(KERN_WARNING
> >>> +               "NUMA: invalid distance: from=%"PRIu8" to=%"PRIu8"
> >> distance=%"PRIu32"\n",
> >>> +               from, to, distance);
> >>> +        return;
> >>> +    }
> >>
> >> I appreciate the checking that node-local references are
> >> NUMA_LOCAL_DISTANCE,
> >> but if they're wrongly passed into here, shouldn't the resulting array still
> >> have NUMA_LOCAL_DISTANCE on its diagonal, at least as far as present
> nodes
> >> go?
> >
> > Apologies in advance to ask more specific details from you as I am not sure
> > if I can correctly understand the "if they're wrongly passed into here" case.
> Do you
> > mean we are always guaranteed that if from == to, the distance will always
> be
> > NUMA_LOCAL_DISTANCE so the (from == to && distance !=
> NUMA_LOCAL_DISTANCE)
> > check is redundant and can be removed? Thanks.
> 
> It's a little odd that you ask me: It is your code which insists on
> NUMA_LOCAL_DISTANCE when from == to. If you insist on your caller to pass
> only this one value, then I think it is only logical to also set the
> respective table entries to that value (rather than leaving them at
> NUMA_NO_DISTANCE).

I think I understand what do you mean now. I was only checking this specific
patch so didn't make the connection with the latter patches.

Kind regards,
Henry

> 
> Jan

Patch

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 4d076b278b..9073398d6e 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -38,6 +38,7 @@  obj-$(CONFIG_LIVEPATCH) += livepatch.o
 obj-y += mem_access.o
 obj-y += mm.o
 obj-y += monitor.o
+obj-$(CONFIG_NUMA) += numa.o
 obj-y += p2m.o
 obj-y += percpu.o
 obj-y += platform.o
diff --git a/xen/arch/arm/include/asm/numa.h b/xen/arch/arm/include/asm/numa.h
index 83f60ad05b..96c856a9f7 100644
--- a/xen/arch/arm/include/asm/numa.h
+++ b/xen/arch/arm/include/asm/numa.h
@@ -22,7 +22,19 @@  typedef u8 nodeid_t;
  */
 #define NR_NODE_MEMBLKS NR_MEM_BANKS
 
+/*
+ * In ACPI spec, 0-9 are the reserved values for node distance,
+ * 10 indicates local node distance, 20 indicates remote node
+ * distance. Set node distance map in device tree will follow
+ * the ACPI's definition.
+ */
+#define NUMA_DISTANCE_UDF_MAX   9
+#define NUMA_LOCAL_DISTANCE     10
+#define NUMA_REMOTE_DISTANCE    20
+
 extern bool numa_disabled(void);
+extern void numa_set_distance(nodeid_t from, nodeid_t to,
+                              unsigned int distance);
 
 #else
 
diff --git a/xen/arch/arm/numa.c b/xen/arch/arm/numa.c
index eb5d0632cb..e4f75f314b 100644
--- a/xen/arch/arm/numa.c
+++ b/xen/arch/arm/numa.c
@@ -28,6 +28,12 @@  enum dt_numa_status {
 
 static enum dt_numa_status __ro_after_init device_tree_numa = DT_NUMA_DEFAULT;
 
+static unsigned char __ro_after_init
+node_distance_map[MAX_NUMNODES][MAX_NUMNODES] = {
+    [0 ... MAX_NUMNODES - 1] = { [0 ... MAX_NUMNODES - 1] = NUMA_NO_DISTANCE }
+};
+
+
 void __init numa_fw_bad(void)
 {
     printk(KERN_ERR "NUMA: device tree numa info table not used.\n");
@@ -48,3 +54,52 @@  int __init arch_numa_setup(const char *opt)
 {
     return -EINVAL;
 }
+
+void __init numa_set_distance(nodeid_t from, nodeid_t to,
+                              unsigned int distance)
+{
+    if ( from >= ARRAY_SIZE(node_distance_map) ||
+         to >= ARRAY_SIZE(node_distance_map[0]) )
+    {
+        printk(KERN_WARNING
+               "NUMA: invalid nodes: from=%"PRIu8" to=%"PRIu8" MAX=%"PRIu8"\n",
+               from, to, MAX_NUMNODES);
+        return;
+    }
+
+    /* NUMA defines NUMA_NO_DISTANCE as unreachable and 0-9 are undefined */
+    if ( distance >= NUMA_NO_DISTANCE || distance <= NUMA_DISTANCE_UDF_MAX ||
+         (from == to && distance != NUMA_LOCAL_DISTANCE) )
+    {
+        printk(KERN_WARNING
+               "NUMA: invalid distance: from=%"PRIu8" to=%"PRIu8" distance=%"PRIu32"\n",
+               from, to, distance);
+        return;
+    }
+
+    node_distance_map[from][to] = distance;
+}
+
+unsigned char __node_distance(nodeid_t from, nodeid_t to)
+{
+    if ( from == to )
+        return NUMA_LOCAL_DISTANCE;
+
+    /*
+     * When NUMA is off, any distance will be treated as unreachable, so
+     * directly return NUMA_NO_DISTANCE from here as an optimization.
+     */
+    if ( numa_disabled() )
+        return NUMA_NO_DISTANCE;
+
+    /*
+     * Check whether the nodes are in the matrix range.
+     * When any node is out of range, except from and to nodes are the
+     * same, we treat them as unreachable.
+     */
+    if ( from >= ARRAY_SIZE(node_distance_map) ||
+         to >= ARRAY_SIZE(node_distance_map[0]) )
+        return NUMA_NO_DISTANCE;
+
+    return node_distance_map[from][to];
+}
diff --git a/xen/arch/x86/include/asm/numa.h b/xen/arch/x86/include/asm/numa.h
index 7866afa408..45456ac441 100644
--- a/xen/arch/x86/include/asm/numa.h
+++ b/xen/arch/x86/include/asm/numa.h
@@ -22,7 +22,6 @@  extern void init_cpu_to_node(void);
 #define arch_want_default_dmazone() (num_online_nodes() > 1)
 
 void srat_parse_regions(paddr_t addr);
-extern u8 __node_distance(nodeid_t a, nodeid_t b);
 unsigned int arch_get_dma_bitsize(void);
 
 #endif
diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
index 56749ddca5..50faf5d352 100644
--- a/xen/arch/x86/srat.c
+++ b/xen/arch/x86/srat.c
@@ -328,7 +328,7 @@  unsigned int numa_node_to_arch_nid(nodeid_t n)
 	return 0;
 }
 
-u8 __node_distance(nodeid_t a, nodeid_t b)
+unsigned char __node_distance(nodeid_t a, nodeid_t b)
 {
 	unsigned index;
 	u8 slit_val;
diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
index b86d0851fc..8356e47b61 100644
--- a/xen/include/xen/numa.h
+++ b/xen/include/xen/numa.h
@@ -114,6 +114,7 @@  extern bool numa_memblks_available(void);
 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 unsigned char __node_distance(nodeid_t a, nodeid_t b);
 
 #else