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