Re: [PATCH v2 3/7] spapr: introduce SpaprMachineClass::numa_assoc_array

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

On 9/2/20 10:51 PM, David Gibson wrote:
> On Tue, Sep 01, 2020 at 09:56:41AM -0300, Daniel Henrique Barboza wrote:
>> The next step to centralize all NUMA/associativity handling in
>> the spapr machine is to create a 'one stop place' for all
>> things ibm,associativity.
>>
>> This patch introduces numa_assoc_array, a 2 dimensional array
>> that will store all ibm,associativity arrays of all NUMA nodes.
>> This array is initialized in a new spapr_numa_associativity_init()
>> function, called in spapr_machine_init(). It is being initialized
>> with the same values used in other ibm,associativity properties
>> around spapr files (i.e. all zeros, last value is node_id).
>> The idea is to remove all hardcoded definitions and FDT writes
>> of ibm,associativity arrays, doing instead a call to the new
>> helper spapr_numa_write_associativity_dt() helper, that will
>> be able to write the DT with the correct values.
>>
>> We'll start small, handling the trivial cases first. The
>> remaining instances of ibm,associativity will be handled
>> next.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> 
> The idea is great, but there's one small but significant problem here:
> 
>> +void spapr_numa_associativity_init(MachineState *machine)
>> +{
>> +    SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
>> +    int nb_numa_nodes = machine->numa_state->num_nodes;
>> +    int i;
>> +
>> +    /*
>> +     * For all associativity arrays: first position is the size,
>> +     * position MAX_DISTANCE_REF_POINTS is always the numa_id,
>> +     * represented by the index 'i'.
>> +     *
>> +     * This will break on sparse NUMA setups, when/if QEMU starts
>> +     * to support it, because there will be no more guarantee that
>> +     * 'i' will be a valid node_id set by the user.
>> +     */
>> +    for (i = 0; i < nb_numa_nodes; i++) {
>> +        smc->numa_assoc_array[i][0] = cpu_to_be32(MAX_DISTANCE_REF_POINTS);
>> +        smc->numa_assoc_array[i][MAX_DISTANCE_REF_POINTS] = cpu_to_be32(i);
> 
> This initialization is called on a machine *instance*, which means it
> should treat the machine class as read-only.  i.e. the
> numa_assoc_array should be in the SpaprMachineState, rather than the
> class.
> 
> I mean, we'd get away with it in practice, since there's only ever
> likely to be a single machine instance, but still we should correct
> this.

Got it. I'll move it to SpaprMachineState. This will also spare a handful of lines
everywhere else since I was instantiating the class just to manipulate the matrix
(and now, in hindsight, I figured that this was a warning about the weirdness
of what I was doing).


Thanks,


DHB

>