All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] spapr_numa.c: FORM2 table handle nodes with no distance info
@ 2021-11-05 13:51 Nicholas Piggin
  2021-11-05 18:52 ` Daniel Henrique Barboza
  0 siblings, 1 reply; 6+ messages in thread
From: Nicholas Piggin @ 2021-11-05 13:51 UTC (permalink / raw)
  To: qemu-ppc
  Cc: Aneesh Kumar K . V, Daniel Henrique Barboza, qemu-devel,
	Nicholas Piggin, David Gibson

A configuration that specifies multiple nodes without distance info
results in the non-local points in the FORM2 matrix having a distance of
0. This causes Linux to complain "Invalid distance value range" because
a node distance is smaller than the local distance.

Fix this by building a simple local / remote fallback for points where
distance information is missing.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/spapr_numa.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
index 5822938448..56ab2a5fb6 100644
--- a/hw/ppc/spapr_numa.c
+++ b/hw/ppc/spapr_numa.c
@@ -546,12 +546,24 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
              * NUMA nodes, but QEMU adds the default NUMA node without
              * adding the numa_info to retrieve distance info from.
              */
-            if (src == dst) {
-                distance_table[i++] = NUMA_DISTANCE_MIN;
-                continue;
+            distance_table[i] = numa_info[src].distance[dst];
+            if (distance_table[i] == 0) {
+                /*
+                 * In case QEMU adds a default NUMA single node when the user
+                 * did not add any, or where the user did not supply distances,
+                 * the value will be 0 here. Populate the table with a fallback
+                 * simple local / remote distance.
+                 */
+                if (src == dst) {
+                    distance_table[i] = NUMA_DISTANCE_MIN;
+                } else {
+                    distance_table[i] = numa_info[src].distance[dst];
+                    if (distance_table[i] < NUMA_DISTANCE_MIN) {
+                        distance_table[i] = NUMA_DISTANCE_DEFAULT;
+                    }
+                }
             }
-
-            distance_table[i++] = numa_info[src].distance[dst];
+            i++;
         }
     }
 
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] spapr_numa.c: FORM2 table handle nodes with no distance info
  2021-11-05 13:51 [PATCH] spapr_numa.c: FORM2 table handle nodes with no distance info Nicholas Piggin
@ 2021-11-05 18:52 ` Daniel Henrique Barboza
  2021-11-08  3:26   ` David Gibson
  2021-11-08  4:22   ` Aneesh Kumar K.V
  0 siblings, 2 replies; 6+ messages in thread
From: Daniel Henrique Barboza @ 2021-11-05 18:52 UTC (permalink / raw)
  To: Nicholas Piggin, qemu-ppc; +Cc: Aneesh Kumar K . V, qemu-devel, David Gibson



On 11/5/21 10:51, Nicholas Piggin wrote:
> A configuration that specifies multiple nodes without distance info
> results in the non-local points in the FORM2 matrix having a distance of
> 0. This causes Linux to complain "Invalid distance value range" because
> a node distance is smaller than the local distance.
> 
> Fix this by building a simple local / remote fallback for points where
> distance information is missing.

Thanks for looking this up. I checked the output of this same scenario with
a FORM1 guest and 4 distance-less NUMA nodes. This is what I got:

[root@localhost ~]# numactl -H
available: 4 nodes (0-3)
(...)
node distances:
node   0   1   2   3
   0:  10  160  160  160
   1:  160  10  160  160
   2:  160  160  10  160
   3:  160  160  160  10
[root@localhost ~]#


With this patch we're getting '20' instead of '160' because you're using
NUMA_DISTANCE_DEFAULT, while FORM1 will default this case to the maximum
NUMA distance the kernel allows for that affinity (160).

I do not have strong feelings about changing this behavior between FORM1 and
FORM2. I tested the same scenario with a x86_64 guest and they also uses '20'
in this case as well, so far as QEMU goes using NUMA_DISTANCE_DEFAULT is
consistent.

Aneesh is already in CC, so I believe he'll let us know if there's something
we're missing and we need to preserve the '160' distance in FORM2 for this
case as well.

For now:


> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---


Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>



