Re: [PATCH v2 3/3] powerpc/numa: Fill distance_lookup_table for offline nodes
Srikar Dronamraju writes:
> * Michael Ellerman [2021-09-23 21:17:25]:
>
>> Srikar Dronamraju writes:
>> > * Michael Ellerman [2021-08-26 23:36:53]:
>> >
>> >> Srikar Dronamraju writes:
>> >> > Scheduler expects unique number of node distances to be available at
>> >> > boot.
>> ...
>> >
>> >> > Fake the offline node's distance_lookup_table entries so that all
>> >> > possible node distances are updated.
>> >>
>> >> Does this work if we have a single node offline at boot?
>> >>
>> >
>> > It should.
>> >
>> >> Say we start with:
>> >>
>> >> node distances:
>> >> node 0 1
>> >> 0: 10 20
>> >> 1: 20 10
>> >>
>> >> And node 2 is offline at boot. We can only initialise that nodes entries
>> >> in the distance_lookup_table:
>> >>
>> >> while (i--)
>> >> distance_lookup_table[node][i] = node;
>> >>
>> >> By filling them all with 2 that causes node_distance(2, X) to return the
>> >> maximum distance for all other nodes X, because we won't break out of
>> >> the loop in __node_distance():
>> >>
>> >> for (i = 0; i < distance_ref_points_depth; i++) {
>> >> if (distance_lookup_table[a][i] == distance_lookup_table[b][i])
>> >> break;
>> >>
>> >> /* Double the distance for each NUMA level */
>> >> distance *= 2;
>> >> }
>> >>
>> >> If distance_ref_points_depth was 4 we'd return 160.
>> >
>> > As you already know, distance 10, 20, .. are defined by Powerpc, form1
>> > affinity. PAPR doesn't define actual distances, it only provides us the
>> > associativity. If there are distance_ref_points_depth is 4,
>> > (distance_ref_points_depth doesn't take local distance into consideration)
>> > 10, 20, 40, 80, 160.
>> >
>> >>
>> >> That'd leave us with 3 unique distances at boot, 10, 20, 160.
>> >>
>> >
>> > So if there are unique distances, then the distances as per the current
>> > code has to be 10, 20, 40, 80.. I dont see a way in which we have a break
>> > in
>> > the series. like having 160 without 80.
>>
>> I'm confused what you mean there.
>
> At the outset, if we have a better probable solution, do let me know, I am
> willing to try that too.
I don't have one in mind no, I'm just trying to satisfy myself that this
solution will work in all cases we're likely to encounter.
>> If we have a node that's offline at boot then we get 160 for that node,
>> that's just the result of having no info for it, so we never break out
>> of the for loop.
>>
>> So if we have two nodes, one hop apart, and then an offline node we get
>> 10, 20, 160.
>>
>> Or if you're using depth = 3 then it's 10, 20, 80.
>
> My understanding is as below:
>
> device-tree provides the max hops by way of
> ibm,associativity-reference-points. This is mapped to
> distance_ref_points_depth in Linux-powerpc.
>
> Now Linux-powerpc encodes hops as (dis-regarding local distance) 20, 40, 80,
> 160, 320 ...
> So if the distance_ref_points_depth is 3, then the hops are 20, 40, 80.
>
> Do you disagree?
I'm not sure. You didn't really address my point.
You said that we can't have 160 without 80 (for depth = 4).
I gave an example where we could see a gap in the used distance values,
ie. 10, 20, 80 for a depth of 3.
Which is not to say that distance 40 doesn't exist in that scenario,
rather that it's not used by any node.
>> >> But when node 2 comes online it might introduce more than 1 new distance
>> >> value, eg. it could be that the actual distances are:
>> >>
>> >> node distances:
>> >> node 0 1 2
>> >> 0: 10 20 40
>> >> 1: 20 10 80
>> >> 2: 40 80 10
>> >>
>> >> ie. we now have 4 distances, 10, 20, 40, 80.
>> >>
>> >> What am I missing?
>> >
>> > As I said above, I am not sure how we can have a break in the series.
>> > If distance_ref_points_depth is 3, the distances has to be 10,20,40,80 as
>> > atleast for form1 affinity.
>>
>> I agree for depth 3 we have to see 10, 20, 40, 80. But nothing
>> guarantees we see each value (other than 10).
>
> The hop distances are not from the device-tree, the device-tree only gives
> us the max hops possible. Linux-powerpc is actually hard-coding the
> distances which each hop distance being 2x the previous.
Yes. I guess I was sloppy to say "see each value", I didn't mean we see
those values directly in the device-tree.
> So we may not see any nodes at a particular hop, but we know maximum hops.
> And if distance_ref_points_depth is 3, then hops are 20, 40, 80 only.
OK, so we agree that "we may not see any nodes at a particular hop".
Which is what I was trying to say above.
>> We can have two nodes one hop apart, so we have 10 and 20, then a third
>> node is added 3 hops away, so we get 10, 20, 80.
>>
>
>> The real problem is that the third node could be 3 hops from node 0
>> and 2 hops from node 1, and so the addition of the third node causes
>> two new distance values (40 & 80) to be required.
>
> So here the max hops as given by device-tree is 3. So we know that we are
> looking for max-distance of
Re: [PATCH v2 3/3] powerpc/numa: Fill distance_lookup_table for offline nodes
* Michael Ellerman [2021-09-23 21:17:25]:
> Srikar Dronamraju writes:
> > * Michael Ellerman [2021-08-26 23:36:53]:
> >
> >> Srikar Dronamraju writes:
> >> > Scheduler expects unique number of node distances to be available at
> >> > boot.
> ...
> >
> >> > Fake the offline node's distance_lookup_table entries so that all
> >> > possible node distances are updated.
> >>
> >> Does this work if we have a single node offline at boot?
> >>
> >
> > It should.
> >
> >> Say we start with:
> >>
> >> node distances:
> >> node 0 1
> >> 0: 10 20
> >> 1: 20 10
> >>
> >> And node 2 is offline at boot. We can only initialise that nodes entries
> >> in the distance_lookup_table:
> >>
> >>while (i--)
> >>distance_lookup_table[node][i] = node;
> >>
> >> By filling them all with 2 that causes node_distance(2, X) to return the
> >> maximum distance for all other nodes X, because we won't break out of
> >> the loop in __node_distance():
> >>
> >>for (i = 0; i < distance_ref_points_depth; i++) {
> >>if (distance_lookup_table[a][i] == distance_lookup_table[b][i])
> >>break;
> >>
> >>/* Double the distance for each NUMA level */
> >>distance *= 2;
> >>}
> >>
> >> If distance_ref_points_depth was 4 we'd return 160.
> >
> > As you already know, distance 10, 20, .. are defined by Powerpc, form1
> > affinity. PAPR doesn't define actual distances, it only provides us the
> > associativity. If there are distance_ref_points_depth is 4,
> > (distance_ref_points_depth doesn't take local distance into consideration)
> > 10, 20, 40, 80, 160.
> >
> >>
> >> That'd leave us with 3 unique distances at boot, 10, 20, 160.
> >>
> >
> > So if there are unique distances, then the distances as per the current
> > code has to be 10, 20, 40, 80.. I dont see a way in which we have a break in
> > the series. like having 160 without 80.
>
> I'm confused what you mean there.
>
At the outset, if we have a better probable solution, do let me know, I am
willing to try that too.
> If we have a node that's offline at boot then we get 160 for that node,
> that's just the result of having no info for it, so we never break out
> of the for loop.
>
> So if we have two nodes, one hop apart, and then an offline node we get
> 10, 20, 160.
>
> Or if you're using depth = 3 then it's 10, 20, 80.
>
My understanding is as below:
device-tree provides the max hops by way of
ibm,associativity-reference-points. This is mapped to
distance_ref_points_depth in Linux-powerpc.
Now Linux-powerpc encodes hops as (dis-regarding local distance) 20, 40, 80,
160, 320 ...
So if the distance_ref_points_depth is 3, then the hops are 20, 40, 80.
Do you disagree?
> >> But when node 2 comes online it might introduce more than 1 new distance
> >> value, eg. it could be that the actual distances are:
> >>
> >> node distances:
> >> node 0 1 2
> >> 0: 10 20 40
> >> 1: 20 10 80
> >> 2: 40 80 10
> >>
> >> ie. we now have 4 distances, 10, 20, 40, 80.
> >>
> >> What am I missing?
> >
> > As I said above, I am not sure how we can have a break in the series.
> > If distance_ref_points_depth is 3, the distances has to be 10,20,40,80 as
> > atleast for form1 affinity.
>
> I agree for depth 3 we have to see 10, 20, 40, 80. But nothing
> guarantees we see each value (other than 10).
The hop distances are not from the device-tree, the device-tree only gives
us the max hops possible. Linux-powerpc is actually hard-coding the
distances which each hop distance being 2x the previous.
So we may not see any nodes at a particular hop, but we know maximum hops.
And if distance_ref_points_depth is 3, then hops are 20, 40, 80 only.
>
> We can have two nodes one hop apart, so we have 10 and 20, then a third
> node is added 3 hops away, so we get 10, 20, 80.
>
> The real problem is that the third node could be 3 hops from node 0
> and 2 hops from node 1, and so the addition of the third node causes
> two new distance values (40 & 80) to be required.
So here the max hops as given by device-tree is 3. So we know that we are
looking for max-distance of 80 by way of distance_ref_points_depth.
Even if the 3rd node was at 4 hops, we would already know the max distance
of 160, by way of distance_ref_points_depth. However in the most unlikely
scenarios where the number of possible nodes are less than the
distance_ref_points_depth(aka max hops) + there are CPUless/memoryless nodes
we may not have initialized to the right distances.
>
> I think maybe what you're saying is that in practice we don't see setups
> like that. But I don't know if I'm happy with a solution that doesn't
> work in the general case, and relies on the particular properties of our
> current set of systems.
>
But our current set of systems are having a problem (Systems can likely
crash on adding a CPU to a node.) The only other way I can think of is the
previous approach were we ask scheduler hook which tells ho
Re: [PATCH v2 3/3] powerpc/numa: Fill distance_lookup_table for offline nodes
Srikar Dronamraju writes:
> * Michael Ellerman [2021-08-26 23:36:53]:
>
>> Srikar Dronamraju writes:
>> > Scheduler expects unique number of node distances to be available at
>> > boot.
...
>
>> > Fake the offline node's distance_lookup_table entries so that all
>> > possible node distances are updated.
>>
>> Does this work if we have a single node offline at boot?
>>
>
> It should.
>
>> Say we start with:
>>
>> node distances:
>> node 0 1
>> 0: 10 20
>> 1: 20 10
>>
>> And node 2 is offline at boot. We can only initialise that nodes entries
>> in the distance_lookup_table:
>>
>> while (i--)
>> distance_lookup_table[node][i] = node;
>>
>> By filling them all with 2 that causes node_distance(2, X) to return the
>> maximum distance for all other nodes X, because we won't break out of
>> the loop in __node_distance():
>>
>> for (i = 0; i < distance_ref_points_depth; i++) {
>> if (distance_lookup_table[a][i] == distance_lookup_table[b][i])
>> break;
>>
>> /* Double the distance for each NUMA level */
>> distance *= 2;
>> }
>>
>> If distance_ref_points_depth was 4 we'd return 160.
>
> As you already know, distance 10, 20, .. are defined by Powerpc, form1
> affinity. PAPR doesn't define actual distances, it only provides us the
> associativity. If there are distance_ref_points_depth is 4,
> (distance_ref_points_depth doesn't take local distance into consideration)
> 10, 20, 40, 80, 160.
>
>>
>> That'd leave us with 3 unique distances at boot, 10, 20, 160.
>>
>
> So if there are unique distances, then the distances as per the current
> code has to be 10, 20, 40, 80.. I dont see a way in which we have a break in
> the series. like having 160 without 80.
I'm confused what you mean there.
If we have a node that's offline at boot then we get 160 for that node,
that's just the result of having no info for it, so we never break out
of the for loop.
So if we have two nodes, one hop apart, and then an offline node we get
10, 20, 160.
Or if you're using depth = 3 then it's 10, 20, 80.
>> But when node 2 comes online it might introduce more than 1 new distance
>> value, eg. it could be that the actual distances are:
>>
>> node distances:
>> node 0 1 2
>> 0: 10 20 40
>> 1: 20 10 80
>> 2: 40 80 10
>>
>> ie. we now have 4 distances, 10, 20, 40, 80.
>>
>> What am I missing?
>
> As I said above, I am not sure how we can have a break in the series.
> If distance_ref_points_depth is 3, the distances has to be 10,20,40,80 as
> atleast for form1 affinity.
I agree for depth 3 we have to see 10, 20, 40, 80. But nothing
guarantees we see each value (other than 10).
We can have two nodes one hop apart, so we have 10 and 20, then a third
node is added 3 hops away, so we get 10, 20, 80.
The real problem is that the third node could be 3 hops from node 0
and 2 hops from node 1, and so the addition of the third node causes
two new distance values (40 & 80) to be required.
I think maybe what you're saying is that in practice we don't see setups
like that. But I don't know if I'm happy with a solution that doesn't
work in the general case, and relies on the particular properties of our
current set of systems.
Possibly we just need to detect that case and WARN about it. The only
problem is we won't know until the system is already up and running, ie.
we can't know at boot that the onlining of the third node will cause 2
new distance values to be added.
cheers
Re: [PATCH v2 3/3] powerpc/numa: Fill distance_lookup_table for offline nodes
* Michael Ellerman [2021-08-26 23:36:53]:
> Srikar Dronamraju writes:
> > Scheduler expects unique number of node distances to be available at
> > boot.
>
> I think it needs "the number of unique node distances" ?
>
> > It uses node distance to calculate this unique node distances.
>
> It iterates over all pairs of nodes and records node_distance() for that
> pair, and then calculates the set of unique distances.
>
> > On POWER, node distances for offline nodes is not available. However,
> > POWER already knows unique possible node distances.
>
> I think it would be more accurate to say PAPR rather than POWER there.
> It's PAPR that defines the way we determine distances and imposes that
> limitation.
>
Okay, will do all the necessary modifications as suggested above.
> > Fake the offline node's distance_lookup_table entries so that all
> > possible node distances are updated.
>
> Does this work if we have a single node offline at boot?
>
It should.
> Say we start with:
>
> node distances:
> node 0 1
> 0: 10 20
> 1: 20 10
>
> And node 2 is offline at boot. We can only initialise that nodes entries
> in the distance_lookup_table:
>
> while (i--)
> distance_lookup_table[node][i] = node;
>
> By filling them all with 2 that causes node_distance(2, X) to return the
> maximum distance for all other nodes X, because we won't break out of
> the loop in __node_distance():
>
> for (i = 0; i < distance_ref_points_depth; i++) {
> if (distance_lookup_table[a][i] == distance_lookup_table[b][i])
> break;
>
> /* Double the distance for each NUMA level */
> distance *= 2;
> }
>
> If distance_ref_points_depth was 4 we'd return 160.
As you already know, distance 10, 20, .. are defined by Powerpc, form1
affinity. PAPR doesn't define actual distances, it only provides us the
associativity. If there are distance_ref_points_depth is 4,
(distance_ref_points_depth doesn't take local distance into consideration)
10, 20, 40, 80, 160.
>
> That'd leave us with 3 unique distances at boot, 10, 20, 160.
>
So if there are unique distances, then the distances as per the current
code has to be 10, 20, 40, 80.. I dont see a way in which we have a break in
the series. like having 160 without 80.
> But when node 2 comes online it might introduce more than 1 new distance
> value, eg. it could be that the actual distances are:
>
> node distances:
> node 0 1 2
> 0: 10 20 40
> 1: 20 10 80
> 2: 40 80 10
>
> ie. we now have 4 distances, 10, 20, 40, 80.
>
> What am I missing?
>
As I said above, I am not sure how we can have a break in the series.
If distance_ref_points_depth is 3, the distances has to be 10,20,40,80 as
atleast for form1 affinity.
> > However this only needs to be done if the number of unique node
> > distances that can be computed for online nodes is less than the
> > number of possible unique node distances as represented by
> > distance_ref_points_depth.
>
> Looking at a few machines they all have distance_ref_points_depth = 2.
>
> So maybe that explains it, in practice we only see 10, 20, 40.
>
> > When the node is actually onlined, distance_lookup_table will be
> > updated with actual entries.
>
> > Cc: linuxppc-dev@lists.ozlabs.org
> > Cc: Nathan Lynch
> > Cc: Michael Ellerman
> > Cc: Ingo Molnar
> > Cc: Peter Zijlstra
> > Cc: Valentin Schneider
> > Cc: Gautham R Shenoy
> > Cc: Vincent Guittot
> > Cc: Geetika Moolchandani
> > Cc: Laurent Dufour
> > Cc: kernel test robot
> > Signed-off-by: Srikar Dronamraju
> > ---
> > arch/powerpc/mm/numa.c | 70 ++
> > 1 file changed, 70 insertions(+)
> >
> > Changelog:
> > v1:
> > https://lore.kernel.org/linuxppc-dev/20210701041552.112072-3-sri...@linux.vnet.ibm.com/t/#u
> > [ Fixed a missing prototype warning Reported-by: kernel test robot
> > ]
> >
> > diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> > index 3c124928a16d..0ee79a08c9e1 100644
> > --- a/arch/powerpc/mm/numa.c
> > +++ b/arch/powerpc/mm/numa.c
> > @@ -856,6 +856,75 @@ void __init dump_numa_cpu_topology(void)
> > }
> > }
> >
> > +/*
> > + * Scheduler expects unique number of node distances to be available at
> > + * boot. It uses node distance to calculate this unique node distances. On
> > + * POWER, node distances for offline nodes is not available. However, POWER
> > + * already knows unique possible node distances. Fake the offline node's
> > + * distance_lookup_table entries so that all possible node distances are
> > + * updated.
> > + */
>
> > +static void __init fake_update_distance_lookup_table(void)
> > +{
> > + unsigned long distance_map;
> > + int i, nr_levels, nr_depth, node;
>
> Are they distances, depths, or levels? :)
>
> Bit more consistency in the variable names might make the code easier to
> follow.
>
> > +
> > + if (!numa_enabled)
> > +
Re: [PATCH v2 3/3] powerpc/numa: Fill distance_lookup_table for offline nodes
Srikar Dronamraju writes:
> Scheduler expects unique number of node distances to be available at
> boot.
I think it needs "the number of unique node distances" ?
> It uses node distance to calculate this unique node distances.
It iterates over all pairs of nodes and records node_distance() for that
pair, and then calculates the set of unique distances.
> On POWER, node distances for offline nodes is not available. However,
> POWER already knows unique possible node distances.
I think it would be more accurate to say PAPR rather than POWER there.
It's PAPR that defines the way we determine distances and imposes that
limitation.
> Fake the offline node's distance_lookup_table entries so that all
> possible node distances are updated.
Does this work if we have a single node offline at boot?
Say we start with:
node distances:
node 0 1
0: 10 20
1: 20 10
And node 2 is offline at boot. We can only initialise that nodes entries
in the distance_lookup_table:
while (i--)
distance_lookup_table[node][i] = node;
By filling them all with 2 that causes node_distance(2, X) to return the
maximum distance for all other nodes X, because we won't break out of
the loop in __node_distance():
for (i = 0; i < distance_ref_points_depth; i++) {
if (distance_lookup_table[a][i] == distance_lookup_table[b][i])
break;
/* Double the distance for each NUMA level */
distance *= 2;
}
If distance_ref_points_depth was 4 we'd return 160.
That'd leave us with 3 unique distances at boot, 10, 20, 160.
But when node 2 comes online it might introduce more than 1 new distance
value, eg. it could be that the actual distances are:
node distances:
node 0 1 2
0: 10 20 40
1: 20 10 80
2: 40 80 10
ie. we now have 4 distances, 10, 20, 40, 80.
What am I missing?
> However this only needs to be done if the number of unique node
> distances that can be computed for online nodes is less than the
> number of possible unique node distances as represented by
> distance_ref_points_depth.
Looking at a few machines they all have distance_ref_points_depth = 2.
So maybe that explains it, in practice we only see 10, 20, 40.
> When the node is actually onlined, distance_lookup_table will be
> updated with actual entries.
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: Nathan Lynch
> Cc: Michael Ellerman
> Cc: Ingo Molnar
> Cc: Peter Zijlstra
> Cc: Valentin Schneider
> Cc: Gautham R Shenoy
> Cc: Vincent Guittot
> Cc: Geetika Moolchandani
> Cc: Laurent Dufour
> Cc: kernel test robot
> Signed-off-by: Srikar Dronamraju
> ---
> arch/powerpc/mm/numa.c | 70 ++
> 1 file changed, 70 insertions(+)
>
> Changelog:
> v1:
> https://lore.kernel.org/linuxppc-dev/20210701041552.112072-3-sri...@linux.vnet.ibm.com/t/#u
> [ Fixed a missing prototype warning Reported-by: kernel test robot
> ]
>
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index 3c124928a16d..0ee79a08c9e1 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -856,6 +856,75 @@ void __init dump_numa_cpu_topology(void)
> }
> }
>
> +/*
> + * Scheduler expects unique number of node distances to be available at
> + * boot. It uses node distance to calculate this unique node distances. On
> + * POWER, node distances for offline nodes is not available. However, POWER
> + * already knows unique possible node distances. Fake the offline node's
> + * distance_lookup_table entries so that all possible node distances are
> + * updated.
> + */
> +static void __init fake_update_distance_lookup_table(void)
> +{
> + unsigned long distance_map;
> + int i, nr_levels, nr_depth, node;
Are they distances, depths, or levels? :)
Bit more consistency in the variable names might make the code easier to
follow.
> +
> + if (!numa_enabled)
> + return;
> +
> + if (!form1_affinity)
> + return;
That doesn't exist since Aneesh's FORM2 series, so that will need a
rebase, and possibly some more rework to interact with that series.
> + /*
> + * distance_ref_points_depth lists the unique numa domains
> + * available. However it ignore LOCAL_DISTANCE. So add +1
> + * to get the actual number of unique distances.
> + */
> + nr_depth = distance_ref_points_depth + 1;
num_depths would be a better name IMHO.
> +
> + WARN_ON(nr_depth > sizeof(distance_map));
Warn but then continue, and corrupt something on the stack? Seems like a
bad idea :)
I guess it's too early to use bitmap_alloc(). But can we at least return
if nr_depth is too big.
> +
> + bitmap_zero(&distance_map, nr_depth);
> + bitmap_set(&distance_map, 0, 1);
> +
> + for_each_online_node(node) {
> + int nd, distance = LOCAL_DISTANCE;
> +
> + if (node == first_online_node)
> + continue;
> +
>
