| Message ID | 20210811102423.28908-4-wei.chen@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add device tree based NUMA support to Arm64 | expand |
On 11.08.2021 12:23, Wei Chen wrote: > When system turns NUMA off or system lacks of NUMA support, > Xen will fake a NUMA node to make system works as a single > node NUMA system. > > In this case the memory node map doesn't need to be allocated > from boot pages. But we should set the memnodemapsize to the > array size of _memnodemap. Xen hadn't done it, and Xen should > assert in phys_to_nid. But because x86 was using an empty > macro "VIRTUAL_BUG_ON" to replace ASSERT, this bug will not > be triggered. How about we promote VIRTUAL_BUG_ON() to expand to at least ASSERT()? > --- a/xen/arch/x86/numa.c > +++ b/xen/arch/x86/numa.c > @@ -270,6 +270,8 @@ void __init numa_initmem_init(unsigned long start_pfn, unsigned long end_pfn) > /* setup dummy node covering all memory */ > memnode_shift = BITS_PER_LONG - 1; > memnodemap = _memnodemap; > + memnodemapsize = ARRAY_SIZE(_memnodemap); But this doesn't reflect reality then, does it? We'd rather want to set the size to 1, I would think. Jan
Hi Jan, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: 2021年8月12日 23:33 > To: Wei Chen <Wei.Chen@arm.com> > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; xen- > devel@lists.xenproject.org; sstabellini@kernel.org; julien@xen.org > Subject: Re: [XEN RFC PATCH 03/40] xen/x86: Initialize memnodemapsize > while faking NUMA node > > On 11.08.2021 12:23, Wei Chen wrote: > > When system turns NUMA off or system lacks of NUMA support, > > Xen will fake a NUMA node to make system works as a single > > node NUMA system. > > > > In this case the memory node map doesn't need to be allocated > > from boot pages. But we should set the memnodemapsize to the > > array size of _memnodemap. Xen hadn't done it, and Xen should > > assert in phys_to_nid. But because x86 was using an empty > > macro "VIRTUAL_BUG_ON" to replace ASSERT, this bug will not > > be triggered. > > How about we promote VIRTUAL_BUG_ON() to expand to at least ASSERT()? > That would be good. Frankly, we discovered this because we used ASSERT in Arm and then noticed that x86 was using VIRTUAL_BUG_ON. > > --- a/xen/arch/x86/numa.c > > +++ b/xen/arch/x86/numa.c > > @@ -270,6 +270,8 @@ void __init numa_initmem_init(unsigned long > start_pfn, unsigned long end_pfn) > > /* setup dummy node covering all memory */ > > memnode_shift = BITS_PER_LONG - 1; > > memnodemap = _memnodemap; > > + memnodemapsize = ARRAY_SIZE(_memnodemap); > > But this doesn't reflect reality then, does it? We'd rather want to > set the size to 1, I would think. > Yes, you're right. Actually, we just only used 1 slot. But furthermore, memnodemap[0] may be set in acpi_scan_nodes, but acpi_scan_nodes doesn't reset memnodemap when it failed. I think maybe we can add: memnodemap[0] = 0; memnodemapsize = 1; How do you think about it? > Jan
On 13.08.2021 09:26, Wei Chen wrote: >> From: Jan Beulich <jbeulich@suse.com> >> Sent: 2021年8月12日 23:33 >> >> On 11.08.2021 12:23, Wei Chen wrote: >>> --- a/xen/arch/x86/numa.c >>> +++ b/xen/arch/x86/numa.c >>> @@ -270,6 +270,8 @@ void __init numa_initmem_init(unsigned long >> start_pfn, unsigned long end_pfn) >>> /* setup dummy node covering all memory */ >>> memnode_shift = BITS_PER_LONG - 1; >>> memnodemap = _memnodemap; >>> + memnodemapsize = ARRAY_SIZE(_memnodemap); >> >> But this doesn't reflect reality then, does it? We'd rather want to >> set the size to 1, I would think. >> > > Yes, you're right. Actually, we just only used 1 slot. But furthermore, > memnodemap[0] may be set in acpi_scan_nodes, but acpi_scan_nodes doesn't > reset memnodemap when it failed. I think maybe we can add: > memnodemap[0] = 0; > memnodemapsize = 1; > How do you think about it? Well, yes, if data may have been put there, then resetting of course makes sense. Jan
diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c index f1066c59c7..d23f4f7919 100644 --- a/xen/arch/x86/numa.c +++ b/xen/arch/x86/numa.c @@ -270,6 +270,8 @@ void __init numa_initmem_init(unsigned long start_pfn, unsigned long end_pfn) /* setup dummy node covering all memory */ memnode_shift = BITS_PER_LONG - 1; memnodemap = _memnodemap; + memnodemapsize = ARRAY_SIZE(_memnodemap); + nodes_clear(node_online_map); node_set_online(0); for ( i = 0; i < nr_cpu_ids; i++ )
When system turns NUMA off or system lacks of NUMA support, Xen will fake a NUMA node to make system works as a single node NUMA system. In this case the memory node map doesn't need to be allocated from boot pages. But we should set the memnodemapsize to the array size of _memnodemap. Xen hadn't done it, and Xen should assert in phys_to_nid. But because x86 was using an empty macro "VIRTUAL_BUG_ON" to replace ASSERT, this bug will not be triggered. In this patch, we set memnodemapsize to ARRAY_SIZE(_memnodemap) to fix it. Signed-off-by: Wei Chen <wei.chen@arm.com> --- xen/arch/x86/numa.c | 2 ++ 1 file changed, 2 insertions(+)