RE: [PATCH 23/37] xen/arm: implement node distance helpers for Arm

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 24 Sep 2021, Wei Chen wrote:
> > -----Original Message-----
> > From: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > Sent: 2021年9月24日 9:47
> > To: Wei Chen <Wei.Chen@xxxxxxx>
> > Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; sstabellini@xxxxxxxxxx; julien@xxxxxxx;
> > Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
> > Subject: Re: [PATCH 23/37] xen/arm: implement node distance helpers for
> > Arm
> > 
> > On Thu, 23 Sep 2021, Wei Chen wrote:
> > > We will parse NUMA nodes distances from device tree or ACPI
> > > table. 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 or ACPI table parsers
> > > 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.
> > >
> > > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > > ---
> > >  xen/arch/arm/Makefile      |  1 +
> > >  xen/arch/arm/numa.c        | 69 ++++++++++++++++++++++++++++++++++++++
> > >  xen/include/asm-arm/numa.h | 13 +++++++
> > >  3 files changed, 83 insertions(+)
> > >  create mode 100644 xen/arch/arm/numa.c
> > >
> > > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> > > index ae4efbf76e..41ca311b6b 100644
> > > --- a/xen/arch/arm/Makefile
> > > +++ b/xen/arch/arm/Makefile
> > > @@ -35,6 +35,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/numa.c b/xen/arch/arm/numa.c
> > > new file mode 100644
> > > index 0000000000..3f08870d69
> > > --- /dev/null
> > > +++ b/xen/arch/arm/numa.c
> > > @@ -0,0 +1,69 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Arm Architecture support layer for NUMA.
> > > + *
> > > + * Copyright (C) 2021 Arm Ltd
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License version 2 as
> > > + * published by the Free Software Foundation.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > > + *
> > > + * You should have received a copy of the GNU General Public License
> > > + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> > > + *
> > > + */
> > > +#include <xen/init.h>
> > > +#include <xen/numa.h>
> > > +
> > > +static uint8_t __read_mostly
> > > +node_distance_map[MAX_NUMNODES][MAX_NUMNODES] = {
> > > +    { 0 }
> > > +};
> > > +
> > > +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: 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 &&
> > > +         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;
> > > +}
> > > +
> > > +uint8_t __node_distance(nodeid_t from, nodeid_t to)
> > > +{
> > > +    /* When NUMA is off, any distance will be treated as remote. */
> > > +    if ( srat_disabled() )
> > 
> > Given that this is ARM specific code and specific to ACPI, I don't think
> > we should have any call to something called "srat_disabled".
> > 
> > I suggest to either rename srat_disabled to numa_distance_disabled.
> > 
> > Other than that, this patch looks OK to me.
> > 
> 
> srat stands for static resource affinity table, I think dtb also can be
> treated as a static resource affinity table. So I keep SRAT in this patch
> and other patches. I have seen your comment in patch#25. Before x86 
> maintainers
> give any feedback, can we still keep srat here?

Jan and I replied in the other thread. I think that in warning messages
"SRAT" should not be mentioned when booting from DT. Ideally functions
names and variables should be renamed too when shared between ACPI and
DT but it is less critical, and it is fine if you don't do that in the
next version.