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

Re: [Xen-devel] [RFC PATCH v1 10/21] ARM: NUMA: Add memory NUMA support



Hello Vijay,

On 09/02/17 15:57, vijay.kilari@xxxxxxxxx wrote:
From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>

For all banks in bootinfo.mem, update nodes[] with
corresponding nodeid and register these nodes by
calling setup_node_bootmem().
compute memnode_shift and initialize memnodemap[] to fetch
nodeid for a given physical address.

Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>
---
 xen/arch/arm/numa.c    | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++
 xen/common/numa.c      | 14 ++++++++
 xen/include/xen/numa.h |  1 +
 3 files changed, 105 insertions(+)

diff --git a/xen/arch/arm/numa.c b/xen/arch/arm/numa.c
index d4dbad4..aa34c82 100644
--- a/xen/arch/arm/numa.c
+++ b/xen/arch/arm/numa.c
@@ -24,10 +24,15 @@
 #include <asm/acpi.h>
 #include <xen/errno.h>
 #include <xen/cpumask.h>
+#include <asm/setup.h>

 int _node_distance[MAX_NUMNODES * 2];
 int *node_distance;
 extern nodemask_t numa_nodes_parsed;
+extern struct node nodes[MAX_NUMNODES] __initdata;
+extern int num_node_memblks;
+extern struct node node_memblk_range[NR_NODE_MEMBLKS];
+extern nodeid_t memblk_nodeid[NR_NODE_MEMBLKS];

 void __init numa_set_cpu_node(int cpu, unsigned long hwid)
 {
@@ -51,6 +56,88 @@ u8 __node_distance(nodeid_t a, nodeid_t b)

 EXPORT_SYMBOL(__node_distance);

+static int __init numa_mem_init(void)

The function return only 2 different value either 0 or -EINVAL. It should be better to use a boolean.

+{
+    nodemask_t memory_nodes_parsed;
+    int bank, nodeid;
+    struct node *nd;
+    paddr_t start, size, end;
+
+    nodes_clear(memory_nodes_parsed);
+    for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ )
+    {
+        start = bootinfo.mem.bank[bank].start;
+        size = bootinfo.mem.bank[bank].size;
+        end = start + size;
+
+        nodeid = get_numa_node(start, end);
+        if ( nodeid == -EINVAL || nodeid > MAX_NUMNODES )

If found, how the nodeid could be invalid? IHMO, this is a parsing bug.

+        {
+            printk(XENLOG_WARNING
+                   "NUMA: node for mem bank start 0x%lx - 0x%lx not found\n",
+                   start, end);
+
+            return -EINVAL;
+        }
+
+        nd = &nodes[nodeid];
+        if ( !node_test_and_set(nodeid, memory_nodes_parsed) )
+        {
+            nd->start = start;
+            nd->end = end;
+        }
+        else
+        {
+            if ( start < nd->start )
+                nd->start = start;
+            if ( nd->end < end )
+                nd->end = end;
+        }
+    }

This function is quite confusing. What is the purpose? Why do you go through bootinfo.mem and not node_memblk_range?

+
+    return 0;
+}
+
+/* Use the information discovered above to actually set up the nodes. */
+static int __init numa_scan_mem_nodes(void)

This function looks very similar to acpi_scan_nodes on x86. This is call to move more code in common.

+{
+    int i;
+
+    memnode_shift = compute_hash_shift(node_memblk_range, num_node_memblks,
+                                       memblk_nodeid);
+    if ( memnode_shift < 0 )
+    {
+        printk(XENLOG_WARNING "No NUMA hash found.\n");
+        memnode_shift = 0;
+    }
+
+    for_each_node_mask(i, numa_nodes_parsed)
+    {
+        u64 size = node_memblk_range[i].end - node_memblk_range[i].start;
+
+        if ( size == 0 )
+            printk(XENLOG_WARNING "NUMA: Node %u has no memory. \n", i);
+
+        printk(XENLOG_INFO
+               "NUMA: NODE[%d]: Start 0x%lx End 0x%lx\n",
+               i, nodes[i].start, nodes[i].end);
+        setup_node_bootmem(i, nodes[i].start, nodes[i].end);
+    }
+
+    return 0;
+}
+
+static int __init numa_initmem_init(void)
+{
+    if ( !numa_mem_init() )
+    {
+        if ( !numa_scan_mem_nodes() )
+            return 0;
+    }

This could be simplified with

if ( !numa_mem_init() && !num_scan_mem_nodes )
   return 0;

However looking at the usage, this function seems rather pointless.

+
+    return -EINVAL;
+}
+
 /*
  * Setup early cpu_to_node.
  */
@@ -74,6 +161,9 @@ int __init numa_init(void)

     ret = dt_numa_init();

+    if ( !ret )
+        ret = numa_initmem_init();
+
 no_numa:
     return ret;
 }
diff --git a/xen/common/numa.c b/xen/common/numa.c
index 62c76af..2f5266a 100644
--- a/xen/common/numa.c
+++ b/xen/common/numa.c
@@ -63,6 +63,20 @@ void __init numa_add_memblk(nodeid_t nodeid, u64 start, u64 
size)
     num_node_memblks++;
 }

+int __init get_numa_node(u64 start, u64 end)

I think this function will be confusing for the caller. Why using num_node_memblks and not the node itself?

+{
+    int i;

Please unsigned int

+
+    for ( i = 0; i < num_node_memblks; i++ )
+    {
+        if ( start >= node_memblk_range[i].start &&
+             end <= node_memblk_range[i].end )
+            return memblk_nodeid[i];
+    }
+
+    return -EINVAL;
+}
+
 int valid_numa_range(u64 start, u64 end, nodeid_t node)
 {
 #ifdef CONFIG_NUMA
diff --git a/xen/include/xen/numa.h b/xen/include/xen/numa.h
index 9392a89..4f04ab4 100644
--- a/xen/include/xen/numa.h
+++ b/xen/include/xen/numa.h
@@ -68,6 +68,7 @@ static inline __attribute__((pure)) nodeid_t 
phys_to_nid(paddr_t addr)
 #endif /* CONFIG_NUMA */

 extern void numa_add_memblk(nodeid_t nodeid, u64 start, u64 size);
+extern int get_numa_node(u64 start, u64 end);
 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);


Regards,

--
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®.