Re: [PATCH v5 7/8] xen/x86: add detection of memory interleaves for different nodes

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.06.2022 06:09, Wei Chen wrote:
> v4 -> v5:
> 1. Remove "nd->end == end && nd->start == start" from
>    conflicting_memblks.
> 2. Use case NO_CONFLICT instead of "default".
> 3. Correct wrong "node" to "pxm" in print message.
> 4. Remove unnecesary "else" to remove the indent depth.
> 5. Convert all ranges to proper mathematical interval
>    representation.

As to this:

> @@ -310,44 +343,74 @@ acpi_numa_memory_affinity_init(const struct 
> acpi_srat_mem_affinity *ma)
>               bad_srat();
>               return;
>       }
> +
> +     /*
> +      * For the node that already has some memory blocks, we will
> +      * expand the node memory range temporarily to check memory
> +      * interleaves with other nodes. We will not use this node
> +      * temp memory range to check overlaps, because it will mask
> +      * the overlaps in same node.
> +      *
> +      * Node with 0 bytes memory doesn't need this expandsion.
> +      */
> +     nd_start = start;
> +     nd_end = end;
> +     nd = &nodes[node];
> +     if (nd->start != nd->end) {
> +             if (nd_start > nd->start)
> +                     nd_start = nd->start;
> +
> +             if (nd_end < nd->end)
> +                     nd_end = nd->end;
> +     }
> +
>       /* It is fine to add this area to the nodes data it will be used later*/
> -     i = conflicting_memblks(start, end);
> -     if (i < 0)
> -             /* everything fine */;
> -     else if (memblk_nodeid[i] == node) {
> -             bool mismatch = !(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) !=
> -                             !test_bit(i, memblk_hotplug);
> -
> -             printk("%sSRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") overlaps with 
> itself (%"PRIpaddr"-%"PRIpaddr")\n",
> -                    mismatch ? KERN_ERR : KERN_WARNING, pxm, start, end,
> -                    node_memblk_range[i].start, node_memblk_range[i].end);
> -             if (mismatch) {
> -                     bad_srat();
> -                     return;
> +     switch (conflicting_memblks(node, start, end, nd_start, nd_end, &i)) {
> +     case OVERLAP:
> +             if (memblk_nodeid[i] == node) {
> +                     bool mismatch = !(ma->flags &
> +                                       ACPI_SRAT_MEM_HOT_PLUGGABLE) !=
> +                                     !test_bit(i, memblk_hotplug);
> +
> +                     printk("%sSRAT: PXM %u [%"PRIpaddr"-%"PRIpaddr"] 
> overlaps with itself [%"PRIpaddr"-%"PRIpaddr"]\n",

As said when discussing v4, mathematical representation is [start,end].
Please properly use a comma instead of a dash here and below plus ...

> +                            mismatch ? KERN_ERR : KERN_WARNING, pxm, start,
> +                            end - 1, node_memblk_range[i].start,
> +                            node_memblk_range[i].end - 1);
> +                     if (mismatch) {
> +                             bad_srat();
> +                             return;
> +                     }
> +                     break;
>               }
> -     } else {
> +
> +             printk(KERN_ERR
> +                    "SRAT: PXM %u [%"PRIpaddr"-%"PRIpaddr"] overlaps with 
> PXM %u [%"PRIpaddr"-%"PRIpaddr"]\n",
> +                    pxm, start, end - 1, node_to_pxm(memblk_nodeid[i]),
> +                    node_memblk_range[i].start,
> +                    node_memblk_range[i].end - 1);
> +             bad_srat();
> +             return;
> +
> +     case INTERLEAVE:
>               printk(KERN_ERR
> -                    "SRAT: PXM %u (%"PRIpaddr"-%"PRIpaddr") overlaps with 
> PXM %u (%"PRIpaddr"-%"PRIpaddr")\n",
> -                    pxm, start, end, node_to_pxm(memblk_nodeid[i]),
> -                    node_memblk_range[i].start, node_memblk_range[i].end);
> +                    "SRAT： PXM %u: [%"PRIpaddr"-%"PRIpaddr"] interleaves 
> with PXM %u memblk [%"PRIpaddr"-%"PRIpaddr"]\n",
> +                    pxm, nd_start, nd_end - 1, node_to_pxm(memblk_nodeid[i]),
> +                    node_memblk_range[i].start, node_memblk_range[i].end - 
> 1);
>               bad_srat();
>               return;
> +
> +     case NO_CONFLICT:
> +             break;
>       }
> +
>       if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)) {
> -             struct node *nd = &nodes[node];
> -
> -             if (!node_test_and_set(node, memory_nodes_parsed)) {
> -                     nd->start = start;
> -                     nd->end = end;
> -             } else {
> -                     if (start < nd->start)
> -                             nd->start = start;
> -                     if (nd->end < end)
> -                             nd->end = end;
> -             }
> +             node_set(node, memory_nodes_parsed);
> +             nd->start = nd_start;
> +             nd->end = nd_end;
>       }
> -     printk(KERN_INFO "SRAT: Node %u PXM %u %"PRIpaddr"-%"PRIpaddr"%s\n",
> -            node, pxm, start, end,
> +
> +     printk(KERN_INFO "SRAT: Node %u PXM %u [%"PRIpaddr"-%"PRIpaddr"]%s\n",
> +            node, pxm, start, end - 1,
>              ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE ? " (hotplug)" : "");
>  
>       node_memblk_range[num_node_memblks].start = start;
> @@ -396,7 +459,7 @@ static int __init nodes_cover_memory(void)
>  
>               if (start < end) {
>                       printk(KERN_ERR "SRAT: No PXM for e820 range: "
> -                             "%"PRIpaddr" - %"PRIpaddr"\n", start, end);
> +                             "[%"PRIpaddr" - %"PRIpaddr"]\n", start, end - 
> 1);

... please be consistent with the use of blanks (personally I'd prefer
no blanks to be there, but I could see people preferring a blank after
the comma). Then
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan