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

[Alex Bennée <alex.bennee@xxxxxxxxxx>]

Jan Beulich <jbeulich@xxxxxxxx> writes:

> 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>
> ---
> 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];

Is that boolean short cutting worth it instead of a more easily
readable:

        if (i == 0 || !nodeids || nodeids[i - 1] != nodeids[i])
           nodes_used++;

?

>          if ( epdx > memtop )
>              memtop = epdx;
>      }
> @@ -144,7 +145,7 @@ int __init compute_hash_shift(struct nod
>  {
>      int shift;
>  
> -    shift = extract_lsb_from_nodes(nodes, numnodes);
> +    shift = extract_lsb_from_nodes(nodes, numnodes, nodeids);
>      if ( memnodemapsize <= ARRAY_SIZE(_memnodemap) )
>          memnodemap = _memnodemap;
>      else if ( allocate_cachealigned_memnodemap() )


-- 
Alex Bennée