Re: [PATCH v3 03/17] xen/arm: implement node distance helpers for Arm

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.04.2023 13:25, Henry Wang wrote:
> From: Wei Chen <wei.chen@xxxxxxx>
> 
> 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 definition from asm/numa.h to xen/numa.h.

Nit: You mean "declaration", not "definition".

> At same time, the
> outdated u8 return value of x86 has been changed to unsigned char.
> 
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> Signed-off-by: Henry Wang <Henry.Wang@xxxxxxx>

non-Arm parts; to more it's not applicable anyway:
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

> --- a/xen/arch/arm/numa.c
> +++ b/xen/arch/arm/numa.c
> @@ -28,6 +28,11 @@ 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 }
> +};

Nit: There's no (incomplete or complete) initializer needed here, if
all you're after is having all slots set to zero.

However, looking at the code below, don't you mean to have the array
pre-set to all NUMA_NO_DISTANCE?

> @@ -48,3 +53,50 @@ 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 0xff as an unreachable node and 0-9 are undefined */
> +    if ( distance >= NUMA_NO_DISTANCE ||
> +         (distance >= NUMA_DISTANCE_UDF_MIN &&

Compilers may warn about comparison of "unsigned int" to be >= 0. I'm
not sure about the usefulness of the NUMA_DISTANCE_UDF_MIN define in
the first place, so maybe best drop it and its use here?

> +unsigned char __node_distance(nodeid_t from, nodeid_t to)
> +{
> +    /* When NUMA is off, any distance will be treated as remote. */
> +    if ( numa_disabled() )
> +        return NUMA_REMOTE_DISTANCE;

Wouldn't it make sense to have the "from == to" special case ahead of
this (rather than further down), thus yielding a sensible result for
from == to == 0? And else return NUMA_NO_DISTANCE, thus having a
sensible result also for any from/to != 0?

> +    /*
> +     * 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 (return 0xFF)
> +     */
> +    if ( from >= ARRAY_SIZE(node_distance_map) ||
> +         to >= ARRAY_SIZE(node_distance_map[0]) )
> +        return from == to ? NUMA_LOCAL_DISTANCE : NUMA_NO_DISTANCE;
> +
> +    return node_distance_map[from][to];
> +}
> +
> +EXPORT_SYMBOL(__node_distance);

What is this needed for?

Jan