Hi Jan,
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: 2022年1月19日 0:13
> To: Wei Chen <wei.c...@arm.com>
> Cc: Bertrand Marquis <bertrand.marq...@arm.com>; xen-
> de...@lists.xenproject.org; sstabell...@kernel.org; jul...@xen.org
> Subject: Re: [PATCH 08/37] xen/x86: add detection of discontinous node
> memory range
>
> On 23.09.2021 14:02, Wei Chen wrote:
> > One NUMA node may contain several memory blocks. In current Xen
> > code, Xen will maintain a node memory range for each node to cover
> > all its memory blocks. But here comes the problem, in the gap of
> > one node's two memory blocks, if there are some memory blocks don't
> > belong to this node (remote memory blocks). This node's memory range
> > will be expanded to cover these remote memory blocks.
> >
> > One node's memory range contains othe nodes' memory, this is obviously
> > not very reasonable. This means current NUMA code only can support
> > node has continous memory blocks. However, on a physical machine, the
> > addresses of multiple nodes can be interleaved.
> >
I will adjust above paragraph to:
... This means current NUMA code only can support node has no interlaced
memory blocks. ...
> > So in this patch, we add code to detect discontinous memory blocks
> > for one node. NUMA initializtion will be failed and error messages
> > will be printed when Xen detect such hardware configuration.
I will adjust above paragraph to:
So in this patch, we add code to detect interleave of different nodes'
memory blocks. NUMA initialization will be ...
>
> Luckily what you actually check for isn't as strict as "discontinuous":
> What you're after is no interleaving of memory. A single nod can still
> have multiple discontiguous ranges (and that'll often be the case on
> x86). Please adjust description and function name accordingly.
>
Yes, we're checking for no interlaced memory among nodes. In one
node's memory range, the memory block still can be discontinuous.
I will rename the subject to:
"add detection of interlaced memory for different nodes"
And I would rename is_node_memory_continuous to:
node_without_interleave_memory.
> > --- a/xen/arch/x86/srat.c
> > +++ b/xen/arch/x86/srat.c
> > @@ -271,6 +271,36 @@ acpi_numa_processor_affinity_init(const struct
> acpi_srat_cpu_affinity *pa)
> > pxm, pa->apic_id, node);
> > }
> >
> > +/*
> > + * Check to see if there are other nodes within this node's range.
> > + * We just need to check full contains situation. Because overlaps
> > + * have been checked before by conflicting_memblks.
> > + */
> > +static bool __init is_node_memory_continuous(nodeid_t nid,
> > + paddr_t start, paddr_t end)
>
> This indentation style demands indenting like ...
>
Ok.
> > +{
> > + nodeid_t i;
>
> ... variable declarations at function scope, i.e. in a Linux-style
> file by a tab.
>
> > +
> > + struct node *nd = &nodes[nid];
> > + for_each_node_mask(i, memory_nodes_parsed)
>
> Please move the blank line to be between declarations and statements.
>
> Also please make nd pointer-to const.
Ok.
>
> > + {
>
> In a Linux-style file opening braces do not belong on their own lines.
>
Ok.
> > + /* Skip itself */
> > + if (i == nid)
> > + continue;
> > +
> > + nd = &nodes[i];
> > + if (start < nd->start && nd->end < end)
> > + {
> > + printk(KERN_ERR
> > + "NODE %u: (%"PRIpaddr"-%"PRIpaddr") intertwine
> with NODE %u (%"PRIpaddr"-%"PRIpaddr")\n",
>
> s/intertwine/interleaves/ ?
Yes, interleaves. I will fix it.
>
> > @@ -344,6 +374,12 @@ acpi_numa_memory_affinity_init(const struct
> acpi_srat_mem_affinity *ma)
> > nd->start = start;
> > if (nd->end < end)
> > nd->end = end;
> > +
> > + /* Check whether this range contains memory for other
> nodes */
> > + if (!is_node_memory_continuous(node, nd->start,
> > nd->end))
> {
> > + bad_srat();
> > + return;
> > + }
>
> I think this check would better come before nodes[] gets updated? Otoh
> bad_srat() will zap everything anyway ...
Yes, when I wrote this check, I considered when the check was failed,
bad_srat would make numa initialization be failed. The values in nodes[]
will not take any effect. So I didn't adjust the order. But if the bad_srat
will be changed in future, and nodes[] will be used in fallback progress,
this will take more effort to debug. In this case, I agree with you,
I will update the order in next version.
Thanks,
Wei Chen
>
> Jan
