Re: [PATCH v3 4/5] spapr_numa: consider user input when defining associativity

[David Gibson]

On Tue, Sep 29, 2020 at 10:38:16AM -0300, Daniel Henrique Barboza wrote:
> This patch puts all the pieces together to finally allow user
> input when defining the NUMA topology of the spapr guest.
> 
> For each NUMA node A, starting at node id 0, the new
> spapr_numa_define_associativity_domains() will:
> 
> - get the distance between node A and B = A + 1
> - get the correspondent NUMA level for this distance
> - assign the associativity domain for A and B for the given
> NUMA level, using the lowest associativity domain value between
> them
> - if there is more NUMA nodes, increment B and repeat

I still find this description very confusing.  The one in the comment
is better, I think, can you maybe copy that one here.

> Since we always start at the first node (id = 0) and go in
> ascending order, we are prioritizing any previous associativity
> already calculated. This is necessary because neither QEMU, nor
> the pSeries kernel, supports multiple associativity domains for
> each resource, meaning that we have to decide which associativity
> relation is relevant. Another side effect is that the first
> NUMA node, node 0, will always have an associativity array
> full of zeroes. This is intended - in fact, the Linux kernel
> expects it (see [1] for more info).
> 
> Ultimately, all of this results in a best effort approximation for
> the actual NUMA distances the user input in the command line. Given
> the nature of how PAPR itself interprets NUMA distances versus the
> expectations risen by how ACPI SLIT works, there might be better
> algorithms but, in the end, it'll also result in another way to
> approximate what the user really wanted.
> 
> To keep this commit message no longer than it already is, the next
> patch will update the existing documentation in ppc-spapr-numa.rst
> with more in depth details and design considerations/drawbacks.
> 
> [1] 
> https://lore.kernel.org/linuxppc-dev/5e8fbea3-8faf-0951-172a-b41a2138fbcf@gmail.com/
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  hw/ppc/spapr_numa.c | 120 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 119 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_numa.c b/hw/ppc/spapr_numa.c
> index 16badb1f4b..f3d43ceb1e 100644
> --- a/hw/ppc/spapr_numa.c
> +++ b/hw/ppc/spapr_numa.c
> @@ -37,12 +37,118 @@ static bool spapr_numa_is_symmetrical(MachineState *ms)
>      return true;
>  }
>  
> +/*
> + * This function will translate the user distances into
> + * what the kernel understand as possible values: 10
> + * (local distance), 20, 40, 80 and 160, and return the equivalent
> + * NUMA level for each. Current heuristic is:
> + *  - local distance (10) returns numa_level = 0x4
> + *  - distances between 11 and 30 inclusive -> rounded to 20,
> + *    numa_level = 0x3
> + *  - distances between 31 and 60 inclusive -> rounded to 40,
> + *    numa_level = 0x2
> + *  - distances between 61 and 120 inclusive -> rounded to 80,
> + *    numa_level = 0x1
> + *  - everything above 120 returns numa_level = 0 to indicate that
> + *    there is no match. This will be calculated as disntace = 160
> + *    by the kernel (as of v5.9)
> + */
> +static uint8_t spapr_numa_get_numa_level(uint8_t distance)
> +{
> +    uint8_t rounded_distance = 160;
> +    uint8_t numa_level;
> +
> +    if (distance > 11 && distance <= 30) {
> +        rounded_distance = 20;
> +    } else if (distance > 31 && distance <= 60) {
> +        rounded_distance = 40;
> +    } else if (distance > 61 && distance <= 120) {
> +        rounded_distance = 80;
> +    }
> +
> +    switch (rounded_distance) {
> +    case 10:
> +        numa_level = 0x4;
> +        break;

Uh.. you could just return the numa_level from the if-else chain
without going via rounded_distance.  (You could put the rounded
distances in comments on each if clause, if you like).

> +    case 20:
> +        numa_level = 0x3;
> +        break;
> +    case 40:
> +        numa_level = 0x2;
> +        break;
> +    case 80:
> +        numa_level = 0x1;
> +        break;
> +    default:
> +        numa_level = 0;
> +    }
> +
> +    return numa_level;
> +}
> +
> +static void spapr_numa_define_associativity_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;
> +
> +    for (src = 0; src < nb_numa_nodes; src++) {
> +        for (dst = src; dst < nb_numa_nodes; dst++) {
> +            /*
> +             * This is how the associativity domain between A and B
> +             * is calculated:
> +             *
> +             * - get the distance between them
> +             * - get the correspondent NUMA level for this distance
> +             * - the arrays were initialized with their own numa_ids,
> +             * and we're calculating the distance in node_id ascending order,
> +             * starting from node 0. This will have a cascade effect in the
> +             * algorithm because the associativity domains that node 0 
> defines
> +             * will be carried over to the other nodes, and node 1
> +             * associativities will be carried over unless there's already a
> +             * node 0 associativity assigned, and so on. This happens because
> +             * we'll assign assoc_src as the associativity domain of dst
> +             * as well, for the given NUMA level.
> +             *
> +             * The PPC kernel expects the associativity domains of node 0 to
> +             * be always 0, and this algorithm will grant that by default.
> +             */
> +            uint8_t distance = numa_info[src].distance[dst];
> +            uint8_t n_level = spapr_numa_get_numa_level(distance);
> +            uint32_t assoc_src;
> +
> +            /*
> +             * n_level = 0 means that the distance is greater than our last
> +             * rounded value (120). In this case there is no NUMA level match
> +             * between src and dst and we can skip the remaining of the loop.
> +             *
> +             * The Linux kernel will assume that the distance between src and
> +             * dst, in this case of no match, is 10 (local distance) doubled
> +             * for each NUMA it didn't match. We have MAX_DISTANCE_REF_POINTS
> +             * levels (4), so this gives us 10*2*2*2*2 = 160.
> +             *
> +             * This logic can be seen in the Linux kernel source code, as of
> +             * v5.9, in arch/powerpc/mm/numa.c, function __node_distance().
> +             */
> +            if (n_level == 0) {
> +                continue;
> +            }
> +
> +            assoc_src = spapr->numa_assoc_array[src][n_level];
> +            spapr->numa_assoc_array[dst][n_level] = assoc_src;

I'm still not convinced that having the entry at n_level match, but
not those at "coarser"/"more distant" levels be different makes sense.

> +        }
> +    }
> +
> +}
> +
>  void spapr_numa_associativity_init(SpaprMachineState *spapr,
>                                     MachineState *machine)
>  {
>      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,
> @@ -56,6 +162,17 @@ void spapr_numa_associativity_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);
> +            }
> +        }
>      }
>  
>      /*
> @@ -85,7 +202,7 @@ void spapr_numa_associativity_init(SpaprMachineState 
> *spapr,
>       * 1 NUMA node) will not benefit from anything we're going to do
>       * after this point.
>       */
> -    if (spapr_machine_using_legacy_numa(spapr)) {
> +    if (using_legacy_numa) {
>          return;
>      }
>  
> @@ -95,6 +212,7 @@ void spapr_numa_associativity_init(SpaprMachineState 
> *spapr,
>          exit(EXIT_FAILURE);
>      }
>  
> +    spapr_numa_define_associativity_domains(spapr);
>  }
>  
>  void spapr_numa_write_associativity_dt(SpaprMachineState *spapr, void *fdt,

-- 
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

signature.asc
Description: PGP signature