>   hw/ppc/spapr_numa.c | 22 +++++++++++++++++-----
>   1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
> index 5822938448..56ab2a5fb6 100644
> --- a/hw/ppc/spapr_numa.c
> +++ b/hw/ppc/spapr_numa.c
> @@ -546,12 +546,24 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>                * NUMA nodes, but QEMU adds the default NUMA node without
>                * adding the numa_info to retrieve distance info from.
>                */
> -            if (src == dst) {
> -                distance_table[i++] = NUMA_DISTANCE_MIN;
> -                continue;
> +            distance_table[i] = numa_info[src].distance[dst];
> +            if (distance_table[i] == 0) {
> +                /*
> +                 * In case QEMU adds a default NUMA single node when the user
> +                 * did not add any, or where the user did not supply distances,
> +                 * the value will be 0 here. Populate the table with a fallback
> +                 * simple local / remote distance.
> +                 */
> +                if (src == dst) {
> +                    distance_table[i] = NUMA_DISTANCE_MIN;
> +                } else {
> +                    distance_table[i] = numa_info[src].distance[dst];
> +                    if (distance_table[i] < NUMA_DISTANCE_MIN) {
> +                        distance_table[i] = NUMA_DISTANCE_DEFAULT;
> +                    }
> +                }
>               }
> -
> -            distance_table[i++] = numa_info[src].distance[dst];
> +            i++;
>           }
>       }
>   
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] spapr_numa.c: FORM2 table handle nodes with no distance info
  2021-11-05 18:52 ` Daniel Henrique Barboza
@ 2021-11-08  3:26   ` David Gibson
  2021-11-08  4:22   ` Aneesh Kumar K.V
  1 sibling, 0 replies; 6+ messages in thread
From: David Gibson @ 2021-11-08  3:26 UTC (permalink / raw)
  To: Daniel Henrique Barboza
  Cc: Aneesh Kumar K . V, qemu-ppc, qemu-devel, Nicholas Piggin

[-- Attachment #1: Type: text/plain, Size: 3735 bytes --]

On Fri, Nov 05, 2021 at 03:52:13PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 11/5/21 10:51, Nicholas Piggin wrote:
> > A configuration that specifies multiple nodes without distance info
> > results in the non-local points in the FORM2 matrix having a distance of
> > 0. This causes Linux to complain "Invalid distance value range" because
> > a node distance is smaller than the local distance.
> > 
> > Fix this by building a simple local / remote fallback for points where
> > distance information is missing.
> 
> Thanks for looking this up. I checked the output of this same scenario with
> a FORM1 guest and 4 distance-less NUMA nodes. This is what I got:
> 
> [root@localhost ~]# numactl -H
> available: 4 nodes (0-3)
> (...)
> node distances:
> node   0   1   2   3
>   0:  10  160  160  160
>   1:  160  10  160  160
>   2:  160  160  10  160
>   3:  160  160  160  10
> [root@localhost ~]#
> 
> 
> With this patch we're getting '20' instead of '160' because you're using
> NUMA_DISTANCE_DEFAULT, while FORM1 will default this case to the maximum
> NUMA distance the kernel allows for that affinity (160).
> 
> I do not have strong feelings about changing this behavior between FORM1 and
> FORM2. I tested the same scenario with a x86_64 guest and they also uses '20'
> in this case as well, so far as QEMU goes using NUMA_DISTANCE_DEFAULT is
> consistent.
> 
> Aneesh is already in CC, so I believe he'll let us know if there's something
> we're missing and we need to preserve the '160' distance in FORM2 for this
> case as well.
> 
> For now:
> 
> 
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> 
> 
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Applied to ppc-for-6.2, thanks.

> 
> 
> 
> >   hw/ppc/spapr_numa.c | 22 +++++++++++++++++-----
> >   1 file changed, 17 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
> > index 5822938448..56ab2a5fb6 100644
> > --- a/hw/ppc/spapr_numa.c
> > +++ b/hw/ppc/spapr_numa.c
> > @@ -546,12 +546,24 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
> >                * NUMA nodes, but QEMU adds the default NUMA node without
> >                * adding the numa_info to retrieve distance info from.
> >                */
> > -            if (src == dst) {
> > -                distance_table[i++] = NUMA_DISTANCE_MIN;
> > -                continue;
> > +            distance_table[i] = numa_info[src].distance[dst];
> > +            if (distance_table[i] == 0) {
> > +                /*
> > +                 * In case QEMU adds a default NUMA single node when the user
> > +                 * did not add any, or where the user did not supply distances,
> > +                 * the value will be 0 here. Populate the table with a fallback
> > +                 * simple local / remote distance.
> > +                 */
> > +                if (src == dst) {
> > +                    distance_table[i] = NUMA_DISTANCE_MIN;
> > +                } else {
> > +                    distance_table[i] = numa_info[src].distance[dst];
> > +                    if (distance_table[i] < NUMA_DISTANCE_MIN) {
> > +                        distance_table[i] = NUMA_DISTANCE_DEFAULT;
> > +                    }
> > +                }
> >               }
> > -
> > -            distance_table[i++] = numa_info[src].distance[dst];
> > +            i++;
> >           }
> >       }
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] spapr_numa.c: FORM2 table handle nodes with no distance info
  2021-11-05 18:52 ` Daniel Henrique Barboza
  2021-11-08  3:26   ` David Gibson
@ 2021-11-08  4:22   ` Aneesh Kumar K.V
  2021-11-08 13:51     ` Nicholas Piggin
  2021-11-08 21:12     ` Daniel Henrique Barboza
  1 sibling, 2 replies; 6+ messages in thread
