Hi Julien,
> -----Original Message-----
> From: Julien Grall <jul...@xen.org>
> Sent: 2021年8月25日 18:37
> To: Wei Chen <wei.c...@arm.com>; xen-devel@lists.xenproject.org;
> sstabell...@kernel.org; jbeul...@suse.com
> Cc: Bertrand Marquis <bertrand.marq...@arm.com>
> Subject: Re: [XEN RFC PATCH 13/40] xen/arm: introduce numa_set_node for
> Arm
>
> Hi Wei,
>
> On 11/08/2021 11:23, Wei Chen wrote:
> > This API is used to set one CPU to a NUMA node. If the system
> > configure NUMA off or system initialize NUMA failed, the
> > online NUMA node would set to only node#0. This will be done
> > in following patches. When NUMA turn off or init failed,
> > node_online_map will be cleared and set node#0 online. So we
> > use node_online_map to prevent to set a CPU to an offline node.
>
> IHMO numa_set_node() should behave exactly the same way on x86 and Arm
> because this is going to be used by the common code.
>
> From the commit message, I don't quite understand why the check is
> necessary on Arm but not on x86. Can you clarify it?
>
Yes, in patch#27, in smpboot.c, dt_smp_init_cpus function.
We will parse CPU numa-node-id from dtb CPU node. If we get
a valid node ID for one CPU, we will invoke numa_set_node to
create CPU-NODE map. But in our testing, we found when NUMA
init failed, numa_set_node still can set CPU to a offline
or invalid NODE. So we're using node_online_map to prevent
this behavior. Otherwise we have to check node_online_map
everywhere before we call numa_set_node.
x86 actually is doing the same way, but it handles node_online_map
check out of numa_set_node:
57 void __init init_cpu_to_node(void)
58 {
59 unsigned int i;
60 nodeid_t node;
61
62 for ( i = 0; i < nr_cpu_ids; i++ )
63 {
64 u32 apicid = x86_cpu_to_apicid[i];
65 if ( apicid == BAD_APICID )
66 continue;
67 node = apicid < MAX_LOCAL_APIC ? apicid_to_node[apicid] :
NUMA_NO_NODE;
68 if ( node == NUMA_NO_NODE || !node_online(node) )
69 node = 0;
70 numa_set_node(i, node);
71 }
72 }
> >
> > Signed-off-by: Wei Chen <wei.c...@arm.com>
> > ---
> > xen/arch/arm/Makefile | 1 +
> > xen/arch/arm/numa.c | 31 +++++++++++++++++++++++++++++++
> > xen/include/asm-arm/numa.h | 2 ++
> > xen/include/asm-x86/numa.h | 1 -
> > xen/include/xen/numa.h | 1 +
> > 5 files changed, 35 insertions(+), 1 deletion(-)
> > create mode 100644 xen/arch/arm/numa.c
> >
> > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> > index 3d3b97b5b4..6e3fb8033e 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..1e30c5bb13
> > --- /dev/null
> > +++ b/xen/arch/arm/numa.c
> > @@ -0,0 +1,31 @@
> > +// 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/nodemask.h>
> > +#include <xen/numa.h>
> > +
> > +void numa_set_node(int cpu, nodeid_t nid)
> > +{
> > + if ( nid >= MAX_NUMNODES ||
> > + !nodemask_test(nid, &node_online_map) )
> > + nid = 0;
> > +
> > + cpu_to_node[cpu] = nid;
> > +}
> I think numa_set_node() will want to be implemented in common code.
>
See my above comment. If x86 is ok, I think yes, we can do it
in common code.
> > diff --git a/xen/include/asm-arm/numa.h b/xen/include/asm-arm/numa.h
> > index ab9c4a2448..1162c702df 100644
> > --- a/xen/include/asm-arm/numa.h
> > +++ b/xen/include/asm-arm/numa.h
> > @@ -27,6 +27,8 @@ extern mfn_t first_valid_mfn;
> > #define node_start_pfn(nid) (mfn_x(first_valid_mfn))
> > #define __node_distance(a, b) (20)
> >
> > +#define numa_set_node(x, y) do { } while (0)
>
> I would define it in xen/numa.h so other arch can take advantage ot it.
> Also, please use a static inline helper so the arguments are evaluated.
>
Ok
> > +
> > #endif
> >
> > #endif /* __ARCH_ARM_NUMA_H */
> > diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h
> > index f8e4e15586..69859b0a57 100644
> > --- a/xen/include/asm-x86/numa.h
> > +++ b/xen/include/asm-x86/numa.h
> > @@ -22,7 +22,6 @@ extern nodeid_t pxm_to_node(unsigned int pxm);
> > #define ZONE_ALIGN (1UL << (MAX_ORDER+PAGE_SHIFT))
> >
> > extern int srat_disabled(void);
> > -extern void numa_set_node(int cpu, nodeid_t node);
> > extern nodeid_t setup_node(unsigned int pxm);
> > extern void srat_detect_node(int cpu);
> >
> > diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
> > index 258a5cb3db..3972aa6b93 100644
> > --- a/xen/include/xen/numa.h
> > +++ b/xen/include/xen/numa.h
> > @@ -70,6 +70,7 @@ extern int valid_numa_range(u64 start, u64 end,
> nodeid_t node);
> >
> > extern void numa_init_array(void);
> > extern void numa_initmem_init(unsigned long start_pfn, unsigned long
> end_pfn);
> > +extern void numa_set_node(int cpu, nodeid_t node);
> > extern bool numa_off;
> > extern int numa_fake;
> >
> >
>
> Cheers,
>
> --
> Julien Grall
