Re: [PATCH v7 7/7] spapr_numa.c: handle auto NUMA node with no distance info

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 15 Sep 2021 22:30:04 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> numa_complete_configuration() in hw/core/numa.c always adds a NUMA node
> for the pSeries machine if none was specified, but without node distance
> information for the single node created.

distance is optional feature, hence generic auto create magic doesn't
do anything with it (it does bare minimum for memhotplug to work).
I'd like to drop auto-generated node altogether and ask user to explicitly
provide needed -numa options (now with deprecation it's possible) if it's required.

> This added node is also not
> accounted for in numa_state->num_nodes, which returns zero.

that's probably a bug, parse_numa_node() should always increments on success,
can you check why it doesn't happen in your case?

> NUMA FORM1 affinity code didn't rely on numa_state information to do its
> job, but FORM2 does. As is now, this is the result of a pSeries guest
> with NUMA FORM2 affinity when no NUMA nodes is specified:
> 
> $ numactl -H available: 1 nodes (0) node 0 cpus: 0 node 0 size: 16222 MB
> node 0 free: 15681 MB No distance information available.
> 
> This can be amended in spapr_numa_FORM2_write_rtas_tables(). We're
> always expecting at least one NUMA node, and we're going to enforce that
> the local distance (the distance to the node to itself) is always 10.
> This allows for the proper creation of the NUMA distance tables, fixing
> the output of 'numactl -H' in the guest:
> 
> $ numactl -H available: 1 nodes (0) node 0 cpus: 0 node 0 size: 16222 MB
> node 0 free: 15685 MB node distances: node   0
>   0:  10
> 
> CC: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> 
> Igor,
> 
> CCing you as a FYI. I wasn't sure whether there is a reason for
> numa_complete_configuration() not adding distance info an update 'num_nodes'
> for the auto-generated NUMA node, I decided to handle this case in
> pseries side instead.
> 
> 
> 
>  hw/ppc/spapr_numa.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
> index 659513b405..d8caf5f6bd 100644
> --- a/hw/ppc/spapr_numa.c
> +++ b/hw/ppc/spapr_numa.c
> @@ -499,7 +499,7 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>  {
>      MachineState *ms = MACHINE(spapr);
>      NodeInfo *numa_info = ms->numa_state->nodes;
> -    int nb_numa_nodes = ms->numa_state->num_nodes;
> +    int nb_numa_nodes = ms->numa_state->num_nodes ?: 1;
>      int distance_table_entries = nb_numa_nodes * nb_numa_nodes;
>      g_autofree uint32_t *lookup_index_table = NULL;
>      g_autofree uint32_t *distance_table = NULL;
> @@ -539,6 +539,17 @@ static void spapr_numa_FORM2_write_rtas_tables(SpaprMachineState *spapr,
>  
>      for (src = 0; src < nb_numa_nodes; src++) {
>          for (dst = 0; dst < nb_numa_nodes; dst++) {
> +            /*
> +             * We need to be explicit with the local distance
> +             * value to cover the case where the user didn't added any
> +             * NUMA nodes, but QEMU adds the default NUMA node without
> +             * adding the numa_info to retrieve the info from.
> +             */
> +            if (src == dst) {
> +                node_distances[i++] = 10;
> +                continue;
> +            }
> +
>              node_distances[i++] = numa_info[src].distance[dst];
>          }
>      }