From: Aneesh Kumar K.V @ 2021-11-08  4:22 UTC (permalink / raw)
  To: Daniel Henrique Barboza, Nicholas Piggin, qemu-ppc
  Cc: qemu-devel, David Gibson

Daniel Henrique Barboza <danielhb413@gmail.com> writes:

> On 11/5/21 10:51, Nicholas Piggin wrote:
>> A configuration that specifies multiple nodes without distance info
>> results in the non-local points in the FORM2 matrix having a distance of
>> 0. This causes Linux to complain "Invalid distance value range" because
>> a node distance is smaller than the local distance.
>> 
>> Fix this by building a simple local / remote fallback for points where
>> distance information is missing.
>
> Thanks for looking this up. I checked the output of this same scenario with
> a FORM1 guest and 4 distance-less NUMA nodes. This is what I got:
>
> [root@localhost ~]# numactl -H
> available: 4 nodes (0-3)
> (...)
> node distances:
> node   0   1   2   3
>    0:  10  160  160  160
>    1:  160  10  160  160
>    2:  160  160  10  160
>    3:  160  160  160  10
> [root@localhost ~]#
>
>
> With this patch we're getting '20' instead of '160' because you're using
> NUMA_DISTANCE_DEFAULT, while FORM1 will default this case to the maximum
> NUMA distance the kernel allows for that affinity (160).

where is that enforced? Do we know why FORM1 picked 160? 

>
> I do not have strong feelings about changing this behavior between FORM1 and
> FORM2. I tested the same scenario with a x86_64 guest and they also uses '20'
> in this case as well, so far as QEMU goes using NUMA_DISTANCE_DEFAULT is
> consistent.
>

for FORM2 I would suggest 20 is the right value and it is also
consistent with other architectures. 

