Re: [PATCH] x86/NUMA: correct memnode_shift calculation for single node system

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.09.2022 17:53, Roger Pau Monné wrote:
> On Tue, Sep 27, 2022 at 04:15:19PM +0200, Jan Beulich wrote:
>> SRAT may describe even a single node system (including such with
>> multiple nodes, but only one having any memory) using multiple ranges.
>> Hence simply counting the number of ranges (note that function
>> parameters are mis-named) is not an indication of the number of nodes in
>> use. Since we only care about knowing whether we're on a single node
>> system, accounting for this is easy: Increment the local variable only
>> when adjacent ranges are for different nodes. That way the count may
>> still end up larger than the number of nodes in use, but it won't be
>> larger than 1 when only a single node has any memory.
>>
>> To compensate populate_memnodemap() now needs to be prepared to find
>> the correct node ID already in place for a range. (This could of course
>> also happen when there's more than one node with memory, while at least
>> one node has multiple adjacent ranges, provided extract_lsb_from_nodes()
>> would also know to recognize this case.)
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

>> --- a/xen/arch/x86/numa.c
>> +++ b/xen/arch/x86/numa.c
>> @@ -78,7 +78,8 @@ static int __init populate_memnodemap(co
>>          if ( (epdx >> shift) >= memnodemapsize )
>>              return 0;
>>          do {
>> -            if ( memnodemap[spdx >> shift] != NUMA_NO_NODE )
>> +            if ( memnodemap[spdx >> shift] != NUMA_NO_NODE &&
>> +                 (!nodeids || memnodemap[spdx >> shift] != nodeids[i]) )
>>                  return -1;
>>  
>>              if ( !nodeids )
>> @@ -114,7 +115,7 @@ static int __init allocate_cachealigned_
>>   * maximum possible shift.
>>   */
>>  static int __init extract_lsb_from_nodes(const struct node *nodes,
>> -                                         int numnodes)
>> +                                         int numnodes, const nodeid_t 
>> *nodeids)
>>  {
>>      int i, nodes_used = 0;
>>      unsigned long spdx, epdx;
>> @@ -127,7 +128,7 @@ static int __init extract_lsb_from_nodes
>>          if ( spdx >= epdx )
>>              continue;
>>          bitfield |= spdx;
>> -        nodes_used++;
>> +        nodes_used += i == 0 || !nodeids || nodeids[i - 1] != nodeids[i];
> 
> I think I would also prefer the `if ( ... ) nodes_used++;` form, as
> it's clearer.

Okay, will switch then. This isn't for 4.17 anyway (I think), so
there's no rush.

Jan