* [PATCH] x86/NUMA: correct memnode_shift calculation for single node system
@ 2022-09-27 14:15 Jan Beulich
2022-09-27 14:44 ` Alex Bennée
2022-09-27 15:53 ` Roger Pau Monné
0 siblings, 2 replies; 6+ messages in thread
From: Jan Beulich @ 2022-09-27 14:15 UTC (permalink / raw)
To: xen-devel@lists.xenproject.org
Cc: Andrew Cooper, Wei Liu, Roger Pau Monné
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@suse.com>
---
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];
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() )
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86/NUMA: correct memnode_shift calculation for single node system
2022-09-27 14:15 [PATCH] x86/NUMA: correct memnode_shift calculation for single node system Jan Beulich
@ 2022-09-27 14:44 ` Alex Bennée
2022-09-27 14:52 ` Jan Beulich
2022-09-27 15:53 ` Roger Pau Monné
1 sibling, 1 reply; 6+ messages in thread
From: Alex Bennée @ 2022-09-27 14:44 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, xen-devel
Jan Beulich <jbeulich@suse.com> 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@suse.com>
> ---
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86/NUMA: correct memnode_shift calculation for single node system
2022-09-27 14:44 ` Alex Bennée
@ 2022-09-27 14:52 ` Jan Beulich
0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2022-09-27 14:52 UTC (permalink / raw)
To: Alex Bennée; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, xen-devel
On 27.09.2022 16:44, Alex Bennée wrote:
> Jan Beulich <jbeulich@suse.com> writes:
>> @@ -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 others (especially my co-maintainers) agree, I'd be willing to
switch. Generally I've come to prefer that form as it often serves
as an indication to compilers to try to avoid branches. (That said,
I've neither checked that this has this effect here, nor would it
really matter much, as this code is run exactly once during boot.)
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86/NUMA: correct memnode_shift calculation for single node system
2022-09-27 14:15 [PATCH] x86/NUMA: correct memnode_shift calculation for single node system Jan Beulich
2022-09-27 14:44 ` Alex Bennée
@ 2022-09-27 15:53 ` Roger Pau Monné
2022-09-27 16:08 ` Jan Beulich
1 sibling, 1 reply; 6+ messages in thread
From: Roger Pau Monné @ 2022-09-27 15:53 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel@lists.xenproject.org, Andrew Cooper, Wei Liu
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@suse.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86/NUMA: correct memnode_shift calculation for single node system
2022-09-27 15:53 ` Roger Pau Monné
@ 2022-09-27 16:08 ` Jan Beulich
2022-09-28 12:19 ` [4.17?] " Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2022-09-27 16:08 UTC (permalink / raw)
To: Roger Pau Monné
Cc: xen-devel@lists.xenproject.org, Andrew Cooper, Wei Liu
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@suse.com>
>
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
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
^ permalink raw reply [flat|nested] 6+ messages in thread* [4.17?] Re: [PATCH] x86/NUMA: correct memnode_shift calculation for single node system
2022-09-27 16:08 ` Jan Beulich
@ 2022-09-28 12:19 ` Jan Beulich
0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2022-09-28 12:19 UTC (permalink / raw)
To: Roger Pau Monné, Henry Wang
Cc: xen-devel@lists.xenproject.org, Andrew Cooper, Wei Liu
On 27.09.2022 18:08, Jan Beulich wrote:
> On 27.09.2022 17:53, Roger Pau Monné wrote:
>> On Tue, Sep 27, 2022 at 04:15:19PM +0200, Jan Beulich wrote:
>>> @@ -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.
Actually I'm not so sure anymore as to 4.17 - we're in feature freeze until
the end of the week, not in code freeze. So I guess this (and the other two
related patches, provided they would get acked) ought to still be eligible.
I guess I'll give it a day for objections to surface, but otherwise commit
v2 perhaps during the afternoon tomorrow.
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-09-28 12:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-09-27 14:15 [PATCH] x86/NUMA: correct memnode_shift calculation for single node system Jan Beulich
2022-09-27 14:44 ` Alex Bennée
2022-09-27 14:52 ` Jan Beulich
2022-09-27 15:53 ` Roger Pau Monné
2022-09-27 16:08 ` Jan Beulich
2022-09-28 12:19 ` [4.17?] " Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.