Re: [PATCH v6 2/6] spapr_numa.c: scrap 'legacy_numa' concept

[Greg Kurz]

On Fri, 10 Sep 2021 16:55:35 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> When first introduced, 'legacy_numa' was a way to refer to guests that
> either wouldn't be affected by associativity domain calculations, namely
> the ones with only 1 NUMA node, and pre 5.2 guests that shouldn't be
> affected by it because it would be an userspace change. Calling these
> cases 'legacy_numa' was a convenient way to label these cases.
> 
> We're about to introduce a new NUMA affinity, FORM2, and this concept
> of 'legacy_numa' is now a bit misleading because, although it is called
> 'legacy' it is in fact a FORM1 exclusive contraint.
> 
> This patch removes spapr_machine_using_legacy_numa() and open code the
> conditions in each caller. While we're at it, move the chunk inside
> spapr_numa_FORM1_affinity_init() that sets all numa_assoc_array domains
> with 'node_id' to spapr_numa_define_FORM1_domains(). This chunk was
> being executed if !pre_5_2_numa_associativity and num_nodes => 1, the
> same conditions in which spapr_numa_define_FORM1_domains() is called
> shortly after.
> 

Those are definitely improvements. Just one question, see below.

> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  hw/ppc/spapr_numa.c | 46 +++++++++++++++++++--------------------------
>  1 file changed, 19 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
> index 786def7c73..fb6059550f 100644
> --- a/hw/ppc/spapr_numa.c
> +++ b/hw/ppc/spapr_numa.c
> @@ -19,15 +19,6 @@
>  /* Moved from hw/ppc/spapr_pci_nvlink2.c */
>  #define SPAPR_GPU_NUMA_ID           (cpu_to_be32(1))
>  
> -static bool spapr_machine_using_legacy_numa(SpaprMachineState *spapr)
> -{
> -    MachineState *machine = MACHINE(spapr);
> -    SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
> -
> -    return smc->pre_5_2_numa_associativity ||
> -           machine->numa_state->num_nodes <= 1;
> -}
> -
>  static bool spapr_numa_is_symmetrical(MachineState *ms)
>  {
>      int src, dst;
> @@ -97,7 +88,18 @@ static void 
> spapr_numa_define_FORM1_domains(SpaprMachineState *spapr)
>      MachineState *ms = MACHINE(spapr);
>      NodeInfo *numa_info = ms->numa_state->nodes;
>      int nb_numa_nodes = ms->numa_state->num_nodes;
> -    int src, dst, i;
> +    int src, dst, i, j;
> +
> +    /*
> +     * Fill all associativity domains of non-zero NUMA nodes with
> +     * node_id. This is required because the default value (0) is
> +     * considered a match with associativity domains of node 0.
> +     */
> +    for (i = 1; i < nb_numa_nodes; i++) {
> +        for (j = 1; j < MAX_DISTANCE_REF_POINTS; j++) {
> +            spapr->numa_assoc_array[i][j] = cpu_to_be32(i);
> +        }
> +    }
>  
>      for (src = 0; src < nb_numa_nodes; src++) {
>          for (dst = src; dst < nb_numa_nodes; dst++) {
> @@ -164,7 +166,6 @@ static void 
> spapr_numa_FORM1_affinity_init(SpaprMachineState *spapr,
>      SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
>      int nb_numa_nodes = machine->numa_state->num_nodes;
>      int i, j, max_nodes_with_gpus;
> -    bool using_legacy_numa = spapr_machine_using_legacy_numa(spapr);
>  
>      /*
>       * For all associativity arrays: first position is the size,
> @@ -178,17 +179,6 @@ static void 
> spapr_numa_FORM1_affinity_init(SpaprMachineState *spapr,
>      for (i = 0; i < nb_numa_nodes; i++) {
>          spapr->numa_assoc_array[i][0] = cpu_to_be32(MAX_DISTANCE_REF_POINTS);
>          spapr->numa_assoc_array[i][MAX_DISTANCE_REF_POINTS] = cpu_to_be32(i);
> -
> -        /*
> -         * Fill all associativity domains of non-zero NUMA nodes with
> -         * node_id. This is required because the default value (0) is
> -         * considered a match with associativity domains of node 0.
> -         */
> -        if (!using_legacy_numa && i != 0) {
> -            for (j = 1; j < MAX_DISTANCE_REF_POINTS; j++) {
> -                spapr->numa_assoc_array[i][j] = cpu_to_be32(i);
> -            }
> -        }
>      }
>  
>      /*
> @@ -214,11 +204,13 @@ static void 
> spapr_numa_FORM1_affinity_init(SpaprMachineState *spapr,
>      }
>  
>      /*
> -     * Legacy NUMA guests (pseries-5.1 and older, or guests with only
> -     * 1 NUMA node) will not benefit from anything we're going to do
> -     * after this point.
> +     * Guests pseries-5.1 and older uses zeroed associativity domains,
> +     * i.e. no domain definition based on NUMA distance input.
> +     *
> +     * Same thing with guests that have only one NUMA node.
>       */
> -    if (using_legacy_numa) {
> +    if (smc->pre_5_2_numa_associativity ||
> +        machine->numa_state->num_nodes <= 1) {
>          return;
>      }
>  
> @@ -334,7 +326,7 @@ static void 
> spapr_numa_FORM1_write_rtas_dt(SpaprMachineState *spapr,
>          cpu_to_be32(maxdomain)
>      };
>  
> -    if (spapr_machine_using_legacy_numa(spapr)) {
> +    if (smc->pre_5_2_numa_associativity) {

This doesn't check the number of NUMA nodes and thus changes behavior,
or am I missing something ?

Rest looks good though so if you add the missing check or provide a
justification, feel free to add:

Reviewed-by: Greg Kurz <groug@kaod.org>

>          uint32_t legacy_refpoints[] = {
>              cpu_to_be32(0x4),
>              cpu_to_be32(0x4),