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

[Daniel Henrique Barboza <danielhb413@gmail.com>]

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().


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