Re: [RFC PATCH v2 11/25] x86: NUMA: Move common code from srat.c

[Julien Grall <julien.grall@arm.com>]

Hi Vijay,

On 28/03/17 16:53, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>
> Move code from xen/arch/x86/srat.c to xen/common/numa.c
> so that it can be used by other archs.
> Few generic static functions in x86/srat.c are made
> non-static common/numa.c
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> ---
>  xen/arch/x86/srat.c        | 152 ++-------------------------------------------
>  xen/common/numa.c          | 146 +++++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-x86/acpi.h |   3 -
>  xen/include/asm-x86/numa.h |   2 -
>  xen/include/xen/numa.h     |  14 +++++
>  5 files changed, 164 insertions(+), 153 deletions(-)
>
> diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
> index 2cc87a3..55947bb 100644
> --- a/xen/arch/x86/srat.c
> +++ b/xen/arch/x86/srat.c
> @@ -23,9 +23,8 @@
>
>  static struct acpi_table_slit *__read_mostly acpi_slit;
>
> -static nodemask_t __initdata memory_nodes_parsed;
> -static nodemask_t __initdata processor_nodes_parsed;
> -static struct node __initdata nodes[MAX_NUMNODES];
> +extern nodemask_t processor_nodes_parsed;
> +extern nodemask_t memory_nodes_parsed;

On v1, Jan clearly NAK to changes like this. Declarations belong in 
header files. It is a different variable compare to v1, but I would have 
expected you to apply what he said everywhere...

[...]

> diff --git a/xen/common/numa.c b/xen/common/numa.c
> index 207ebd8..1789bba 100644
> --- a/xen/common/numa.c
> +++ b/xen/common/numa.c
> @@ -32,6 +32,8 @@
>  static int numa_setup(char *s);
>  custom_param("numa", numa_setup);
>
> +nodemask_t __initdata memory_nodes_parsed;
> +nodemask_t __initdata processor_nodes_parsed;
>  struct node_data node_data[MAX_NUMNODES];
>
>  /* Mapping from pdx to node id */
> @@ -47,6 +49,10 @@ cpumask_t __read_mostly node_to_cpumask[MAX_NUMNODES];
>
>  static bool numa_off = 0;
>  static bool acpi_numa = 1;
> +static int num_node_memblks;
> +static struct node node_memblk_range[NR_NODE_MEMBLKS];
> +static nodeid_t memblk_nodeid[NR_NODE_MEMBLKS];
> +static struct node __initdata nodes[MAX_NUMNODES];

It would make sense to keep those variables together with 
{memory,processor}_nodes_parsed.

[...]

> +int valid_numa_range(paddr_t start, paddr_t end, nodeid_t node)
> +{
> +    int i;
> +
> +    for (i = 0; i < get_num_node_memblks(); i++) {

common/numa.c is using Xen coding style whilst arch/x86/srat.c is using 
Linux coding style.

You decided to validly switch to soft tab, making quite difficult to 
check if this patch is only code movement. But you did not go far enough 
and fix the coding style of the code moved.

Please do it properly and not half of it. For simplicity I would be OK 
that it is done in this patch. But this needs to be clearly written in 
the commit message.

> +        struct node *nd = get_node_memblk_range(i);
> +
> +        if (nd->start <= start && nd->end > end &&
> +            get_memblk_nodeid(i) == node)
> +            return 1;
> +    }
> +
> +    return 0;
> +}

[...]

> diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h
> index 421e8b7..7cff220 100644
> --- a/xen/include/asm-x86/numa.h
> +++ b/xen/include/asm-x86/numa.h
> @@ -47,8 +47,6 @@ static inline __attribute__((pure)) nodeid_t phys_to_nid(paddr_t addr)
>  #define node_end_pfn(nid)       (NODE_DATA(nid)->node_start_pfn + \
>                                   NODE_DATA(nid)->node_spanned_pages)
>
> -extern int valid_numa_range(paddr_t start, paddr_t end, nodeid_t node);
> -
>  void srat_parse_regions(uint64_t addr);
>  extern uint8_t __node_distance(nodeid_t a, nodeid_t b);
>  unsigned int arch_get_dma_bitsize(void);
> diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
> index eed40af..ee53526 100644
> --- a/xen/include/xen/numa.h
> +++ b/xen/include/xen/numa.h
> @@ -13,6 +13,7 @@
>  #define NUMA_NO_DISTANCE 0xFF
>
>  #define MAX_NUMNODES    (1 << NODES_SHIFT)
> +#define NR_NODE_MEMBLKS (MAX_NUMNODES * 2)
>
>  struct node {
>      paddr_t start;
> @@ -28,6 +29,19 @@ 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);
> +extern int valid_numa_range(paddr_t start, paddr_t end, nodeid_t node);
> +extern int conflicting_memblks(paddr_t start, paddr_t end);
> +extern void cutoff_node(int i, paddr_t start, paddr_t end);
> +extern struct node *get_numa_node(int id);
> +extern nodeid_t get_memblk_nodeid(int memblk);
> +extern nodeid_t *get_memblk_nodeid_map(void);
> +extern struct node *get_node_memblk_range(int memblk);
> +extern struct node *get_memblk(int memblk);
> +extern int numa_add_memblk(nodeid_t nodeid, paddr_t start, uint64_t size);
> +extern int get_num_node_memblks(void);
> +extern int arch_sanitize_nodes_memory(void);
> +extern void numa_failed(void);
> +extern int numa_scan_nodes(uint64_t start, uint64_t end);

See my comment on the previous patch.

>
>  #define vcpu_to_node(v) (cpu_to_node((v)->processor))
>
>

Cheers,

-- 
Julien Grall

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