|
[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
On Thu, Mar 2, 2017 at 9:35 PM, Julien Grall <julien.grall@xxxxxxx> wrote:
> 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?
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/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?
OK. Wll check
>
>> +{
>> + 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |