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

[Jan Beulich <jbeulich@xxxxxxxx>]

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