Re: [PATCH] spapr_numa.c: FORM2 table handle nodes with no distance info

[Daniel Henrique Barboza]

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++;
> > >            }
> > >        }