RE: [PATCH 08/37] xen/x86: add detection of discontinous node memory range

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 2022年1月19日 0:13
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx; sstabellini@xxxxxxxxxx; julien@xxxxxxx
> 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