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年6月1日 14:32
> 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 01.06.2022 04:53, Wei Chen wrote:
> >> From: Jan Beulich <jbeulich@xxxxxxxx>
> >> Sent: 2022年5月31日 21:21
> >>
> >> 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 "&&"?
> 
> No, I mean the entire 2nd if().
> 

OK.

> >>> +         /*
> >>> +          * 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.
> 
> I wasn't referring to the "nid != memblk_nodeid[i]" part at all. What
> I'm after is for the two range checks to come as close as possible to
> what the other range check does. (Which, as I notice only now, would
> include the dropping of the unnecessary inner pair of parentheses.)
> E.g. (there are other variations of it)
> 
>               if (nid != memblk_nodeid[i] &&
>                   nd->start >= nd_start && nd->end <= nd_end)
>                       return INTERLEAVE;
> 

Oh, thanks. I had thought too much, I will drop them.

> >>> @@ -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?
> 
> Yes. Why could this not be possible?
> 

Ok.

> >>>                  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.
> 
> As said - I consider this form less desirable, as it'll defer
> noticing of an issue from build-time to runtime. If you think that
> form is better, may I ask why?
> 

Ok. I will drop the default. I had mis-understood your comment.

> Jan