Re: [PATCH v6 3/6] spapr: introduce spapr_numa_associativity_reset()

[Greg Kurz]

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

> Greg,
> 
> 
> On 9/14/21 16:58, Daniel Henrique Barboza wrote:
> > 
> > 
> > On 9/14/21 08:55, Greg Kurz wrote:
> >> On Fri, 10 Sep 2021 16:55:36 -0300
> >> Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
> >>
> 
> [...]
> 
> 
> >>>       }
> >>> @@ -167,6 +167,11 @@ static void 
> >>> spapr_numa_FORM1_affinity_init(SpaprMachineState *spapr,
> >>>       int nb_numa_nodes = machine->numa_state->num_nodes;
> >>>       int i, j, max_nodes_with_gpus;
> >>> +    /* init FORM1_assoc_array */
> >>> +    for (i = 0; i < MAX_NODES + NVGPU_MAX_NUM; i++) {
> >>> +        spapr->FORM1_assoc_array[i] = g_new0(uint32_t, NUMA_ASSOC_SIZE);
> >>
> >> Why dynamic allocation since you have fixed size ?
> > 
> > If I use static allocation the C compiler complains that I can't assign a
> > uint32_t** pointer to a uint32_t[MAX_NODES + NVGPU_MAX_NUM][NUMA_ASSOC_SIZE]
> > array type.
> > 
> > And given that the FORM2 array is a [MAX_NODES + NVGPU_MAX_NUM][2] array, 
> > the
> > way I worked around that here is to use dynamic allocation. Then C 
> > considers valid
> > to use numa_assoc_array as an uint32_t** pointer for both FORM1 and FORM2
> > 2D arrays. I'm fairly certain that there might be a way of doing static 
> > allocation
> > and keeping the uint32_t** pointer as is, but didn't find any. Tips welcome 
> > :D
> > 
> > An alternative that I considered, without the need for this dynamic 
> > allocation hack,
> > is to make both FORM1 and FORM2 data structures the same size (i.e.
> > an [MAX_NODES + NVGPU_MAX_NUM][NUMA_ASSOC_SIZE] uint32_t array) and then 
> > numa_assoc_array
> > can be a pointer of the same array type for both. Since we're controlling 
> > FORM1 and
> > FORM2 sizes separately inside the functions this would work. The downside 
> > is that
> > FORM2 2D array would be bigger than necessary.
> 
> I took a look and didn't find any neat way of doing a pointer switch
> between 2 static arrays without either allocating dynamic mem on the
> pointer and then g_memdup'ing it, or make all arrays the same size
> (i.e. numa_assoc_array would also be a statically allocated array
> with FORM1 size) and then memcpy() the chosen mode.
> 
> I just posted a new version in which I'm not relying on 'numa_assoc_array'.
> Instead, the DT functions will use a get_associativity() to retrieve
> the current associativity array based on CAS choice, in a similar
> manner like it is already done with the array sizes. This also allowed
> us to get rid of associativity_reset().
> 


Hi Daniel,

I've vaguely started at looking at the new version and it seems
to look better indeed. Now that KVM Forum 2021 and its too many
captivating talks are over :-) , I'll try to find some time to
review.

Cheers,

--
Greg

> 
> Thanks,
> 
> 
> Daniel
> 
> 
> 
> > 
> > 
> > I don't have strong opinions about which way to do it, so I'm all ears.
> > 
> > 
> > Thanks,
> > 
> > 
> > Daniel
> > 
> > 
> > 
> >>
> >>> +    }
> >>> +
> >>>       /*
> >>>        * For all associativity arrays: first position is the size,
> >>>        * position MAX_DISTANCE_REF_POINTS is always the numa_id,
> >>> @@ -177,8 +182,8 @@ static void 
> >>> spapr_numa_FORM1_affinity_init(SpaprMachineState *spapr,
> >>>        * 'i' will be a valid node_id set by the user.
> >>>        */
> >>>       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);
> >>> +        spapr->FORM1_assoc_array[i][0] = 
> >>> cpu_to_be32(MAX_DISTANCE_REF_POINTS);
> >>> +        spapr->FORM1_assoc_array[i][MAX_DISTANCE_REF_POINTS] = 
> >>> cpu_to_be32(i);
> >>>       }
> >>>       /*
> >>> @@ -192,15 +197,15 @@ static void 
> >>> spapr_numa_FORM1_affinity_init(SpaprMachineState *spapr,
> >>>       max_nodes_with_gpus = nb_numa_nodes + NVGPU_MAX_NUM;
> >>>       for (i = nb_numa_nodes; i < max_nodes_with_gpus; i++) {
> >>> -        spapr->numa_assoc_array[i][0] = 
> >>> cpu_to_be32(MAX_DISTANCE_REF_POINTS);
> >>> +        spapr->FORM1_assoc_array[i][0] = 
> >>> cpu_to_be32(MAX_DISTANCE_REF_POINTS);
> >>>           for (j = 1; j < MAX_DISTANCE_REF_POINTS; j++) {
> >>>               uint32_t gpu_assoc = smc->pre_5_1_assoc_refpoints ?
> >>>                                    SPAPR_GPU_NUMA_ID : cpu_to_be32(i);
> >>> -            spapr->numa_assoc_array[i][j] = gpu_assoc;
> >>> +            spapr->FORM1_assoc_array[i][j] = gpu_assoc;
> >>>           }
> >>> -        spapr->numa_assoc_array[i][MAX_DISTANCE_REF_POINTS] = 
> >>> cpu_to_be32(i);
> >>> +        spapr->FORM1_assoc_array[i][MAX_DISTANCE_REF_POINTS] = 
> >>> cpu_to_be32(i);
> >>>       }
> >>>       /*
> >>> @@ -227,14 +232,33 @@ void 
> >>> spapr_numa_associativity_init(SpaprMachineState *spapr,
> >>>                                      MachineState *machine)
> >>>   {
> >>>       spapr_numa_FORM1_affinity_init(spapr, machine);
> >>> +
> >>> +    /*
> >>> +     * Default to FORM1 affinity until CAS. We'll call affinity_reset()
> >>> +     * during CAS when we're sure about which NUMA affinity the guest
> >>> +     * is going to use.
> >>> +     *
> >>> +     * This step is a failsafe - guests in the wild were able to read
> >>> +     * FORM1 affinity info before CAS for a long time. Since 
> >>> affinity_reset()
> >>> +     * is just a pointer switch between data that was already populated
> >>> +     * here, this is an acceptable overhead to be on the safer side.
> >>> +     */
> >>> +    spapr->numa_assoc_array = spapr->FORM1_assoc_array;
> >>
> >> The right way to do that is to call spapr_numa_associativity_reset() from
> >> spapr_machine_reset() because we want to revert to FORM1 each time the
> >> guest is rebooted.
> >>
> >>> +}
> >>> +
> >>> +void spapr_numa_associativity_reset(SpaprMachineState *spapr)
> >>> +{
> >>> +    /* No FORM2 affinity implemented yet */
> >>
> >> This seems quite obvious at this point, not sure the comment helps.
> >>
> >>> +    spapr->numa_assoc_array = spapr->FORM1_assoc_array;
> >>>   }
> >>>   void spapr_numa_write_associativity_dt(SpaprMachineState *spapr, void 
> >>> *fdt,
> >>>                                          int offset, int nodeid)
> >>>   {
> >>> +    /* Hardcode the size of FORM1 associativity array for now */
> >>>       _FDT((fdt_setprop(fdt, offset, "ibm,associativity",
> >>>                         spapr->numa_assoc_array[nodeid],
> >>> -                      sizeof(spapr->numa_assoc_array[nodeid]))));
> >>> +                      NUMA_ASSOC_SIZE * sizeof(uint32_t))));
> >>
> >> Rather than doing this temporary change that gets undone in
> >> a later patch, I suggest you introduce get_numa_assoc_size()
> >> in a preliminary patch and use it here already :
> >>
> >> -                      NUMA_ASSOC_SIZE * sizeof(uint32_t))));
> >> +                      get_numa_assoc_size(spapr) * sizeof(uint32_t))));
> >>
> >> This will simplify the reviewing.
> >>
> >>>   }
> >>>   static uint32_t *spapr_numa_get_vcpu_assoc(SpaprMachineState *spapr,
> >>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >>> index 637652ad16..8a9490f0bf 100644
> >>> --- a/include/hw/ppc/spapr.h
> >>> +++ b/include/hw/ppc/spapr.h
> >>> @@ -249,7 +249,8 @@ struct SpaprMachineState {
> >>>       unsigned gpu_numa_id;
> >>>       SpaprTpmProxy *tpm_proxy;
> >>> -    uint32_t numa_assoc_array[MAX_NODES + 
> >>> NVGPU_MAX_NUM][NUMA_ASSOC_SIZE];
> >>> +    uint32_t *FORM1_assoc_array[MAX_NODES + NVGPU_MAX_NUM];
> >>
> >> As said above, I really don't see the point in not having :
> >>
> >>      uint32_t *FORM1_assoc_array[MAX_NODES + 
> >> NVGPU_MAX_NUM][NUMA_ASSOC_SIZE];
> >>
> >>> +    uint32_t **numa_assoc_array;
> >>>       Error *fwnmi_migration_blocker;
> >>>   };
> >>> diff --git a/include/hw/ppc/spapr_numa.h b/include/hw/ppc/spapr_numa.h
> >>> index 6f9f02d3de..ccf3e4eae8 100644
> >>> --- a/include/hw/ppc/spapr_numa.h
> >>> +++ b/include/hw/ppc/spapr_numa.h
> >>> @@ -24,6 +24,7 @@
> >>>    */
> >>>   void spapr_numa_associativity_init(SpaprMachineState *spapr,
> >>>                                      MachineState *machine);
> >>> +void spapr_numa_associativity_reset(SpaprMachineState *spapr);
> >>>   void spapr_numa_write_rtas_dt(SpaprMachineState *spapr, void *fdt, int 
> >>> rtas);
> >>>   void spapr_numa_write_associativity_dt(SpaprMachineState *spapr, void 
> >>> *fdt,
> >>>                                          int offset, int nodeid);
> >>