Re: [Qemu-ppc] [Qemu-devel] [PATCH for 3.1] spapr: Fix ibm, max-associativity-domains property number of nodes

[Serhii Popovych]

Greg Kurz wrote:
> On Mon, 19 Nov 2018 14:48:34 +0100
> Laurent Vivier <address@hidden> wrote:
> 
>> On 19/11/2018 14:27, Greg Kurz wrote:
>>> On Mon, 19 Nov 2018 08:09:38 -0500
>>> Serhii Popovych <address@hidden> wrote:
>>>   
>>>> Laurent Vivier reported off by one with maximum number of NUMA nodes
>>>> provided by qemu-kvm being less by one than required according to
>>>> description of "ibm,max-associativity-domains" property in LoPAPR.
>>>>
>>>> It appears that I incorrectly treated LoPAPR description of this
>>>> property assuming it provides last valid domain (NUMA node here)
>>>> instead of maximum number of domains.
>>>>
>>>>   ### Before hot-add
>>>>
>>>>   (qemu) info numa
>>>>   3 nodes
>>>>   node 0 cpus: 0
>>>>   node 0 size: 0 MB
>>>>   node 0 plugged: 0 MB
>>>>   node 1 cpus:
>>>>   node 1 size: 1024 MB
>>>>   node 1 plugged: 0 MB
>>>>   node 2 cpus:
>>>>   node 2 size: 0 MB
>>>>   node 2 plugged: 0 MB
>>>>
>>>>   $ numactl -H
>>>>   available: 2 nodes (0-1)
>>>>   node 0 cpus: 0
>>>>   node 0 size: 0 MB
>>>>   node 0 free: 0 MB
>>>>   node 1 cpus:
>>>>   node 1 size: 999 MB
>>>>   node 1 free: 658 MB
>>>>   node distances:
>>>>   node   0   1
>>>>     0:  10  40
>>>>     1:  40  10
>>>>
>>>>   ### Hot-add
>>>>
>>>>   (qemu) object_add memory-backend-ram,id=mem0,size=1G
>>>>   (qemu) device_add pc-dimm,id=dimm1,memdev=mem0,node=2
>>>>   (qemu) [   87.704898] pseries-hotplug-mem: Attempting to hot-add 4 ...
>>>>   <there is no "Initmem setup node 2 [mem 0xHEX-0xHEX]">
>>>>   [   87.705128] lpar: Attempting to resize HPT to shift 21
>>>>   ... <HPT resize messages>
>>>>
>>>>   ### After hot-add
>>>>
>>>>   (qemu) info numa
>>>>   3 nodes
>>>>   node 0 cpus: 0
>>>>   node 0 size: 0 MB
>>>>   node 0 plugged: 0 MB
>>>>   node 1 cpus:
>>>>   node 1 size: 1024 MB
>>>>   node 1 plugged: 0 MB
>>>>   node 2 cpus:
>>>>   node 2 size: 1024 MB
>>>>   node 2 plugged: 1024 MB
>>>>
>>>>   $ numactl -H
>>>>   available: 2 nodes (0-1)
>>>>   ^^^^^^^^^^^^^^^^^^^^^^^^
>>>>              Still only two nodes (and memory hot-added to node 0 below)
>>>>   node 0 cpus: 0
>>>>   node 0 size: 1024 MB
>>>>   node 0 free: 1021 MB
>>>>   node 1 cpus:
>>>>   node 1 size: 999 MB
>>>>   node 1 free: 658 MB
>>>>   node distances:
>>>>   node   0   1
>>>>     0:  10  40
>>>>     1:  40  10
>>>>
>>>> After fix applied numactl(8) reports 3 nodes available and memory
>>>> plugged into node 2 as expected.
>>>>
>>>> Fixes: da9f80fbad21 ("spapr: Add ibm,max-associativity-domains property")
>>>> Reported-by: Laurent Vivier <address@hidden>
>>>> Signed-off-by: Serhii Popovych <address@hidden>
>>>> ---
>>>>  hw/ppc/spapr.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>> index 7afd1a1..843ae6c 100644
>>>> --- a/hw/ppc/spapr.c
>>>> +++ b/hw/ppc/spapr.c
>>>> @@ -1033,7 +1033,7 @@ static void spapr_dt_rtas(sPAPRMachineState *spapr, 
>>>> void *fdt)
>>>>          cpu_to_be32(0),
>>>>          cpu_to_be32(0),
>>>>          cpu_to_be32(0),
>>>> -        cpu_to_be32(nb_numa_nodes ? nb_numa_nodes - 1 : 0),
>>>> +        cpu_to_be32(nb_numa_nodes ? nb_numa_nodes : 0),  
>>>
>>> Maybe simply cpu_to_be32(nb_numa_nodes) ?  
>>
>> Or "cpu_to_be32(nb_numa_nodes ? nb_numa_nodes : 1)" ?

Linux handles zero correctly, but nb_numa_nodes ?: 1 looks better.

I did testing with just cpu_to_be32(nb_numa_nodes) and
cpu_to_be32(nb_numa_nodes ? nb_numa_nodes : 1) it works with Linux
correctly in both cases

(guest)# numactl -H
available: 1 nodes (0)
node 0 cpus: 0
node 0 size: 487 MB
node 0 free: 148 MB
node distances:
node   0
  0:  10

(qemu) info numa
0 nodes

>>
>> In spapr_populate_drconf_memory() we have this logic.
>>
> 
> Hmm... maybe you're right, it seems that the code assumes
> non-NUMA configs have at one node. Similar assumption is
> also present in pc_dimm_realize():
> 
>     if (((nb_numa_nodes > 0) && (dimm->node >= nb_numa_nodes)) ||
>         (!nb_numa_nodes && dimm->node)) 
According to this nb_numa_nodes can be zero

>         error_setg(errp, "'DIMM property " PC_DIMM_NODE_PROP " has value %"
>                    PRIu32 "' which exceeds the number of numa nodes: %d",
>                    dimm->node, nb_numa_nodes ? nb_numa_nodes : 1);
and this just handles this case to show proper error message.

>         return;
>     }

> 
> This is a bit confusing...
> 
>> Thanks,
>> Laurent
> 
> 


-- 
Thanks,
Serhii

signature.asc
Description: OpenPGP digital signature