* [PATCH] powerpc/pseries: Fix numa FORM2 parsing fallback code
@ 2021-11-05 13:29 Nicholas Piggin
2021-11-05 16:05 ` Aneesh Kumar K.V
2021-11-08 5:20 ` Michael Ellerman
0 siblings, 2 replies; 5+ messages in thread
From: Nicholas Piggin @ 2021-11-05 13:29 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Aneesh Kumar K . V, Nicholas Piggin
In case the FORM2 distance table from firmware is not the expected size,
there is fallback code that just populates the lookup table as local vs
remote.
However it then continues on to use the distance table. Fix.
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Fixes: 1c6b5a7e7405 ("powerpc/pseries: Add support for FORM2 associativity")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/mm/numa.c | 29 +++++++++++++----------------
1 file changed, 13 insertions(+), 16 deletions(-)
diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index 6f14c8fb6359..0789cde7f658 100644
--- a/arch/powerpc/mm/numa.c
+++ b/arch/powerpc/mm/numa.c
@@ -380,6 +380,7 @@ static void initialize_form2_numa_distance_lookup_table(void)
const __be32 *numa_lookup_index;
int numa_dist_table_length;
int max_numa_index, distance_index;
+ bool good = true;
if (firmware_has_feature(FW_FEATURE_OPAL))
root = of_find_node_by_path("/ibm,opal");
@@ -407,30 +408,26 @@ static void initialize_form2_numa_distance_lookup_table(void)
if (numa_dist_table_length != max_numa_index * max_numa_index) {
WARN(1, "Wrong NUMA distance information\n");
- /* consider everybody else just remote. */
- for (i = 0; i < max_numa_index; i++) {
- for (j = 0; j < max_numa_index; j++) {
- int nodeA = numa_id_index_table[i];
- int nodeB = numa_id_index_table[j];
-
- if (nodeA == nodeB)
- numa_distance_table[nodeA][nodeB] = LOCAL_DISTANCE;
- else
- numa_distance_table[nodeA][nodeB] = REMOTE_DISTANCE;
- }
- }
+ good = false;
}
-
distance_index = 0;
for (i = 0; i < max_numa_index; i++) {
for (j = 0; j < max_numa_index; j++) {
int nodeA = numa_id_index_table[i];
int nodeB = numa_id_index_table[j];
-
- numa_distance_table[nodeA][nodeB] = numa_dist_table[distance_index++];
- pr_debug("dist[%d][%d]=%d ", nodeA, nodeB, numa_distance_table[nodeA][nodeB]);
+ int dist;
+
+ if (good)
+ dist = numa_dist_table[distance_index++];
+ else if (nodeA == nodeB)
+ dist = LOCAL_DISTANCE;
+ else
+ dist = REMOTE_DISTANCE;
+ numa_distance_table[nodeA][nodeB] = dist;
+ pr_debug("dist[%d][%d]=%d ", nodeA, nodeB, dist);
}
}
+
of_node_put(root);
}
--
2.23.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc/pseries: Fix numa FORM2 parsing fallback code
2021-11-05 13:29 [PATCH] powerpc/pseries: Fix numa FORM2 parsing fallback code Nicholas Piggin
@ 2021-11-05 16:05 ` Aneesh Kumar K.V
2021-11-08 5:20 ` Michael Ellerman
1 sibling, 0 replies; 5+ messages in thread
From: Aneesh Kumar K.V @ 2021-11-05 16:05 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Nicholas Piggin
Nicholas Piggin <npiggin@gmail.com> writes:
> In case the FORM2 distance table from firmware is not the expected size,
> there is fallback code that just populates the lookup table as local vs
> remote.
>
> However it then continues on to use the distance table. Fix.
>
Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Fixes: 1c6b5a7e7405 ("powerpc/pseries: Add support for FORM2 associativity")
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/mm/numa.c | 29 +++++++++++++----------------
> 1 file changed, 13 insertions(+), 16 deletions(-)
>
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index 6f14c8fb6359..0789cde7f658 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -380,6 +380,7 @@ static void initialize_form2_numa_distance_lookup_table(void)
> const __be32 *numa_lookup_index;
> int numa_dist_table_length;
> int max_numa_index, distance_index;
> + bool good = true;
>
> if (firmware_has_feature(FW_FEATURE_OPAL))
> root = of_find_node_by_path("/ibm,opal");
> @@ -407,30 +408,26 @@ static void initialize_form2_numa_distance_lookup_table(void)
>
> if (numa_dist_table_length != max_numa_index * max_numa_index) {
> WARN(1, "Wrong NUMA distance information\n");
> - /* consider everybody else just remote. */
> - for (i = 0; i < max_numa_index; i++) {
> - for (j = 0; j < max_numa_index; j++) {
> - int nodeA = numa_id_index_table[i];
> - int nodeB = numa_id_index_table[j];
> -
> - if (nodeA == nodeB)
> - numa_distance_table[nodeA][nodeB] = LOCAL_DISTANCE;
> - else
> - numa_distance_table[nodeA][nodeB] = REMOTE_DISTANCE;
> - }
> - }
> + good = false;
> }
> -
> distance_index = 0;
> for (i = 0; i < max_numa_index; i++) {
> for (j = 0; j < max_numa_index; j++) {
> int nodeA = numa_id_index_table[i];
> int nodeB = numa_id_index_table[j];
> -
> - numa_distance_table[nodeA][nodeB] = numa_dist_table[distance_index++];
> - pr_debug("dist[%d][%d]=%d ", nodeA, nodeB, numa_distance_table[nodeA][nodeB]);
> + int dist;
> +
> + if (good)
> + dist = numa_dist_table[distance_index++];
> + else if (nodeA == nodeB)
> + dist = LOCAL_DISTANCE;
> + else
> + dist = REMOTE_DISTANCE;
> + numa_distance_table[nodeA][nodeB] = dist;
> + pr_debug("dist[%d][%d]=%d ", nodeA, nodeB, dist);
> }
> }
> +
> of_node_put(root);
> }
>
> --
> 2.23.0
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc/pseries: Fix numa FORM2 parsing fallback code
2021-11-05 13:29 [PATCH] powerpc/pseries: Fix numa FORM2 parsing fallback code Nicholas Piggin
2021-11-05 16:05 ` Aneesh Kumar K.V
@ 2021-11-08 5:20 ` Michael Ellerman
2021-11-08 13:53 ` Nicholas Piggin
1 sibling, 1 reply; 5+ messages in thread
From: Michael Ellerman @ 2021-11-08 5:20 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Aneesh Kumar K . V, Nicholas Piggin
Nicholas Piggin <npiggin@gmail.com> writes:
> In case the FORM2 distance table from firmware is not the expected size,
> there is fallback code that just populates the lookup table as local vs
> remote.
>
> However it then continues on to use the distance table. Fix.
>
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Fixes: 1c6b5a7e7405 ("powerpc/pseries: Add support for FORM2 associativity")
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/mm/numa.c | 29 +++++++++++++----------------
> 1 file changed, 13 insertions(+), 16 deletions(-)
>
> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> index 6f14c8fb6359..0789cde7f658 100644
> --- a/arch/powerpc/mm/numa.c
> +++ b/arch/powerpc/mm/numa.c
> @@ -380,6 +380,7 @@ static void initialize_form2_numa_distance_lookup_table(void)
> const __be32 *numa_lookup_index;
> int numa_dist_table_length;
> int max_numa_index, distance_index;
> + bool good = true;
numa_dist_table is a pointer, so couldn't we just set it to NULL if the
info it's pointing at is invalid?
>
> if (firmware_has_feature(FW_FEATURE_OPAL))
> root = of_find_node_by_path("/ibm,opal");
> @@ -407,30 +408,26 @@ static void initialize_form2_numa_distance_lookup_table(void)
>
> if (numa_dist_table_length != max_numa_index * max_numa_index) {
> WARN(1, "Wrong NUMA distance information\n");
> - /* consider everybody else just remote. */
> - for (i = 0; i < max_numa_index; i++) {
> - for (j = 0; j < max_numa_index; j++) {
> - int nodeA = numa_id_index_table[i];
> - int nodeB = numa_id_index_table[j];
> -
> - if (nodeA == nodeB)
> - numa_distance_table[nodeA][nodeB] = LOCAL_DISTANCE;
> - else
> - numa_distance_table[nodeA][nodeB] = REMOTE_DISTANCE;
> - }
> - }
> + good = false;
ie. numa_dist_table = NULL;
> }
> -
> distance_index = 0;
> for (i = 0; i < max_numa_index; i++) {
> for (j = 0; j < max_numa_index; j++) {
> int nodeA = numa_id_index_table[i];
> int nodeB = numa_id_index_table[j];
> -
> - numa_distance_table[nodeA][nodeB] = numa_dist_table[distance_index++];
> - pr_debug("dist[%d][%d]=%d ", nodeA, nodeB, numa_distance_table[nodeA][nodeB]);
> + int dist;
> +
> + if (good)
if (numa_dist_table)
> + dist = numa_dist_table[distance_index++];
> + else if (nodeA == nodeB)
> + dist = LOCAL_DISTANCE;
> + else
> + dist = REMOTE_DISTANCE;
> + numa_distance_table[nodeA][nodeB] = dist;
> + pr_debug("dist[%d][%d]=%d ", nodeA, nodeB, dist);
> }
> }
> +
> of_node_put(root);
> }
But maybe before we do that we can rename it, because it is really easy
to confuse numa_dist_table and numa_distance_table if you don't look
closely.
cheers
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc/pseries: Fix numa FORM2 parsing fallback code
2021-11-08 5:20 ` Michael Ellerman
@ 2021-11-08 13:53 ` Nicholas Piggin
2021-11-09 1:10 ` Michael Ellerman
0 siblings, 1 reply; 5+ messages in thread
From: Nicholas Piggin @ 2021-11-08 13:53 UTC (permalink / raw)
To: linuxppc-dev, Michael Ellerman; +Cc: Aneesh Kumar K . V
Excerpts from Michael Ellerman's message of November 8, 2021 3:20 pm:
> Nicholas Piggin <npiggin@gmail.com> writes:
>> In case the FORM2 distance table from firmware is not the expected size,
>> there is fallback code that just populates the lookup table as local vs
>> remote.
>>
>> However it then continues on to use the distance table. Fix.
>>
>> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> Fixes: 1c6b5a7e7405 ("powerpc/pseries: Add support for FORM2 associativity")
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/mm/numa.c | 29 +++++++++++++----------------
>> 1 file changed, 13 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
>> index 6f14c8fb6359..0789cde7f658 100644
>> --- a/arch/powerpc/mm/numa.c
>> +++ b/arch/powerpc/mm/numa.c
>> @@ -380,6 +380,7 @@ static void initialize_form2_numa_distance_lookup_table(void)
>> const __be32 *numa_lookup_index;
>> int numa_dist_table_length;
>> int max_numa_index, distance_index;
>> + bool good = true;
>
> numa_dist_table is a pointer, so couldn't we just set it to NULL if the
> info it's pointing at is invalid?
Yeah probably could just do that.
>
>>
>> if (firmware_has_feature(FW_FEATURE_OPAL))
>> root = of_find_node_by_path("/ibm,opal");
>> @@ -407,30 +408,26 @@ static void initialize_form2_numa_distance_lookup_table(void)
>>
>> if (numa_dist_table_length != max_numa_index * max_numa_index) {
>> WARN(1, "Wrong NUMA distance information\n");
>> - /* consider everybody else just remote. */
>> - for (i = 0; i < max_numa_index; i++) {
>> - for (j = 0; j < max_numa_index; j++) {
>> - int nodeA = numa_id_index_table[i];
>> - int nodeB = numa_id_index_table[j];
>> -
>> - if (nodeA == nodeB)
>> - numa_distance_table[nodeA][nodeB] = LOCAL_DISTANCE;
>> - else
>> - numa_distance_table[nodeA][nodeB] = REMOTE_DISTANCE;
>> - }
>> - }
>> + good = false;
>
> ie. numa_dist_table = NULL;
>
>> }
>> -
>> distance_index = 0;
>> for (i = 0; i < max_numa_index; i++) {
>> for (j = 0; j < max_numa_index; j++) {
>> int nodeA = numa_id_index_table[i];
>> int nodeB = numa_id_index_table[j];
>> -
>> - numa_distance_table[nodeA][nodeB] = numa_dist_table[distance_index++];
>> - pr_debug("dist[%d][%d]=%d ", nodeA, nodeB, numa_distance_table[nodeA][nodeB]);
>> + int dist;
>> +
>> + if (good)
>
> if (numa_dist_table)
>
>> + dist = numa_dist_table[distance_index++];
>> + else if (nodeA == nodeB)
>> + dist = LOCAL_DISTANCE;
>> + else
>> + dist = REMOTE_DISTANCE;
>> + numa_distance_table[nodeA][nodeB] = dist;
>> + pr_debug("dist[%d][%d]=%d ", nodeA, nodeB, dist);
>> }
>> }
>> +
>> of_node_put(root);
>> }
>
>
> But maybe before we do that we can rename it, because it is really easy
> to confuse numa_dist_table and numa_distance_table if you don't look
> closely.
Maybe dt_form2_distances?
Thanks,
Nick
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] powerpc/pseries: Fix numa FORM2 parsing fallback code
2021-11-08 13:53 ` Nicholas Piggin
@ 2021-11-09 1:10 ` Michael Ellerman
0 siblings, 0 replies; 5+ messages in thread
From: Michael Ellerman @ 2021-11-09 1:10 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Aneesh Kumar K . V
Nicholas Piggin <npiggin@gmail.com> writes:
> Excerpts from Michael Ellerman's message of November 8, 2021 3:20 pm:
>> Nicholas Piggin <npiggin@gmail.com> writes:
>>> In case the FORM2 distance table from firmware is not the expected size,
>>> there is fallback code that just populates the lookup table as local vs
>>> remote.
>>>
>>> However it then continues on to use the distance table. Fix.
>>>
>>> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>> Fixes: 1c6b5a7e7405 ("powerpc/pseries: Add support for FORM2 associativity")
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>> arch/powerpc/mm/numa.c | 29 +++++++++++++----------------
>>> 1 file changed, 13 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
>>> index 6f14c8fb6359..0789cde7f658 100644
>>> --- a/arch/powerpc/mm/numa.c
>>> +++ b/arch/powerpc/mm/numa.c
>>> @@ -380,6 +380,7 @@ static void initialize_form2_numa_distance_lookup_table(void)
>>> const __be32 *numa_lookup_index;
>>> int numa_dist_table_length;
>>> int max_numa_index, distance_index;
>>> + bool good = true;
>>
>> numa_dist_table is a pointer, so couldn't we just set it to NULL if the
>> info it's pointing at is invalid?
>
> Yeah probably could just do that.
>
>>
>>>
>>> if (firmware_has_feature(FW_FEATURE_OPAL))
>>> root = of_find_node_by_path("/ibm,opal");
>>> @@ -407,30 +408,26 @@ static void initialize_form2_numa_distance_lookup_table(void)
>>>
>>> if (numa_dist_table_length != max_numa_index * max_numa_index) {
>>> WARN(1, "Wrong NUMA distance information\n");
>>> - /* consider everybody else just remote. */
>>> - for (i = 0; i < max_numa_index; i++) {
>>> - for (j = 0; j < max_numa_index; j++) {
>>> - int nodeA = numa_id_index_table[i];
>>> - int nodeB = numa_id_index_table[j];
>>> -
>>> - if (nodeA == nodeB)
>>> - numa_distance_table[nodeA][nodeB] = LOCAL_DISTANCE;
>>> - else
>>> - numa_distance_table[nodeA][nodeB] = REMOTE_DISTANCE;
>>> - }
>>> - }
>>> + good = false;
>>
>> ie. numa_dist_table = NULL;
>>
>>> }
>>> -
>>> distance_index = 0;
>>> for (i = 0; i < max_numa_index; i++) {
>>> for (j = 0; j < max_numa_index; j++) {
>>> int nodeA = numa_id_index_table[i];
>>> int nodeB = numa_id_index_table[j];
>>> -
>>> - numa_distance_table[nodeA][nodeB] = numa_dist_table[distance_index++];
>>> - pr_debug("dist[%d][%d]=%d ", nodeA, nodeB, numa_distance_table[nodeA][nodeB]);
>>> + int dist;
>>> +
>>> + if (good)
>>
>> if (numa_dist_table)
>>
>>> + dist = numa_dist_table[distance_index++];
>>> + else if (nodeA == nodeB)
>>> + dist = LOCAL_DISTANCE;
>>> + else
>>> + dist = REMOTE_DISTANCE;
>>> + numa_distance_table[nodeA][nodeB] = dist;
>>> + pr_debug("dist[%d][%d]=%d ", nodeA, nodeB, dist);
>>> }
>>> }
>>> +
>>> of_node_put(root);
>>> }
>>
>>
>> But maybe before we do that we can rename it, because it is really easy
>> to confuse numa_dist_table and numa_distance_table if you don't look
>> closely.
>
> Maybe dt_form2_distances?
Or just "form2_distances", it's only a local so the fact that it's from
the dt is clear enough I think.
cheers
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-11-09 1:11 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-05 13:29 [PATCH] powerpc/pseries: Fix numa FORM2 parsing fallback code Nicholas Piggin
2021-11-05 16:05 ` Aneesh Kumar K.V
2021-11-08 5:20 ` Michael Ellerman
2021-11-08 13:53 ` Nicholas Piggin
2021-11-09 1:10 ` Michael Ellerman
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.