RE: [PATCH v2 03/17] xen/arm: implement node distance helpers for Arm

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 2023年1月11日 0:47
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: nd <nd@xxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Julien
> Grall <julien@xxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxx>; Wei
> Liu <wl@xxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 03/17] xen/arm: implement node distance helpers for
> Arm
> 
> On 10.01.2023 09:49, Wei Chen wrote:
> > --- a/xen/arch/arm/include/asm/numa.h
> > +++ b/xen/arch/arm/include/asm/numa.h
> > @@ -28,6 +28,20 @@ enum dt_numa_status {
> >      DT_NUMA_OFF,
> >  };
> >
> > +/*
> > + * 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_MIN   0
> > +#define NUMA_DISTANCE_UDF_MAX   9
> > +#define NUMA_LOCAL_DISTANCE     10
> > +#define NUMA_REMOTE_DISTANCE    20
> 
> In the absence of a caller of numa_set_distance() it is entirely unclear
> whether this tying to ACPI used values is actually appropriate.
> 

From Kernel's NUMA device tree binding, it seems DT NUMA are reusing
ACPI used values for distances [1].

> > --- a/xen/arch/arm/numa.c
> > +++ b/xen/arch/arm/numa.c
> > @@ -2,7 +2,7 @@
> >  /*
> >   * Arm Architecture support layer for NUMA.
> >   *
> > - * Copyright (C) 2021 Arm Ltd
> > + * Copyright (C) 2022 Arm Ltd
> 
> I don't think it makes sense to change such after the fact. And certainly
> not in an unrelated patch.
> 

I will retore it, and add a SPDX header.

> > @@ -22,6 +22,11 @@
> >
> >  static enum dt_numa_status __read_mostly device_tree_numa;
> >
> > +static unsigned char __read_mostly
> > +node_distance_map[MAX_NUMNODES][MAX_NUMNODES] = {
> > +    { 0 }
> > +};
> 
> __ro_after_init?
> 

Yes.

> > @@ -42,3 +47,48 @@ 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 >= MAX_NUMNODES || to >= MAX_NUMNODES )
> > +    {
> > +        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 &&
> 
> Nit: Indentation.
> 

Ok.

> > +         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)
> > +{
> > +    /* When NUMA is off, any distance will be treated as remote. */
> > +    if ( numa_disabled() )
> > +        return NUMA_REMOTE_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 (return 0xFF)
> > +     */
> > +    if ( from >= MAX_NUMNODES || to >= MAX_NUMNODES )
> 
> I guess using ARRAY_SIZE() here would be more future-proof.
> 

I will use it in next version.

[1]https://www.kernel.org/doc/Documentation/devicetree/bindings/numa.txt

Thanks,
Wei Chen

> Jan