RE: [XEN RFC PATCH 20/40] xen/arm: implement node distance helpers for Arm64

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Stefano,

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Sent: 2021年8月27日 7:52
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; sstabellini@xxxxxxxxxx; julien@xxxxxxx;
> jbeulich@xxxxxxxx; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
> Subject: Re: [XEN RFC PATCH 20/40] xen/arm: implement node distance
> helpers for Arm64
> 
> On Wed, 11 Aug 2021, Wei Chen wrote:
> > In current Xen code, __node_distance is a fake API, it always
> > returns NUMA_REMOTE_DISTANCE(20). Now we use a matrix to record
> > the distance between any two nodes. Accordingly, we provide a
> > set_node_distance API to set the distance for any two nodes in
> > this patch.
> >
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > ---
> >  xen/arch/arm/numa.c        | 44 ++++++++++++++++++++++++++++++++++++++
> >  xen/include/asm-arm/numa.h | 12 ++++++++++-
> >  xen/include/asm-x86/numa.h |  1 -
> >  xen/include/xen/numa.h     |  2 +-
> >  4 files changed, 56 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/arch/arm/numa.c b/xen/arch/arm/numa.c
> > index 566ad1e52b..f61a8df645 100644
> > --- a/xen/arch/arm/numa.c
> > +++ b/xen/arch/arm/numa.c
> > @@ -23,6 +23,11 @@
> >  #include <xen/pfn.h>
> >  #include <asm/setup.h>
> >
> > +static uint8_t __read_mostly
> > +node_distance_map[MAX_NUMNODES][MAX_NUMNODES] = {
> > +    { NUMA_REMOTE_DISTANCE }
> > +};
> > +
> >  void numa_set_node(int cpu, nodeid_t nid)
> >  {
> >      if ( nid >= MAX_NUMNODES ||
> > @@ -32,6 +37,45 @@ void numa_set_node(int cpu, nodeid_t nid)
> >      cpu_to_node[cpu] = nid;
> >  }
> >
> > +void __init numa_set_distance(nodeid_t from, nodeid_t to, uint32_t
> distance)
> > +{
> > +    if ( from >= MAX_NUMNODES || to >= MAX_NUMNODES )
> > +    {
> > +        printk(KERN_WARNING
> > +            "NUMA nodes are out of matrix, from=%u to=%u distance=%u\n",
> > +            from, to, distance);
> 
> NIT: please align. Example:
> 
> printk(KERN_WARNING
>        "NUMA nodes are out of matrix, from=%u to=%u distance=%u\n",
> 
> Also please use PRIu32 for uint32_t. Probably should use PRIu8 for
> nodeids.
> 

OK

> 
> > +        return;
> > +    }
> > +
> > +    /* NUMA defines 0xff as an unreachable node and 0-9 are undefined
> */
> > +    if ( distance >= NUMA_NO_DISTANCE ||
> > +        (distance >= NUMA_DISTANCE_UDF_MIN &&
> > +         distance <= NUMA_DISTANCE_UDF_MAX) ||
> > +        (from == to && distance != NUMA_LOCAL_DISTANCE) )
> > +    {
> > +        printk(KERN_WARNING
> > +            "Invalid NUMA node distance, from:%d to:%d distance=%d\n",
> > +            from, to, distance);
> 
> NIT: please align
> 
> Also you used %u before for nodeids, which is better because from and to
> are unsigned. Distance should be uint32_t.
> 

OK

> 
> > +        return;
> > +    }
> > +
> > +    node_distance_map[from][to] = distance;
> 
> Shouldn't we also be setting:
> 
>     node_distance_map[to][from] = distance;
> 
> ?
> 

No, we want numa_set_distance behavior is single.
"node_distance_map[to][from] = distance" is handled in caller.

> 
> > +}
> > +
> > +uint8_t __node_distance(nodeid_t from, nodeid_t to)
> > +{
> > +    /*
> > +     * 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 )
> > +        return from == to ? NUMA_LOCAL_DISTANCE : NUMA_NO_DISTANCE;
> > +
> > +    return node_distance_map[from][to];
> > +}
> > +EXPORT_SYMBOL(__node_distance);
> > +
> >  void __init numa_init(bool acpi_off)
> >  {
> >      uint32_t idx;
> > diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h
> > index bb495a24e1..559b028a01 100644
> > --- a/xen/include/asm-arm/numa.h
> > +++ b/xen/include/asm-arm/numa.h
> > @@ -12,8 +12,19 @@ typedef u8 nodeid_t;
> >   * set the number of NUMA memory block number to 128.
> >   */
> >  #define NODES_SHIFT      6
> > +/*
> > + * 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
> >
> >  extern void numa_init(bool acpi_off);
> > +extern void numa_set_distance(nodeid_t from, nodeid_t to, uint32_t
> distance);
> >
> >  /*
> >   * Temporary for fake NUMA node, when CPU, memory and distance
> > @@ -21,7 +32,6 @@ extern void numa_init(bool acpi_off);
> >   * symbols will be removed.
> >   */
> >  extern mfn_t first_valid_mfn;
> > -#define __node_distance(a, b) (20)
> >
> >  #else
> >
> > diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h
> > index 5a57a51e26..e0253c20b7 100644
> > --- a/xen/include/asm-x86/numa.h
> > +++ b/xen/include/asm-x86/numa.h
> > @@ -21,7 +21,6 @@ extern nodeid_t apicid_to_node[];
> >  extern void init_cpu_to_node(void);
> >
> >  void srat_parse_regions(u64 addr);
> > -extern u8 __node_distance(nodeid_t a, nodeid_t b);
> >  unsigned int arch_get_dma_bitsize(void);
> >
> >  #endif
> > diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
> > index cb08d2eca9..0475823b13 100644
> > --- a/xen/include/xen/numa.h
> > +++ b/xen/include/xen/numa.h
> > @@ -58,7 +58,7 @@ static inline __attribute__((pure)) nodeid_t
> phys_to_nid(paddr_t addr)
> >  #define node_spanned_pages(nid)    (NODE_DATA(nid)->node_spanned_pages)
> >  #define node_end_pfn(nid)       (NODE_DATA(nid)->node_start_pfn + \
> >                              NODE_DATA(nid)->node_spanned_pages)
> > -
> > +extern u8 __node_distance(nodeid_t a, nodeid_t b);
> >  extern void numa_add_cpu(int cpu);
> >
> >  struct node {
> > --
> > 2.25.1
> >