> Aneesh is already in CC, so I believe he'll let us know if there's something
> we're missing and we need to preserve the '160' distance in FORM2 for this
> case as well.
>
> For now:
>
>
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>
>
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>
>
>
>>   hw/ppc/spapr_numa.c | 22 +++++++++++++++++-----
>>   1 file changed, 17 insertions(+), 5 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
>> index 5822938448..56ab2a5fb6 100644
>> --- a/hw/ppc/spapr_numa.c
>> +++ b/hw/ppc/spapr_numa.c
>> @@ -546,12 +546,24 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>>                * NUMA nodes, but QEMU adds the default NUMA node without
>>                * adding the numa_info to retrieve distance info from.
>>                */
>> -            if (src == dst) {
>> -                distance_table[i++] = NUMA_DISTANCE_MIN;
>> -                continue;

We always initialized the local distance to be NUMA_DISTANCE_MIN
irrespective of what is specified via Qemu command line before? If so
then the above change will break that? 

>> +            distance_table[i] = numa_info[src].distance[dst];
>> +            if (distance_table[i] == 0) {

we know distance_table[i] is == 0 here and ..

>> +                /*
>> +                 * In case QEMU adds a default NUMA single node when the user
>> +                 * did not add any, or where the user did not supply distances,
>> +                 * the value will be 0 here. Populate the table with a fallback
>> +                 * simple local / remote distance.
>> +                 */
>> +                if (src == dst) {
>> +                    distance_table[i] = NUMA_DISTANCE_MIN;
>> +                } else {
>> +                    distance_table[i] = numa_info[src].distance[dst];
>> +                    if (distance_table[i] < NUMA_DISTANCE_MIN) {


considering we reached here after checking distance_table[i] == 0 do we
need to do the above two lines?

>> +                        distance_table[i] = NUMA_DISTANCE_DEFAULT;
>> +                    }
>> +                }
>>               }
>> -
>> -            distance_table[i++] = numa_info[src].distance[dst];
>> +            i++;
>>           }
>>       }




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] spapr_numa.c: FORM2 table handle nodes with no distance info
  2021-11-08  4:22   ` Aneesh Kumar K.V
@ 2021-11-08 13:51     ` Nicholas Piggin
  2021-11-08 21:12     ` Daniel Henrique Barboza
  1 sibling, 0 replies; 6+ messages in thread
From: Nicholas Piggin @ 2021-11-08 13:51 UTC (permalink / raw)
  To: Aneesh Kumar K.V, Daniel Henrique Barboza, qemu-ppc
  Cc: qemu-devel, David Gibson

Excerpts from Aneesh Kumar K.V's message of November 8, 2021 2:22 pm:
> Daniel Henrique Barboza <danielhb413@gmail.com> writes:
> 
>> On 11/5/21 10:51, Nicholas Piggin wrote:
>>> A configuration that specifies multiple nodes without distance info
>>> results in the non-local points in the FORM2 matrix having a distance of
>>> 0. This causes Linux to complain "Invalid distance value range" because
>>> a node distance is smaller than the local distance.
>>> 
>>> Fix this by building a simple local / remote fallback for points where
>>> distance information is missing.
>>
>> Thanks for looking this up. I checked the output of this same scenario with
>> a FORM1 guest and 4 distance-less NUMA nodes. This is what I got:
>>
>> [root@localhost ~]# numactl -H
>> available: 4 nodes (0-3)
>> (...)
>> node distances:
>> node   0   1   2   3
>>    0:  10  160  160  160
>>    1:  160  10  160  160
>>    2:  160  160  10  160
>>    3:  160  160  160  10
>> [root@localhost ~]#
>>
>>
>> With this patch we're getting '20' instead of '160' because you're using
>> NUMA_DISTANCE_DEFAULT, while FORM1 will default this case to the maximum
>> NUMA distance the kernel allows for that affinity (160).
> 
> where is that enforced? Do we know why FORM1 picked 160? 
> 
>>
>> I do not have strong feelings about changing this behavior between FORM1 and
>> FORM2. I tested the same scenario with a x86_64 guest and they also uses '20'
>> in this case as well, so far as QEMU goes using NUMA_DISTANCE_DEFAULT is
>> consistent.
>>
> 
> for FORM2 I would suggest 20 is the right value and it is also
> consistent with other architectures. 
> 
>> Aneesh is already in CC, so I believe he'll let us know if there's something
>> we're missing and we need to preserve the '160' distance in FORM2 for this
>> case as well.
>>
>> For now:
>>
>>
>>> 
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>
>>
>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>
>>
>>
>>>   hw/ppc/spapr_numa.c | 22 +++++++++++++++++-----
>>>   1 file changed, 17 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
>>> index 5822938448..56ab2a5fb6 100644
>>> --- a/hw/ppc/spapr_numa.c
>>> +++ b/hw/ppc/spapr_numa.c
>>> @@ -546,12 +546,24 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>>>                * NUMA nodes, but QEMU adds the default NUMA node without
>>>                * adding the numa_info to retrieve distance info from.
>>>                */
>>> -            if (src == dst) {
>>> -                distance_table[i++] = NUMA_DISTANCE_MIN;
>>> -                continue;
> 
> We always initialized the local distance to be NUMA_DISTANCE_MIN
> irrespective of what is specified via Qemu command line before? If so
> then the above change will break that? 

That's true. I think command line should take priority and if we have to 
override it for some reason then we should print a warning.

> 
>>> +            distance_table[i] = numa_info[src].distance[dst];
>>> +            if (distance_table[i] == 0) {
> 
> we know distance_table[i] is == 0 here and ..
> 
>>> +                /*
>>> +                 * In case QEMU adds a default NUMA single node when the user
>>> +                 * did not add any, or where the user did not supply distances,
>>> +                 * the value will be 0 here. Populate the table with a fallback
>>> +                 * simple local / remote distance.
>>> +                 */
>>> +                if (src == dst) {
>>> +                    distance_table[i] = NUMA_DISTANCE_MIN;
>>> +                } else {
>>> +                    distance_table[i] = numa_info[src].distance[dst];
>>> +                    if (distance_table[i] < NUMA_DISTANCE_MIN) {
> 
> 
> considering we reached here after checking distance_table[i] == 0 do we
> need to do the above two lines?

Oh that's true. I think the lines could just be removed.

Thanks,
Nick

> 
>>> +                        distance_table[i] = NUMA_DISTANCE_DEFAULT;
>>> +                    }
>>> +                }
>>>               }
>>> -
>>> -            distance_table[i++] = numa_info[src].distance[dst];
>>> +            i++;
>>>           }
>>>       }
> 
> 
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] spapr_numa.c: FORM2 table handle nodes with no distance info
  2021-11-08  4:22   ` Aneesh Kumar K.V
  2021-11-08 13:51     ` Nicholas Piggin
@ 2021-11-08 21:12     ` Daniel Henrique Barboza
  1 sibling, 0 replies; 6+ messages in thread
