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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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>

> ---
> On my Skylake system this changes memnodemapsize from 17 to 1 (and the
> shift from 20 to 63).
> 
> --- 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.

Thanks, Roger.