Re: [Xen-devel] [RFC PATCH v2 10/25] x86: NUMA: Move numa code and make it generic

[Julien Grall <julien.grall@xxxxxxx>]

Hi Vijay,

On 28/03/17 16:53, vijay.kilari@xxxxxxxxx wrote:
> diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
> index 3bdab9a..33c6806 100644
> --- a/xen/arch/x86/numa.c
> +++ b/xen/arch/x86/numa.c
> @@ -10,286 +10,13 @@
>  #include <xen/ctype.h>
>  #include <xen/nodemask.h>
>  #include <xen/numa.h>
> -#include <xen/keyhandler.h>
>  #include <xen/time.h>
>  #include <xen/smp.h>
>  #include <xen/pfn.h>
>  #include <asm/acpi.h>
> -#include <xen/sched.h>
> -#include <xen/softirq.h>
> -
> -static int numa_setup(char *s);
> -custom_param("numa", numa_setup);
> -
> -struct node_data node_data[MAX_NUMNODES];
> -
> -/* Mapping from pdx to node id */
> -unsigned int memnode_shift;
> -static typeof(*memnodemap) _memnodemap[64];
> -unsigned long memnodemapsize;
> -uint8_t *memnodemap;
> -
> -nodeid_t __read_mostly cpu_to_node[NR_CPUS] = {
> -    [0 ... NR_CPUS-1] = NUMA_NO_NODE
> -};
> -/*
> - * Keep BIOS's CPU2node information, should not be used for memory allocaion
> - */
> -nodeid_t apicid_to_node[MAX_LOCAL_APIC] = {
> -    [0 ... MAX_LOCAL_APIC-1] = NUMA_NO_NODE
> -};
Why this is moved in this patch from here to x86/srat.c?

[...]

> diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
> index 7cf4771..2cc87a3 100644
> --- a/xen/arch/x86/srat.c
> +++ b/xen/arch/x86/srat.c
> @@ -27,6 +27,13 @@ static nodemask_t __initdata memory_nodes_parsed;
>  static nodemask_t __initdata processor_nodes_parsed;
>  static struct node __initdata nodes[MAX_NUMNODES];
>
> +/*
> + * Keep BIOS's CPU2node information, should not be used for memory allocaion
> + */
> +nodeid_t apicid_to_node[MAX_LOCAL_APIC] = {
> +    [0 ... MAX_LOCAL_APIC-1] = NUMA_NO_NODE
> +};
> +
This does not belong to this patch...

>  struct pxm2node {
>         unsigned int pxm;
>         nodeid_t node;
[...]

> diff --git a/xen/common/numa.c b/xen/common/numa.c
> new file mode 100644
> index 0000000..207ebd8
> --- /dev/null
> +++ b/xen/common/numa.c
> @@ -0,0 +1,488 @@
> +/*
> + * Common NUMA handling functions for x86 and arm.
> + * Original code extracted from arch/x86/numa.c
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms and conditions 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/mm.h>
> +#include <xen/string.h>
> +#include <xen/init.h>
> +#include <xen/ctype.h>
> +#include <xen/nodemask.h>
> +#include <xen/numa.h>
> +#include <xen/keyhandler.h>
> +#include <xen/time.h>
> +#include <xen/smp.h>
> +#include <xen/pfn.h>
> +#include <asm/acpi.h>
> +#include <xen/sched.h>
> +#include <xen/softirq.h>
Whilst you are moving this in a newfile, please order the includes.

[...]

> +static unsigned int __init extract_lsb_from_nodes(const struct node *nodes,
> +                                                  int numnodes)
> +{
> +    unsigned int i, nodes_used = 0;
> +    unsigned long spdx, epdx;
> +    unsigned long bitfield = 0, memtop = 0;
> +
> +    for ( i = 0; i < numnodes; i++ )
> +    {
> +        spdx = paddr_to_pdx(nodes[i].start);
> +        epdx = paddr_to_pdx(nodes[i].end - 1) + 1;
> +        if ( spdx >= epdx )
> +            continue;
> +        bitfield |= spdx;
> +        nodes_used++;
> +        if ( epdx > memtop )
> +            memtop = epdx;
> +    }
> +    if ( nodes_used <= 1 )
> +        i = BITS_PER_LONG - 1;
> +    else
> +        i = find_first_bit(&bitfield, sizeof(unsigned long) * 8);
> +
It is interesting to see that newline was added in the process of moving 
the code.
> +    memnodemapsize = (memtop >> i) + 1;
[....]

> diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
> index 922fbd8..eed40af 100644
> --- a/xen/include/xen/numa.h
> +++ b/xen/include/xen/numa.h
> @@ -14,6 +14,21 @@
>
>  #define MAX_NUMNODES    (1 << NODES_SHIFT)
>
> +struct node {
> +    paddr_t start;
> +    paddr_t end;
> +};
> +
> +extern int compute_memnode_shift(struct node *nodes, int numnodes,
> +                                 nodeid_t *nodeids, unsigned int *shift);
> +extern void numa_init_array(void);
> +extern bool_t srat_disabled(void);
> +extern void numa_set_node(int cpu, nodeid_t node);
> +extern nodeid_t acpi_setup_node(unsigned int pxm);
> +extern void srat_detect_node(int cpu);
> +extern void setup_node_bootmem(nodeid_t nodeid, paddr_t start, paddr_t end);
> +extern void init_cpu_to_node(void);
Can you please be consistent with this file and drop the unecessary 
"extern".
> +
>  #define vcpu_to_node(v) (cpu_to_node((v)->processor))
>
>  #define domain_to_node(d) \
> @@ -23,4 +38,7 @@
>  bool is_numa_off(void);
>  bool get_acpi_numa(void);
>  void set_acpi_numa(bool val);
> +int get_numa_fake(void);
> +extern int numa_emulation(uint64_t start_pfn, uint64_t end_pfn);
> +extern void numa_dummy_init(uint64_t start_pfn, uint64_t end_pfn);
Ditto.

>  #endif /* _XEN_NUMA_H */
--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel