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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.
> 
> 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.

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.

> --- 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 ...

> +{
> +     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.

> +     {

In a Linux-style file opening braces do not belong on their own lines.

> +             /* 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/ ?

> @@ -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 ...

Jan