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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Jan,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 2022年5月18日 21:31
> To: Wei Chen <Wei.Chen@xxxxxxx>
> Cc: nd <nd@xxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Roger Pau
> Monné <roger.pau@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; Jiamei Xie
> <Jiamei.Xie@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3 8/9] xen/x86: add detection of memory interleaves
> for different nodes
> 
> 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".
> 

Ack.

> > +   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_".
> 

Oh, yes. I all drop it for all above enumerations.

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

The caller "acpi_numa_memory_affinity_init" defines int for node memory
block id, and as a return value at the same time. So I haven't changed it.
As we don't use this "int" for return value any more, I agree, it will
never be negative, I would fix it.

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

Ok.

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

Oh! thanks for pointing out this one! Yes, right side should be nd->end.

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

Did you mean, we need to add if ... else ... in this block? If yes,
is it ok to update this block like:
        if (nd->start != nd->end) {
                nd_start = min(nd_start, nd->start);
                nd_end = max(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);
> > +   if (status == ERR_OVERLAP) {
> 
> Please use switch(status) when checking enumerated values.
> 

Ok, I will do it.

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

Yes, I will do it.

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

Ok, will use PXM here. But I have question for upcoming changes, if we
move this part of code to common. As device tree NUMA doesn't have
PXM concept (even I can use a fake node_to_pxm to do 1:1 mapping), so
can we still use PXM here?

> Jan