Re: [RFC PATCH v1 03/21] NUMA: Move arch specific NUMA code as common

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

Hello Vijay,

On 09/02/17 15:56, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
>
> Move some common numa code from xen/arch/x86/srat.c
> to xen/common/numa.c
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> ---
>  xen/arch/x86/srat.c        | 54 ++++-----------------------------------------
>  xen/common/numa.c          | 55 ++++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-x86/acpi.h |  1 -
>  xen/include/asm-x86/numa.h |  1 -
>  xen/include/xen/numa.h     |  5 ++++-
>  5 files changed, 63 insertions(+), 53 deletions(-)
>
> diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
> index d86783e..58dee09 100644
> --- a/xen/arch/x86/srat.c
> +++ b/xen/arch/x86/srat.c
> @@ -25,7 +25,7 @@ static struct acpi_table_slit *__read_mostly acpi_slit;
>
>  static nodemask_t memory_nodes_parsed __initdata;
>  static nodemask_t processor_nodes_parsed __initdata;
> -static struct node nodes[MAX_NUMNODES] __initdata;
> +extern struct node nodes[MAX_NUMNODES] __initdata;
>
>  struct pxm2node {
>  	unsigned pxm;
> @@ -36,9 +36,9 @@ static struct pxm2node __read_mostly pxm2node[MAX_NUMNODES] =
>
>  static unsigned node_to_pxm(nodeid_t n);
>
> -static int num_node_memblks;
> -static struct node node_memblk_range[NR_NODE_MEMBLKS];
> -static nodeid_t memblk_nodeid[NR_NODE_MEMBLKS];
> +extern int num_node_memblks;
> +extern struct node node_memblk_range[NR_NODE_MEMBLKS];
> +extern nodeid_t memblk_nodeid[NR_NODE_MEMBLKS];
>  static __initdata DECLARE_BITMAP(memblk_hotplug, NR_NODE_MEMBLKS);
>
>  static inline bool_t node_found(unsigned idx, unsigned pxm)
> @@ -103,52 +103,6 @@ nodeid_t setup_node(unsigned pxm)
>  	return node;
>  }
>
> -int valid_numa_range(u64 start, u64 end, nodeid_t node)
> -{
> -	int i;
> -
> -	for (i = 0; i < num_node_memblks; i++) {
> -		struct node *nd = &node_memblk_range[i];
> -
> -		if (nd->start <= start && nd->end > end &&
> -			memblk_nodeid[i] == node )
> -			return 1;
> -	}
> -
> -	return 0;
> -}
> -
> -static __init int conflicting_memblks(u64 start, u64 end)
> -{
> -	int i;
> -
> -	for (i = 0; i < num_node_memblks; i++) {
> -		struct node *nd = &node_memblk_range[i];
> -		if (nd->start == nd->end)
> -			continue;
> -		if (nd->end > start && nd->start < end)
> -			return i;
> -		if (nd->end == end && nd->start == start)
> -			return i;
> -	}
> -	return -1;
> -}
> -
> -static __init void cutoff_node(int i, u64 start, u64 end)
> -{
> -	struct node *nd = &nodes[i];
> -	if (nd->start < start) {
> -		nd->start = start;
> -		if (nd->end < nd->start)
> -			nd->start = nd->end;
> -	}
> -	if (nd->end > end) {
> -		nd->end = end;
> -		if (nd->start > nd->end)
> -			nd->start = nd->end;
> -	}
> -}
> -
>  static __init void bad_srat(void)
>  {
>  	int i;
> diff --git a/xen/common/numa.c b/xen/common/numa.c
> index 59dcb63..13f147c 100644
> --- a/xen/common/numa.c
> +++ b/xen/common/numa.c
> @@ -46,6 +46,61 @@ nodeid_t cpu_to_node[NR_CPUS] __read_mostly = {
>
>  cpumask_t node_to_cpumask[MAX_NUMNODES] __read_mostly;
>
> +int num_node_memblks;
> +struct node node_memblk_range[NR_NODE_MEMBLKS];
> +nodeid_t memblk_nodeid[NR_NODE_MEMBLKS];
> +struct node nodes[MAX_NUMNODES] __initdata;
> +
> +int valid_numa_range(u64 start, u64 end, nodeid_t node)

I am not sure why you move this code in common code when it is not even 
used in your series.

Furthermore, please use paddr_t rather than u64.

> +{
> +#ifdef CONFIG_NUMA

common/numa.c should really not be compiled at all for configuration not 
supporting NUMA. In other words, I really don't want to see #ifdefery in 
common/numa.c.

> +    int i;
> +
> +    for (i = 0; i < num_node_memblks; i++) {

I know you are moving code around, but fix the coding style before hand 
would have been appreciated.

> +        struct node *nd = &node_memblk_range[i];
> +
> +        if (nd->start <= start && nd->end > end &&
> +            memblk_nodeid[i] == node )
> +            return 1;
> +    }
> +
> +    return 0;
> +#else
> +    return 1;
> +#endif
> +}
> +
> +__init int conflicting_memblks(u64 start, u64 end)

Ditto for u64.

> +{
> +    int i;
> +
> +    for (i = 0; i < num_node_memblks; i++) {
> +        struct node *nd = &node_memblk_range[i];
> +        if (nd->start == nd->end)
> +            continue;
> +        if (nd->end > start && nd->start < end)
> +            return i;
> +        if (nd->end == end && nd->start == start)
> +            return i;
> +    }
> +    return -1;
> +}
> +
> +__init void cutoff_node(int i, u64 start, u64 end)

Same remark as above.

> +{
> +    struct node *nd = &nodes[i];
> +    if (nd->start < start) {
> +        nd->start = start;
> +        if (nd->end < nd->start)
> +            nd->start = nd->end;
> +    }
> +    if (nd->end > end) {
> +        nd->end = end;
> +        if (nd->start > nd->end)
> +            nd->start = nd->end;
> +    }
> +}
> +
>  /*
>   * Given a shift value, try to populate memnodemap[]
>   * Returns :
> diff --git a/xen/include/asm-x86/acpi.h b/xen/include/asm-x86/acpi.h
> index d36bee9..f1a8e9d 100644
> --- a/xen/include/asm-x86/acpi.h
> +++ b/xen/include/asm-x86/acpi.h
> @@ -106,7 +106,6 @@ extern void acpi_reserve_bootmem(void);
>
>  extern s8 acpi_numa;
>  extern int acpi_scan_nodes(u64 start, u64 end);
> -#define NR_NODE_MEMBLKS (MAX_NUMNODES*2)
>
>  #ifdef CONFIG_ACPI_SLEEP
>
> diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h
> index 61bcd8e..df1f7d5 100644
> --- a/xen/include/asm-x86/numa.h
> +++ b/xen/include/asm-x86/numa.h
> @@ -28,7 +28,6 @@ extern int srat_disabled(void);
>  extern void srat_detect_node(int cpu);
>
>  extern nodeid_t apicid_to_node[];
> -extern int valid_numa_range(u64 start, u64 end, nodeid_t node);
>
>  void srat_parse_regions(u64 addr);
>  extern u8 __node_distance(nodeid_t a, nodeid_t b);
> diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
> index dd33c92..810f742 100644
> --- a/xen/include/xen/numa.h
> +++ b/xen/include/xen/numa.h
> @@ -11,7 +11,7 @@
>  #define NUMA_NO_DISTANCE 0xFF
>
>  #define MAX_NUMNODES    (1 << NODES_SHIFT)
> -

Spurious change.

> +#define NR_NODE_MEMBLKS (MAX_NUMNODES*2)
>  #define vcpu_to_node(v) (cpu_to_node((v)->processor))
>
>  #define domain_to_node(d) \
> @@ -66,6 +66,9 @@ static inline __attribute__((pure)) nodeid_t phys_to_nid(paddr_t addr)
>  #define clear_node_cpumask(cpu) do {} while (0)
>  #endif /* CONFIG_NUMA */
>
> +extern int valid_numa_range(u64 start, u64 end, nodeid_t node);
> +extern int conflicting_memblks(u64 start, u64 end);
> +extern void cutoff_node(int i, u64 start, u64 end);
>  extern void numa_add_cpu(int cpu);
>  extern nodeid_t setup_node(unsigned int pxm);
>  extern void numa_set_node(int cpu, nodeid_t node);
>

Regards,

-- 
Julien Grall

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