Re: [PATCH] virtio-balloon: optimize the virtio-balloon on the ARM platform.

[David Hildenbrand <david@redhat.com>]

On 28.02.23 09:56, Michael S. Tsirkin wrote:
> On Tue, Feb 28, 2023 at 03:26:56AM +0000, Yangming wrote:
>>>> Optimize the virtio-balloon feature on the ARM platform by adding a
>>>> variable to keep track of the current hot-plugged pc-dimm size,
>>>> instead of traversing the virtual machine's memory modules to count
>>>> the current RAM size during the balloon inflation or deflation
>>>> process. This variable can be updated only when plugging or unplugging
>>>> the device, which will result in an increase of approximately 60%
>>>> efficiency of balloon process on the ARM platform.
>>>>
>>>> We tested the total amount of time required for the balloon inflation
>>> process on ARM:
>>>> inflate the balloon to 64GB of a 128GB guest under stress.
>>>> Before: 102 seconds
>>>> After: 42 seconds
>>>>
>>>> Signed-off-by: Qi Xi <xiqi2@huawei.com>
>>>> Signed-off-by: Ming Yang yangming73@huawei.com
>>>> ---
>>>>    hw/mem/pc-dimm.c           |  2 ++
>>>>    hw/virtio/virtio-balloon.c | 33 +++++----------------------------
>>>>    include/hw/boards.h        |  1 +
>>>>    3 files changed, 8 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c index
>>>> 50ef83215c..192fc7922c 100644
>>>> --- a/hw/mem/pc-dimm.c
>>>> +++ b/hw/mem/pc-dimm.c
>>>> @@ -81,6 +81,7 @@ void pc_dimm_plug(PCDIMMDevice *dimm,
>>> MachineState
>>>> *machine)
>>>>
>>>>        memory_device_plug(MEMORY_DEVICE(dimm), machine);
>>>>        vmstate_register_ram(vmstate_mr, DEVICE(dimm));
>>>> +    machine->device_memory->dimm_size += vmstate_mr->size;
>>>>    }
>>>>
>>>>    void pc_dimm_unplug(PCDIMMDevice *dimm, MachineState *machine)
>>> @@
>>>> -90,6 +91,7 @@ void pc_dimm_unplug(PCDIMMDevice *dimm,
>>> MachineState
>>>> *machine)
>>>>
>>>>        memory_device_unplug(MEMORY_DEVICE(dimm), machine);
>>>>        vmstate_unregister_ram(vmstate_mr, DEVICE(dimm));
>>>> +    machine->device_memory->dimm_size -= vmstate_mr->size;
>>>>    }
>>>
>>> Ahh, missed that my previous comment was not addressed: we only want to
>>> track "real" DIMMs, not NVDIMMs.
>>>
>>> --
>>> Thanks,
>>>
>>> David / dhildenb
>>
>> Optimize the virtio-balloon feature on the ARM platform by adding
>> a variable to keep track of the current hot-plugged pc-dimm size,
>> instead of traversing the virtual machine's memory modules to count
>> the current RAM size during the balloon inflation or deflation
>> process. This variable can be updated only when plugging or unplugging
>> the device, which will result in an increase of approximately 60%
>> efficiency of balloon process on the ARM platform.
>>
>> We tested the total amount of time required for the balloon inflation process on ARM:
>> inflate the balloon to 64GB of a 128GB guest under stress.
>> Before: 102 seconds
>> After: 42 seconds
>>
>> Signed-off-by: Qi Xi <xiqi2@huawei.com>
>> Signed-off-by: Ming Yang yangming73@huawei.com
>> ---
>>   hw/mem/pc-dimm.c           |  8 ++++++++
>>   hw/virtio/virtio-balloon.c | 33 +++++----------------------------
>>   include/hw/boards.h        |  1 +
>>   3 files changed, 14 insertions(+), 28 deletions(-)
>>
>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
>> index 50ef83215c..2107615016 100644
>> --- a/hw/mem/pc-dimm.c
>> +++ b/hw/mem/pc-dimm.c
>> @@ -81,6 +81,10 @@ void pc_dimm_plug(PCDIMMDevice *dimm, MachineState *machine)
>>   
>>       memory_device_plug(MEMORY_DEVICE(dimm), machine);
>>       vmstate_register_ram(vmstate_mr, DEVICE(dimm));
>> +    bool is_nvdimm = object_dynamic_cast(OBJECT(dimm), TYPE_NVDIMM);
>> +    if (!is_nvdimm) {
>> +        machine->device_memory->dimm_size += vmstate_mr->size;
>> +    }
>>   }
>>   
>>   void pc_dimm_unplug(PCDIMMDevice *dimm, MachineState *machine)
>> @@ -90,6 +94,10 @@ void pc_dimm_unplug(PCDIMMDevice *dimm, MachineState *machine)
>>   
>>       memory_device_unplug(MEMORY_DEVICE(dimm), machine);
>>       vmstate_unregister_ram(vmstate_mr, DEVICE(dimm));
>> +    bool is_nvdimm = object_dynamic_cast(OBJECT(dimm), TYPE_NVDIMM);
>> +    if (!is_nvdimm) {
>> +        machine->device_memory->dimm_size -= vmstate_mr->size;
>> +    }
>>   }
>>
> 
> add comments here explaining why are nvdimms excluded?
>    

I really prefer to avoid mixing declaration and initialization where it 
an be avoided.

Further, the local variable can be easily avoided completely.

if (!object_dynamic_cast(OBJECT(dimm), TYPE_NVDIMM))

With that and with the other comments addressed

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb