[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.