RE: [PATCH v4 7/8] 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月31日 21:21
> 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 v4 7/8] xen/x86: add detection of memory interleaves
> for different nodes
> 
> On 23.05.2022 08:25, Wei Chen wrote:
> > @@ -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;
> 
> Knowing that nd's range is non-empty, is this 2nd condition actually
> needed here? (Such an adjustment, if you decided to make it and if not
> split out to a separate patch, would need calling out in the
> description.)

The 2nd condition here, you meant is "(nd->end == end && nd->start == start)"
or just "nd->start == start" after "&&"?

My understanding is the first case, "(nd->end == end && nd->start == start)"
will be covered by "(nd->end > start && nd->start < end)". If so, I'll remove
it in the next version and add some descriptions in the commit log and code
comment.

> 
> > +           /*
> > +            * 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;
> 
> Doesn't this need to be <= in both cases (albeit I think one of the two
> expressions would want switching around, to better line up with the
> earlier one, visible in context further up).
> 

Yes, I will add "="in both cases. But for switching around, I also
wanted to make a better line up. But if nid == memblk_nodeid[i],
the check of (nd_start < nd->start && nd->end < nd_end) is meaningless.
I'll adjust their order in the next version if you think this is
acceptable.

> > @@ -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;
> 
> I don't think you need this local variable.
> 

Why I don't need this one? Did you mean I can use
switch (conflicting_memblks(...)) directly?

> > @@ -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)
> > +   {
> 
> Style: Missing blank before ( and the brace goes on the same line here
> (Linux style).
> 

Ok.

> > +   case OVERLAP:
> > +   {
> 
> Please omit braces at case labels unless you need a new scope to declare
> variables.
> 

OK.

> > +           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;
> >             }
> 
> To limit indentation depth, on of the two sides of the conditional can
> be moved out, by omitting the unnecessary "else". To reduce the diff
> it may be worthwhile to invert the if() condition, allowing the (then
> implicit) "else" case to remain (almost) unchanged from the original.
> 

I will adjust them in next version.

> > -   } 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]),
> 
> Hmm, you have PXM in the log message text, but you still pass "node" as
> first argument.
> 

I will fix it.

> Since you're touching all these messages, could I ask you to convert
> all ranges to proper mathematical interval representation? I.e.
> [start,end) here aiui as the end addresses look to be non-inclusive.
> 

Sure, I will do it.

> >                    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;
> 
> This wants to be "case NO_CONFLICT:", such that the compiler would
> warn if a new enumerator appears without adding code here. (An
> alternative - which personally I don't like - would be to put
> ASSERT_UNREACHABLE() in the default: case. The downside is that
> then the issue would only be noticeable at runtime.)
> 

Thanks, I will adjust it to:
        case NO_CONFLICT:
                break;
        default:
                ASSERT_UNREACHABLE();
in next version.


> Jan