From: Daniel Henrique Barboza @ 2021-11-08 21:12 UTC (permalink / raw)
  To: Aneesh Kumar K.V, Nicholas Piggin, qemu-ppc; +Cc: qemu-devel, David Gibson



On 11/8/21 01:22, Aneesh Kumar K.V wrote:
> Daniel Henrique Barboza <danielhb413@gmail.com> writes:
> 
>> On 11/5/21 10:51, Nicholas Piggin wrote:
>>> A configuration that specifies multiple nodes without distance info
>>> results in the non-local points in the FORM2 matrix having a distance of
>>> 0. This causes Linux to complain "Invalid distance value range" because
>>> a node distance is smaller than the local distance.
>>>
>>> Fix this by building a simple local / remote fallback for points where
>>> distance information is missing.
>>
>> Thanks for looking this up. I checked the output of this same scenario with
>> a FORM1 guest and 4 distance-less NUMA nodes. This is what I got:
>>
>> [root@localhost ~]# numactl -H
>> available: 4 nodes (0-3)
>> (...)
>> node distances:
>> node   0   1   2   3
>>     0:  10  160  160  160
>>     1:  160  10  160  160
>>     2:  160  160  10  160
>>     3:  160  160  160  10
>> [root@localhost ~]#
>>
>>
>> With this patch we're getting '20' instead of '160' because you're using
>> NUMA_DISTANCE_DEFAULT, while FORM1 will default this case to the maximum
>> NUMA distance the kernel allows for that affinity (160).
> 
> where is that enforced? Do we know why FORM1 picked 160?


It's the kernel algorithm that determines FORM1 distance. It doubles the
distance value of the previous level. It starts with the LOCAL_DISTANCE (10)
for the first NUMA level, second level is 10*2, and so on for all 4
reference-points (10, 20, 40, 80). If no match is found in the 4th level,
it doubles once more, giving us 160.

What is happening here is that the absence of a distance (distance == 0)
is being handled by FORM1 code in QEMU in a way that the associativity domains
will cause this kernel behavior I described above.

