Re: [PATCH 05/10] spapr: make ibm,max-associativity-domains scale with user input

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

On 8/19/20 11:55 PM, David Gibson wrote:
> On Fri, Aug 14, 2020 at 05:54:19PM -0300, Daniel Henrique Barboza wrote:
>> The ibm,max-associativity-domains is considering that only a single
>> associativity domain can exist in the same NUMA level. This is true
>> today because we do not support any type of NUMA distance user
>> customization, and all nodes are in the same distance to each other.
>>
>> To enhance NUMA distance support in the pSeries machine we need to
>> make this limit flexible. This patch rewrites the max-associativity
>> logic to consider that multiple associativity domains can co-exist
>> in the same NUMA level. We're using the legacy_numa() helper to
>> avoid leaking unneeded guest changes.
> 
> 
> Hrm.  I find the above a bit hard to understand.  Having the limit be
> one less than the number of nodes at every level except the last seems
> kind of odd to me.

I took a bit to reply on this because I was reconsidering this logic.

I tried to "not be greedy" with this maximum number and ended up doing
something that breaks in a simple scenario. Today, in a single conf with
a single NUMA node with a single CPU, and say 2 GPUs, given that all GPUs
are in their own associativity domains, we would have something like:

cpu0: 0 0 0 0 0 0
gpu1: gpu_1 gpu_1 gpu_1 gpu_1
gpu2: gpu_2 gpu_2 gpu_2 gpu_2

This would already break apart what I did there. I think we should simplify
and just set maxdomains to be all nodes in all levels, like we do today
but using spapr->gpu_numa_id as an alias to maxnodes.


Thanks,

DHB

> 
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   hw/ppc/spapr.c | 18 ++++++++++++++++--
>>   1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 073a59c47d..b0c4b80a23 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -919,13 +919,20 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
>>           cpu_to_be32(SPAPR_MEMORY_BLOCK_SIZE & 0xffffffff),
>>           cpu_to_be32(ms->smp.max_cpus / ms->smp.threads),
>>       };
>> -    uint32_t maxdomain = cpu_to_be32(spapr->extra_numa_nodes > 1 ? 1 : 0);
>> +
>> +    /* The maximum domains for a given NUMA level, supposing that every
>> +     * additional NUMA node belongs to the same domain (aside from the
>> +     * 4th level, where we must support all available NUMA domains), is
>> +     * total number of domains - 1. */
>> +    uint32_t total_nodes_number = ms->numa_state->num_nodes +
>> +                                  spapr->extra_numa_nodes;
>> +    uint32_t maxdomain = cpu_to_be32(total_nodes_number - 1);
>>       uint32_t maxdomains[] = {
>>           cpu_to_be32(4),
>>           maxdomain,
>>           maxdomain,
>>           maxdomain,
>> -        cpu_to_be32(ms->numa_state->num_nodes + spapr->extra_numa_nodes),
>> +        cpu_to_be32(total_nodes_number),
>>       };
>>   
>>       _FDT(rtas = fdt_add_subnode(fdt, 0, "rtas"));
>> @@ -962,6 +969,13 @@ static void spapr_dt_rtas(SpaprMachineState *spapr, void *fdt)
>>                        qemu_hypertas->str, qemu_hypertas->len));
>>       g_string_free(qemu_hypertas, TRUE);
>>   
>> +    if (spapr_machine_using_legacy_numa(spapr)) {
>> +        maxdomain = cpu_to_be32(spapr->extra_numa_nodes > 1 ? 1 : 0);
>> +        maxdomains[1] = maxdomain;
>> +        maxdomains[2] = maxdomain;
>> +        maxdomains[3] = maxdomain;
>> +    }
>> +
>>       if (smc->pre_5_1_assoc_refpoints) {
>>           nr_refpoints = 2;
>>       }
>