On Fri, 10 Sep 2021 16:55:35 -0300
Daniel Henrique Barboza <danielhb...@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 <danielhb...@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 <gr...@kaod.org>
> uint32_t legacy_refpoints[] = {
> cpu_to_be32(0x4),
> cpu_to_be32(0x4),