I'll check it out later and see if that's easily fixable.

> 
>>
>> I do not have strong feelings about changing this behavior between FORM1 and
>> FORM2. I tested the same scenario with a x86_64 guest and they also uses '20'
>> in this case as well, so far as QEMU goes using NUMA_DISTANCE_DEFAULT is
>> consistent.
>>
> 
> for FORM2 I would suggest 20 is the right value and it is also
> consistent with other architectures.
> 
>> Aneesh is already in CC, so I believe he'll let us know if there's something
>> we're missing and we need to preserve the '160' distance in FORM2 for this
>> case as well.
>>
>> For now:
>>
>>
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>
>>
>> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>
>>
>>
>>>    hw/ppc/spapr_numa.c | 22 +++++++++++++++++-----
>>>    1 file changed, 17 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
>>> index 5822938448..56ab2a5fb6 100644
>>> --- a/hw/ppc/spapr_numa.c
>>> +++ b/hw/ppc/spapr_numa.c
>>> @@ -546,12 +546,24 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>>>                 * NUMA nodes, but QEMU adds the default NUMA node without
>>>                 * adding the numa_info to retrieve distance info from.
>>>                 */
>>> -            if (src == dst) {
>>> -                distance_table[i++] = NUMA_DISTANCE_MIN;
>>> -                continue;
> 
> We always initialized the local distance to be NUMA_DISTANCE_MIN
> irrespective of what is specified via Qemu command line before? If so
> then the above change will break that?

No. I added this piece of code above because QEMU can auto-generate a single
NUMA node if the user added no NUMA nodes in the command line. This
auto-generated NUMA node didn't have the local_distance for itself set. That's
the only case where I was setting distance = 10. The remaining entries
were being written as-is. And now we need Nick's patch as well because
I missed other instances of absent distances hehe

I don't believe that we're breaking anything with this patch because we're
checking for distance = 0 first, and QEMU doesn't allow any distance < 10 to
be set:

-numa dist,src=0,dst=1,val=3
qemu-system-x86_64: -numa dist,src=0,dst=1,val=3: NUMA distance (3) is invalid, it shouldn't be less than 10


This means that we're not overwriting any user setting by accident.

> 
>>> +            distance_table[i] = numa_info[src].distance[dst];
>>> +            if (distance_table[i] == 0) {
> 
> we know distance_table[i] is == 0 here and ..
> 
>>> +                /*
>>> +                 * In case QEMU adds a default NUMA single node when the user
>>> +                 * did not add any, or where the user did not supply distances,
>>> +                 * the value will be 0 here. Populate the table with a fallback
>>> +                 * simple local / remote distance.
>>> +                 */
>>> +                if (src == dst) {
>>> +                    distance_table[i] = NUMA_DISTANCE_MIN;
>>> +                } else {
>>> +                    distance_table[i] = numa_info[src].distance[dst];
>>> +                    if (distance_table[i] < NUMA_DISTANCE_MIN) {
> 
> 
> considering we reached here after checking distance_table[i] == 0 do we
> need to do the above two lines?

You're right. I believe we can make it work with

                 if (src == dst) {
                     distance_table[i] = NUMA_DISTANCE_MIN;
                 } else {
                     distance_table[i] = NUMA_DISTANCE_DEFAULT;
                 }


Nick, care to re-send?



Thanks,



Daniel

> 
>>> +                        distance_table[i] = NUMA_DISTANCE_DEFAULT;
>>> +                    }
>>> +                }
>>>                }
>>> -
>>> -            distance_table[i++] = numa_info[src].distance[dst];
>>> +            i++;
>>>            }
>>>        }
> 
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-11-08 21:13 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-05 13:51 [PATCH] spapr_numa.c: FORM2 table handle nodes with no distance info Nicholas Piggin
2021-11-05 18:52 ` Daniel Henrique Barboza
2021-11-08  3:26   ` David Gibson
2021-11-08  4:22   ` Aneesh Kumar K.V
2021-11-08 13:51     ` Nicholas Piggin
2021-11-08 21:12     ` Daniel Henrique Barboza

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.