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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi,

It seems that this patch is the only one that is not properly acked and
reviewed in this series. So this is a gentle ping asking if everyone is happy
with this patch or are there any more actions that should be taken from the
author. Thanks!

Kind regards,
Henry

> -----Original Message-----
> Subject: [PATCH v4 7/8] xen/x86: add detection of memory interleaves for
> different nodes
> 
> 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 other nodes' memory, this is
> obviously not very reasonable. This means current NUMA code only
> can support node has no interleaved memory blocks. However, on a
> physical machine, the addresses of multiple nodes can be interleaved.
> 
> So in this patch, we add code to detect memory interleaves of
> different nodes. NUMA initialization will be failed and error
> messages will be printed when Xen detect such hardware configuration.
> 
> Change-Id: Ia7ff9a9128ecbe3eb4dddd1307ae8fbe65575ccf
> Issue-Id: SCM-2240
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> Tested-by: Jiamei Xie <jiamei.xie@xxxxxxx>
> ---
> v3 -> v4:
> 1. Drop "ERR" prefix for enumeration, and remove init value.
> 2. Use "switch case" for enumeration, and add "default:"
> 3. Use "PXM" in log messages.
> 4. Use unsigned int for node memory block id.
> 5. Fix some code-style comments.
> 6. Use "nd->end" in node range expansion check.
> v2 -> v3:
> 1. Merge the check code from a separate function to
>    conflicting_memblks. This will reduce the loop
>    times of node memory blocks.
> 2. Use an enumeration to indicate conflict check status.
> 3. Use a pointer to get conflict memory block id.
> v1 -> v2:
> 1. Update the description to say we're after is no memory
>    interleaves of different nodes.
> 2. Only update node range when it passes the interleave check.
> 3. Don't use full upper-case for "node".
> ---
>  xen/arch/x86/srat.c | 132 +++++++++++++++++++++++++++++++++-----------
>  1 file changed, 101 insertions(+), 31 deletions(-)
> 
> diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
> index 8ffe43bdfe..a831df7648 100644
> --- 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,
> +     OVERLAP,
> +     INTERLEAVE,
> +};
> +
>  static inline bool node_found(unsigned idx, unsigned pxm)
>  {
>       return ((pxm2node[idx].pxm == pxm) &&
> @@ -119,20 +125,45 @@ 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, unsigned int
> *mblkid)
>  {
> -     int i;
> +     unsigned 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;
> +
> +             /* Skip 0 bytes node memory block. */
>               if (nd->start == nd->end)
>                       continue;
> +             /*
> +              * Use memblk range to check memblk overlaps, include the
> +              * self-overlap case.
> +              */
>               if (nd->end > start && nd->start < end)
> -                     return i;
> +                     return OVERLAP;
>               if (nd->end == end && nd->start == start)
> -                     return i;
> +                     return OVERLAP;
> +             /*
> +              * Use node memory range to check whether new range
> contains
> +              * memory from other nodes - interleave check. We just
> need
> +              * to check full contains situation. Because overlaps have
> +              * been checked above.
> +              */
> +             if (nid != memblk_nodeid[i] &&
> +                 (nd_start < nd->start && nd->end < nd_end))
> +                     return INTERLEAVE;
>       }
> -     return -1;
> +
> +     return NO_CONFLICT;
>  }
> 
>  static __init void cutoff_node(int i, paddr_t start, paddr_t end)
> @@ -275,10 +306,13 @@ acpi_numa_processor_affinity_init(const struct
> acpi_srat_cpu_affinity *pa)
>  void __init
>  acpi_numa_memory_affinity_init(const struct acpi_srat_mem_affinity *ma)
>  {
> +     enum conflicts status;
> +     struct node *nd;
> +     paddr_t nd_start, nd_end;
>       paddr_t start, end;
>       unsigned pxm;
>       nodeid_t node;
> -     int i;
> +     unsigned int i;
> 
>       if (srat_disabled())
>               return;
> @@ -310,42 +344,78 @@ 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) {
> +     status = conflicting_memblks(node, start, end, nd_start, nd_end,
> &i);
> +     switch(status)
> +     {
> +     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",
> +                            mismatch ? KERN_ERR : KERN_WARNING, pxm,
> start,
> +                            end, node_memblk_range[i].start,
> +                            node_memblk_range[i].end);
> +                     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,
> node_to_pxm(memblk_nodeid[i]),
> +                            node_memblk_range[i].start,
> +                            node_memblk_range[i].end);
>                       bad_srat();
>                       return;
>               }
> -     } else {
> +     }
> +
> +     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]),
> +                    "SRAT： PXM %u: (%"PRIpaddr"-%"PRIpaddr")
> interleaves with PXM %u memblk (%"PRIpaddr"-%"PRIpaddr")\n",
> +                    node, nd_start, nd_end,
> node_to_pxm(memblk_nodeid[i]),
>                      node_memblk_range[i].start,
> node_memblk_range[i].end);
>               bad_srat();
>               return;
>       }
> -     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;
> -             }
> +     default:
> +             break;
> +     }
> +
> +     if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)) {
> +             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,
>              ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE ? " (hotplug)" :
> "");
> --
> 2.25.1
>