Re: [XEN RFC PATCH 13/40] xen/arm: introduce numa_set_node for Arm

[Julien Grall <julien@xxxxxxx>]

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?
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> ---
>   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.

> 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.
> +
>   #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