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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.05.2022 03:46, Wei Chen wrote:
> --- a/xen/arch/x86/srat.c
> +++ b/xen/arch/x86/srat.c
> @@ -42,6 +42,12 @@ static struct node node_memblk_range[NR_NODE_MEMBLKS];
>  static nodeid_t memblk_nodeid[NR_NODE_MEMBLKS];
>  static __initdata DECLARE_BITMAP(memblk_hotplug, NR_NODE_MEMBLKS);
>  
> +enum conflicts {
> +     NO_CONFLICT = 0,

No need for the "= 0".

> +     ERR_OVERLAP,

While this at least can be an error (the self-overlap case is merely
warned about), ...

> +     ERR_INTERLEAVE,

... I don't think this is, and hence I'd recommend to drop "ERR_".

> @@ -119,20 +125,43 @@ int valid_numa_range(paddr_t start, paddr_t end, 
> nodeid_t node)
>       return 0;
>  }
>  
> -static __init int conflicting_memblks(paddr_t start, paddr_t end)
> +static enum conflicts __init
> +conflicting_memblks(nodeid_t nid, paddr_t start, paddr_t end,
> +                 paddr_t nd_start, paddr_t nd_end, int *mblkid)

Why "int"? Can the value passed back be negative?

>  {
>       int i;
>  
> +     /*
> +      * Scan all recorded nodes' memory blocks to check conflicts:
> +      * Overlap or interleave.
> +      */
>       for (i = 0; i < num_node_memblks; i++) {
>               struct node *nd = &node_memblk_range[i];
> +             *mblkid = i;

Style: Please maintain a blank line between declaration(s) and
statement(s).

> @@ -310,42 +342,67 @@ 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 < end)

Did you mean nd->end here on the right side of < ? By intentionally
not adding "default:" in the body, you then also allow the compiler
to point out that addition of a new enumerator also needs handling
here.

> +                     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) {
> +     status = conflicting_memblks(node, start, end, nd_start, nd_end, &i);
> +     if (status == ERR_OVERLAP) {

Please use switch(status) when checking enumerated values.

> +             if (memblk_nodeid[i] == node) {
> +                     bool mismatch = !(ma->flags &
> +                                     ACPI_SRAT_MEM_HOT_PLUGGABLE) !=
> +                                     !test_bit(i, memblk_hotplug);

Style: The middle line wants indenting by two more characters.

> +
> +                     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;
> +                     }
> +             } else {
> +                     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);
>                       bad_srat();
>                       return;
>               }
> -     } else {
> +     } else if (status == ERR_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]),
> +                    "SRAT： Node %u: (%"PRIpaddr"-%"PRIpaddr") interleaves 
> with node %u memblk (%"PRIpaddr"-%"PRIpaddr")\n",
> +                    node, nd_start, nd_end, memblk_nodeid[i],

Please log pxm (not node) here just like is done in the overlap case.
The remote node ID will then require converting to PXM, of course.

Jan