diff mbox

[RFC,v3,09/24] NUMA: x86: Move common code from srat.c

Message ID 1500378106-2620-10-git-send-email-vijay.kilari@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Vijay Kilari July 18, 2017, 11:41 a.m. UTC
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.

Apart from moving the code the following changes are done
 - Coding style of code moved to numa.c is changed to xen style
 - {memory,processor}_nodes_parsed are made global and moved
   to xen/nodemask.h
 - Few generic static functions in x86/srat.c are made
   non-static
 - Functions moved from x85/srat.c to common/numa.c are made
   non-static
 - numa_scan_nodes() is made as static function
 - compute_memnode_shift() and setup_node_bootmem() are made
   static.

Also {memory,processor}_nodes_parsed are made as global.
These are used across multiple code files. Adding helpers
to access these nodemask_t is complex.

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
---
v3: - Move declaration of {memory,processor}_nodes_parsed to header
      file
    - Drop redundant get_memblk() declaration
    - numa_scan_nodes(), setup_node_bootmem(), compute_memnode_shift()
      are made as static function
---
 xen/arch/x86/srat.c        | 151 +----------------------------------------
 xen/common/numa.c          | 165 +++++++++++++++++++++++++++++++++++++++++++--
 xen/include/asm-x86/acpi.h |   2 -
 xen/include/asm-x86/numa.h |   2 -
 xen/include/xen/nodemask.h |   2 +
 xen/include/xen/numa.h     |  13 ++--
 6 files changed, 174 insertions(+), 161 deletions(-)

Comments

Julien Grall July 20, 2017, 11:17 a.m. UTC | #1
Hi Vijay,

On 18/07/17 12:41, 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.
>
> Apart from moving the code the following changes are done
>  - Coding style of code moved to numa.c is changed to xen style
>  - {memory,processor}_nodes_parsed are made global and moved
>    to xen/nodemask.h
>  - Few generic static functions in x86/srat.c are made
>    non-static
>  - Functions moved from x85/srat.c to common/numa.c are made
>    non-static
>  - numa_scan_nodes() is made as static function
>  - compute_memnode_shift() and setup_node_bootmem() are made
>    static.

You modify the coding style at the same time as the same time as moving 
the code. This makes quite difficult to make sure that a mistake didn't 
slip in the new code. Can you please diving this patch in smaller chunk 
(i.e moving code in smaller chunk) to ease the review?

We can think of merging all of them when committing it.

Cheers,
Vijay Kilari July 20, 2017, 11:43 a.m. UTC | #2
Hi Julien,

On Thu, Jul 20, 2017 at 4:47 PM, Julien Grall <julien.grall@arm.com> wrote:
> Hi Vijay,
>
> On 18/07/17 12:41, 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.
>>
>> Apart from moving the code the following changes are done
>>  - Coding style of code moved to numa.c is changed to xen style
>>  - {memory,processor}_nodes_parsed are made global and moved
>>    to xen/nodemask.h
>>  - Few generic static functions in x86/srat.c are made
>>    non-static
>>  - Functions moved from x85/srat.c to common/numa.c are made
>>    non-static
>>  - numa_scan_nodes() is made as static function
>>  - compute_memnode_shift() and setup_node_bootmem() are made
>>    static.
>
>
> You modify the coding style at the same time as the same time as moving the
> code. This makes quite difficult to make sure that a mistake didn't slip in
> the new code. Can you please diving this patch in smaller chunk (i.e moving
> code in smaller chunk) to ease the review?

OK. I will do so.

>
> We can think of merging all of them when committing it.
>
> Cheers,
>
> --
> Julien Grall
Stefano Stabellini July 24, 2017, 8:35 p.m. UTC | #3
On Thu, 20 Jul 2017, Julien Grall wrote:
> Hi Vijay,
> 
> On 18/07/17 12:41, 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.
> > 
> > Apart from moving the code the following changes are done
> >  - Coding style of code moved to numa.c is changed to xen style
> >  - {memory,processor}_nodes_parsed are made global and moved
> >    to xen/nodemask.h
> >  - Few generic static functions in x86/srat.c are made
> >    non-static
> >  - Functions moved from x85/srat.c to common/numa.c are made
> >    non-static
> >  - numa_scan_nodes() is made as static function
> >  - compute_memnode_shift() and setup_node_bootmem() are made
> >    static.
> 
> You modify the coding style at the same time as the same time as moving the
> code. This makes quite difficult to make sure that a mistake didn't slip in
> the new code. Can you please diving this patch in smaller chunk (i.e moving
> code in smaller chunk) to ease the review?

For your reference and my own, the code movement in this patch is
correct. Splitting it would be good of course.


> We can think of merging all of them when committing it.
diff mbox

Patch

diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
index 03bc37d..be2634a 100644
--- a/xen/arch/x86/srat.c
+++ b/xen/arch/x86/srat.c
@@ -23,10 +23,6 @@ 
 
 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];
-
 struct pxm2node {
 	unsigned int pxm;
 	nodeid_t node;
@@ -36,49 +32,8 @@  static struct pxm2node __read_mostly pxm2node[MAX_NUMNODES] =
 
 static unsigned int 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];
 static __initdata DECLARE_BITMAP(memblk_hotplug, NR_NODE_MEMBLKS);
 
-static struct node *get_numa_node(unsigned int id)
-{
-	return &nodes[id];
-}
-
-static nodeid_t get_memblk_nodeid(unsigned int id)
-{
-	return memblk_nodeid[id];
-}
-
-static nodeid_t *get_memblk_nodeid_map(void)
-{
-	return &memblk_nodeid[0];
-}
-
-static struct node *get_node_memblk_range(unsigned int memblk)
-{
-	return &node_memblk_range[memblk];
-}
-
-static int get_num_node_memblks(void)
-{
-	return num_node_memblks;
-}
-
-static int __init numa_add_memblk(nodeid_t nodeid, paddr_t start, uint64_t size)
-{
-	if (nodeid >= NR_NODE_MEMBLKS)
-		return -EINVAL;
-
-	node_memblk_range[num_node_memblks].start = start;
-	node_memblk_range[num_node_memblks].end = start + size;
-	memblk_nodeid[num_node_memblks] = nodeid;
-	num_node_memblks++;
-
-	return 0;
-}
-
 static inline bool node_found(unsigned int idx, unsigned int pxm)
 {
 	return ((pxm2node[idx].pxm == pxm) &&
@@ -149,54 +104,7 @@  nodeid_t acpi_setup_node(unsigned int pxm)
 	return node;
 }
 
-int valid_numa_range(paddr_t start, paddr_t end, nodeid_t node)
-{
-	int i;
-
-	for (i = 0; i < get_num_node_memblks(); i++) {
-		struct node *nd = get_node_memblk_range(i);
-
-		if (nd->start <= start && nd->end > end &&
-			get_memblk_nodeid(i) == node)
-			return 1;
-	}
-
-	return 0;
-}
-
-static int __init conflicting_memblks(paddr_t start, paddr_t end)
-{
-	int i;
-
-	for (i = 0; i < get_num_node_memblks(); i++) {
-		struct node *nd = get_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 void __init cutoff_node(nodeid_t i, paddr_t start, paddr_t end)
-{
-	struct node *nd = get_numa_node(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 void __init numa_failed(void)
+void __init numa_failed(void)
 {
 	int i;
 	printk(KERN_ERR "SRAT: SRAT not used.\n");
@@ -412,7 +320,7 @@  acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma)
 
 /* Sanity check to catch more bad SRATs (they are amazingly common).
    Make sure the PXMs cover all memory. */
-static bool __init arch_sanitize_nodes_memory(void)
+bool __init arch_sanitize_nodes_memory(void)
 {
 	int i;
 
@@ -509,61 +417,6 @@  void __init srat_parse_regions(paddr_t addr)
 	pfn_pdx_hole_setup(mask >> PAGE_SHIFT);
 }
 
-/* Use the information discovered above to actually set up the nodes. */
-int __init numa_scan_nodes(paddr_t start, paddr_t end)
-{
-	unsigned int i;
-	nodemask_t all_nodes_parsed;
-	struct node *memblks;
-	nodeid_t *nodeids;
-
-	/* First clean up the node list */
-	for (i = 0; i < MAX_NUMNODES; i++)
-		cutoff_node(i, start, end);
-
-	if (acpi_numa <= 0)
-		return -1;
-
-	if (!arch_sanitize_nodes_memory()) {
-		numa_failed();
-		return -1;
-	}
-
-	memblks = get_node_memblk_range(0);
-	nodeids = get_memblk_nodeid_map();
-	if (compute_memnode_shift(node_memblk_range, num_node_memblks,
-				  memblk_nodeid)) {
-		memnode_shift = 0;
-		printk(KERN_ERR
-		     "SRAT: No NUMA node hash function found. Contact maintainer\n");
-		numa_failed();
-		return -1;
-	}
-
-	nodes_or(all_nodes_parsed, memory_nodes_parsed, processor_nodes_parsed);
-
-	/* Finally register nodes */
-	for_each_node_mask(i, all_nodes_parsed)
-	{
-		struct node *nd = get_numa_node(i);
-		uint64_t size = nd->end - nd->start;
-
-		if ( size == 0 )
-			printk(KERN_WARNING "SRAT: Node %u has no memory. "
-			       "BIOS Bug or mis-configured hardware?\n", i);
-
-		setup_node_bootmem(i, nd->start, nd->end);
-	}
-	for (i = 0; i < nr_cpu_ids; i++) {
-		if (cpu_to_node[i] == NUMA_NO_NODE)
-			continue;
-		if (!node_isset(cpu_to_node[i], processor_nodes_parsed))
-			numa_set_node(i, NUMA_NO_NODE);
-	}
-	numa_init_array();
-	return 0;
-}
-
 static unsigned int node_to_pxm(nodeid_t n)
 {
 	unsigned int i;
diff --git a/xen/common/numa.c b/xen/common/numa.c
index 0381f1b..74c4697 100644
--- a/xen/common/numa.c
+++ b/xen/common/numa.c
@@ -54,12 +54,109 @@  cpumask_t __read_mostly node_to_cpumask[MAX_NUMNODES];
 
 bool numa_off;
 s8 acpi_numa = 0;
+nodemask_t __initdata memory_nodes_parsed;
+nodemask_t __initdata processor_nodes_parsed;
+static struct node __initdata nodes[MAX_NUMNODES];
+static int num_node_memblks;
+static struct node node_memblk_range[NR_NODE_MEMBLKS];
+static nodeid_t memblk_nodeid[NR_NODE_MEMBLKS];
 
 int srat_disabled(void)
 {
     return numa_off || acpi_numa < 0;
 }
 
+struct node *get_numa_node(unsigned int id)
+{
+    return &nodes[id];
+}
+
+nodeid_t get_memblk_nodeid(unsigned int id)
+{
+    return memblk_nodeid[id];
+}
+
+static nodeid_t *get_memblk_nodeid_map(void)
+{
+    return &memblk_nodeid[0];
+}
+
+struct node *get_node_memblk_range(unsigned int memblk)
+{
+    return &node_memblk_range[memblk];
+}
+
+int get_num_node_memblks(void)
+{
+    return num_node_memblks;
+}
+
+int __init numa_add_memblk(nodeid_t nodeid, paddr_t start, uint64_t size)
+{
+    if ( nodeid >= NR_NODE_MEMBLKS )
+        return -EINVAL;
+
+    node_memblk_range[num_node_memblks].start = start;
+    node_memblk_range[num_node_memblks].end = start + size;
+    memblk_nodeid[num_node_memblks] = nodeid;
+    num_node_memblks++;
+
+    return 0;
+}
+
+int valid_numa_range(paddr_t start, paddr_t end, nodeid_t node)
+{
+    int i;
+
+    for ( i = 0; i < get_num_node_memblks(); i++ )
+    {
+        struct node *nd = get_node_memblk_range(i);
+
+        if ( nd->start <= start && nd->end > end &&
+             get_memblk_nodeid(i) == node )
+            return 1;
+    }
+
+    return 0;
+}
+
+int __init conflicting_memblks(paddr_t start, paddr_t end)
+{
+    int i;
+
+    for ( i = 0; i < get_num_node_memblks(); i++ )
+    {
+        struct node *nd = get_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 void __init cutoff_node(nodeid_t i, paddr_t start, paddr_t end)
+{
+    struct node *nd = get_numa_node(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 :
@@ -154,8 +251,8 @@  static unsigned int __init extract_lsb_from_nodes(const struct node *nodes,
     return i;
 }
 
-int __init compute_memnode_shift(struct node *nodes, unsigned int numnodes,
-                                 nodeid_t *nodeids)
+static int __init compute_memnode_shift(struct node *nodes, unsigned int numnodes,
+                                        nodeid_t *nodeids)
 {
     int ret;
 
@@ -180,7 +277,8 @@  int __init compute_memnode_shift(struct node *nodes, unsigned int numnodes,
     return 0;
 }
 /* initialize NODE_DATA given nodeid and start/end */
-void __init setup_node_bootmem(nodeid_t nodeid, paddr_t start, paddr_t end)
+static void __init setup_node_bootmem(nodeid_t nodeid, paddr_t start,
+                                      paddr_t end)
 {
     unsigned long start_pfn, end_pfn;
 
@@ -193,7 +291,7 @@  void __init setup_node_bootmem(nodeid_t nodeid, paddr_t start, paddr_t end)
     node_set_online(nodeid);
 }
 
-void __init numa_init_array(void)
+static void __init numa_init_array(void)
 {
     int rr, i;
 
@@ -214,6 +312,65 @@  void __init numa_init_array(void)
     }
 }
 
+/* Use the information discovered above to actually set up the nodes. */
+static int __init numa_scan_nodes(paddr_t start, paddr_t end)
+{
+    unsigned int i;
+    nodemask_t all_nodes_parsed;
+    struct node *memblks;
+    nodeid_t *nodeids;
+
+    /* First clean up the node list */
+    for ( i = 0; i < MAX_NUMNODES; i++ )
+        cutoff_node(i, start, end);
+
+    if ( acpi_numa <= 0 )
+        return -1;
+
+    if ( !arch_sanitize_nodes_memory() )
+    {
+        numa_failed();
+        return -1;
+    }
+
+    memblks = get_node_memblk_range(0);
+    nodeids = get_memblk_nodeid_map();
+    if ( compute_memnode_shift(node_memblk_range, num_node_memblks,
+                  memblk_nodeid) )
+    {
+        memnode_shift = 0;
+        printk(KERN_ERR
+               "SRAT: No NUMA node hash function found. Contact maintainer\n");
+        numa_failed();
+        return -1;
+    }
+
+    nodes_or(all_nodes_parsed, memory_nodes_parsed, processor_nodes_parsed);
+
+    /* Finally register nodes */
+    for_each_node_mask(i, all_nodes_parsed)
+    {
+        struct node *nd = get_numa_node(i);
+        uint64_t size = nd->end - nd->start;
+
+        if ( size == 0 )
+            printk(KERN_WARNING "SRAT: Node %u has no memory. "
+                   "BIOS Bug or mis-configured hardware?\n", i);
+
+        setup_node_bootmem(i, nd->start, nd->end);
+    }
+
+    for ( i = 0; i < nr_cpu_ids; i++ )
+    {
+        if (cpu_to_node[i] == NUMA_NO_NODE)
+            continue;
+        if (!node_isset(cpu_to_node[i], processor_nodes_parsed))
+            numa_set_node(i, NUMA_NO_NODE);
+    }
+    numa_init_array();
+    return 0;
+}
+
 #ifdef CONFIG_NUMA_EMU
 static unsigned int __initdata numa_fake;
 
diff --git a/xen/include/asm-x86/acpi.h b/xen/include/asm-x86/acpi.h
index a65c85f..59c34e7 100644
--- a/xen/include/asm-x86/acpi.h
+++ b/xen/include/asm-x86/acpi.h
@@ -103,8 +103,6 @@  extern void acpi_reserve_bootmem(void);
 
 #define ARCH_HAS_POWER_INIT	1
 
-extern int numa_scan_nodes(paddr_t start, paddr_t end);
-
 #ifdef CONFIG_ACPI_SLEEP
 
 extern struct acpi_sleep_info acpi_sinfo;
diff --git a/xen/include/asm-x86/numa.h b/xen/include/asm-x86/numa.h
index 41bb3ef..d8a0a44 100644
--- a/xen/include/asm-x86/numa.h
+++ b/xen/include/asm-x86/numa.h
@@ -17,8 +17,6 @@  extern void srat_detect_node(int cpu);
 extern nodeid_t apicid_to_node[];
 extern void init_cpu_to_node(void);
 
-extern int valid_numa_range(paddr_t start, paddr_t end, nodeid_t node);
-
 void srat_parse_regions(paddr_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/nodemask.h b/xen/include/xen/nodemask.h
index 2a90dc1..c52c110 100644
--- a/xen/include/xen/nodemask.h
+++ b/xen/include/xen/nodemask.h
@@ -80,6 +80,8 @@ 
 
 typedef struct { DECLARE_BITMAP(bits, MAX_NUMNODES); } nodemask_t;
 extern nodemask_t _unused_nodemask_arg_;
+extern nodemask_t processor_nodes_parsed;
+extern nodemask_t memory_nodes_parsed;
 
 #define node_set(node, dst) __node_set((node), &(dst))
 static inline void __node_set(int node, volatile nodemask_t *dstp)
diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
index c6bbbdf..110d5dc 100644
--- a/xen/include/xen/numa.h
+++ b/xen/include/xen/numa.h
@@ -26,11 +26,8 @@  extern bool numa_off;
 extern s8 acpi_numa;
 
 void numa_initmem_init(unsigned long start_pfn, unsigned long end_pfn);
-int compute_memnode_shift(struct node *nodes, unsigned int numnodes,
-                          nodeid_t *nodeids);
 int srat_disabled(void);
-void numa_init_array(void);
-void setup_node_bootmem(nodeid_t nodeid, paddr_t start, paddr_t end);
+int valid_numa_range(paddr_t start, paddr_t end, nodeid_t node);
 
 #ifdef CONFIG_NUMA
 #define cpu_to_node(cpu)         (cpu_to_node[cpu])
@@ -65,6 +62,14 @@  static inline __attribute_pure__ nodeid_t phys_to_nid(paddr_t addr)
 
 void numa_add_cpu(int cpu);
 void numa_set_node(int cpu, nodeid_t node);
+int conflicting_memblks(paddr_t start, paddr_t end);
+struct node *get_numa_node(unsigned int id);
+nodeid_t get_memblk_nodeid(unsigned int memblk);
+struct node *get_node_memblk_range(unsigned int memblk);
+int numa_add_memblk(nodeid_t nodeid, paddr_t start, uint64_t size);
+int get_num_node_memblks(void);
+bool arch_sanitize_nodes_memory(void);
+void numa_failed(void);
 #else
 static inline void numa_add_cpu(int cpu) { }
 static inline void numa_set_node(int cpu, nodeid_t node) { }