Re: [PATCH v7 2/6] xen/x86: move generically usable NUMA code from x86 to common

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.10.2022 08:14, Wei Chen wrote:
> There are some codes in x86/numa.c can be shared by common
> architectures to implememnt NUMA support. Just like some
> variables and functions to check and store NUMA memory map.
> And some variables and functions to do NUMA initialization.
> 
> In this patch, we move them to common/numa.c and xen/numa.h
> and use the CONFIG_NUMA to gate them for non-NUMA supported
> architectures. As the target header file is Xen-style, so
> we trim some spaces and replace tabs for the codes that has
> been moved to xen/numa.h at the same time.
> 
> As acpi_scan_nodes has been used in a common function, it
> doesn't make sense to use acpi_xxx in common code, so we
> rename it to numa_process_nodes in this patch too. After that
> if we still use CONFIG_ACPI_NUMA in to gate numa_process_nodes
> in numa_initmem_init, that doesn't make sense. As CONFIG_NUMA
> will be selected by CONFIG_ACPI_NUMA for x86. So, we replace
> CONFIG_ACPI_NUMA by CONFIG_NUMA to gate numa_process_nodes.
> 
> As arch_numa_disabled has been implememnted for ACPI NUMA,
> we can rename srat_disabled to numa_disabled and move it
> to common code as well.
> 
> The macro node_to_first_cpu(node) hasn't been used anywhere,
> so we drop it in this patch too.
> 
> Because some architectures allow to use all 64 physical address
> bits, but some architectures are not (like Arm64 allows 52, 48
> bits). In this case, we use min(PADDR_BITS, BITS_PER_LONG - 1)
> to calculate the shift when only one node is in the system in
> this patch too.
> 
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with one small further request (could be taken care of also while
committing if no other need for a v8 arises):

> --- /dev/null
> +++ b/xen/common/numa.c
> @@ -0,0 +1,464 @@
> +/*
> + * Generic VM initialization for NUMA setups.
> + * Copyright 2002,2003 Andi Kleen, SuSE Labs.
> + * Adapted for Xen: Ryan Harper <ryanh@xxxxxxxxxx>
> + */
> +
> +#include <xen/init.h>
> +#include <xen/keyhandler.h>
> +#include <xen/mm.h>
> +#include <xen/nodemask.h>
> +#include <xen/numa.h>
> +#include <xen/param.h>
> +#include <xen/sched.h>
> +#include <xen/softirq.h>
> +
> +struct node_data __ro_after_init node_data[MAX_NUMNODES];
> +
> +/* Mapping from pdx to node id */
> +unsigned int __ro_after_init memnode_shift;
> +unsigned long __ro_after_init memnodemapsize;
> +nodeid_t *__ro_after_init memnodemap;
> +static typeof(*memnodemap) __ro_after_init _memnodemap[64];
> +
> +nodeid_t __read_mostly cpu_to_node[NR_CPUS] = {
> +    [0 ... NR_CPUS-1] = NUMA_NO_NODE
> +};
> +
> +cpumask_t __read_mostly node_to_cpumask[MAX_NUMNODES];
> +
> +nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
> +
> +bool __ro_after_init numa_off;
> +
> +bool numa_disabled(void)
> +{
> +    return numa_off || arch_numa_disabled();
> +}
> +
> +/*
> + * Given a shift value, try to populate memnodemap[]
> + * Returns :
> + * 1 if OK
> + * 0 if memnodmap[] too small (of shift too small)

May I ask that you correct this comment line: "of" (alone) makes no sense
here. Either "or" was meant or it would want to be "because of". Unless
this is a language tweak I'm entirely unaware of ...

Jan