Re: [Xen-devel] [RFC PATCH v1 10/21] ARM: NUMA: Add memory NUMA support
On Thu, Mar 2, 2017 at 9:35 PM, Julien Grall wrote:
> Hello Vijay,
>
>
> On 09/02/17 15:57, vijay.kil...@gmail.com wrote:
>>
>> From: Vijaya Kumar K
>>
>> 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
>> ---
>> 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
>> #include
>> #include
>> +#include
>>
>> 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?
node_memblk_range[] contains memory bank to node info read from DT memory node.
We go through bootinfo.mem and populate nodes[] with actual memory
range indexed by
nodeid.
I think this is similar to nodes_cover_memory in arch/x86/srat.c
>
>> +
>> +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.
>
Yes, I have already fixed it in next revision.
>
>> +{
>> +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.
OK. I have changed this in next revision.
>
>> +
>> +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/xe
Re: [Xen-devel] [RFC PATCH v1 10/21] ARM: NUMA: Add memory NUMA support
Hello Vijay,
On 09/02/17 15:57, vijay.kil...@gmail.com wrote:
From: Vijaya Kumar K
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
---
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
#include
#include
+#include
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 @@ stat
[Xen-devel] [RFC PATCH v1 10/21] ARM: NUMA: Add memory NUMA support
From: Vijaya Kumar K
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
---
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
#include
#include
+#include
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)
+{
+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 )
+{
+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;
+}
+}
+
+return 0;
+}
+
+/* Use the information discovered above to actually set up the nodes. */
+static int __init numa_scan_mem_nodes(void)
+{
+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;
+}
+
+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)
+{
+int i;
+
+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);
--
2.7.4
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
