Reply: [PATCH v2] virtio-balloon: optimize the virtio-balloon on the ARM platform

Reply: [PATCH v2] virtio-balloon: optimize the virtio-balloon on the ARM platform

[Yangming via]

> On 08.03.23 01:42, Michael S. Tsirkin wrote:
> > On Wed, Mar 01, 2023 at 06:38:13AM +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
> >> ---
> >> Refactor the code by adding comments and removing unnecessary code.
> >>
> >>   hw/mem/pc-dimm.c           |  7 +++++++
> >>   hw/virtio/virtio-balloon.c | 33 +++++----------------------------
> >>   include/hw/boards.h        |  2 ++
> >>   3 files changed, 14 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c index
> >> 50ef83215c..3f2734a267 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));
> >> +    /* count only "real" DIMMs, not NVDIMMs */
> >> +    if (!object_dynamic_cast(OBJECT(dimm), TYPE_NVDIMM)) {
> >> +        machine->device_memory->dimm_size += vmstate_mr->size;
> >> +    }
> >>   }
> >>
> >>   void pc_dimm_unplug(PCDIMMDevice *dimm, MachineState *machine)
> >
> > vmstate_mr->size is Int128 you are not supposed to do math on it.
> >
> > And generally poking at this struct is a bad idea.
> >
> > I think memory_region_size will do what you want but not 100% sure.
> > Maybe you need to look at the flatview ...
> 
> Good point, we should use memory_region_size().
> 
> --
> Thanks,
> 
> David / dhildenb

Thanks for the suggestion. The problem will be fixed in the upcoming third version by using 'memory_region_size()'.

By the way, we found that the size of the object is aligned with 'qemu_host_page_size' before allocating the memory (see details in 'qemu_ram_alloc_internal()' from softmmu/physmem.c). This means that the actual allocated memory may differ from the size defined in the Object. As a result, in 'get_current_ram_size()', the original method of counting hot-plugged memory from the Object may not be accurate. Now, we count the size from the memory region, which is a proper way to get the actual size of memory allocated.

Thanks,

XiQi

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

[David Hildenbrand]

On 09.03.23 07:14, Yangming wrote:
>> On 08.03.23 01:42, Michael S. Tsirkin wrote:
>>> On Wed, Mar 01, 2023 at 06:38:13AM +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
>>>> ---
>>>> Refactor the code by adding comments and removing unnecessary code.
>>>>
>>>>    hw/mem/pc-dimm.c           |  7 +++++++
>>>>    hw/virtio/virtio-balloon.c | 33 +++++----------------------------
>>>>    include/hw/boards.h        |  2 ++
>>>>    3 files changed, 14 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c index
>>>> 50ef83215c..3f2734a267 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));
>>>> +    /* count only "real" DIMMs, not NVDIMMs */
>>>> +    if (!object_dynamic_cast(OBJECT(dimm), TYPE_NVDIMM)) {
>>>> +        machine->device_memory->dimm_size += vmstate_mr->size;
>>>> +    }
>>>>    }
>>>>
>>>>    void pc_dimm_unplug(PCDIMMDevice *dimm, MachineState *machine)
>>>
>>> vmstate_mr->size is Int128 you are not supposed to do math on it.
>>>
>>> And generally poking at this struct is a bad idea.
>>>
>>> I think memory_region_size will do what you want but not 100% sure.
>>> Maybe you need to look at the flatview ...
>>
>> Good point, we should use memory_region_size().
>>
>> --
>> Thanks,
>>
>> David / dhildenb
> 
> Thanks for the suggestion. The problem will be fixed in the upcoming third version by using 'memory_region_size()'.
> 
> By the way, we found that the size of the object is aligned with 'qemu_host_page_size' before allocating the memory (see details in 'qemu_ram_alloc_internal()' from softmmu/physmem.c). This means that the actual allocated memory may differ from the size defined in the Object. As a result, in 'get_current_ram_size()', the original method of counting hot-plugged memory from the Object may not be accurate. Now, we count the size from the memory region, which is a proper way to get the actual size of memory allocated.

You'd have to have a memory backend size that is not aligned to the host 
page size.

qemu-system-x86_64 -nographic -nodefaults -object 
memory-backend-ram,id=tmp,size=1K

While that seems to be possible, it's something very odd to happen (I 
cannot think of a reasonable use case).



-- 
Thanks,

David / dhildenb

Reply: [PATCH v2] virtio-balloon: optimize the virtio-balloon on the ARM platform

[Yangming via]

> On 09.03.23 07:14, Yangming wrote:
> >> On 08.03.23 01:42, Michael S. Tsirkin wrote:
> >>> On Wed, Mar 01, 2023 at 06:38:13AM +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
> >>>> ---
> >>>> Refactor the code by adding comments and removing unnecessary
> code.
> >>>>
> >>>>    hw/mem/pc-dimm.c           |  7 +++++++
> >>>>    hw/virtio/virtio-balloon.c | 33 +++++----------------------------
> >>>>    include/hw/boards.h        |  2 ++
> >>>>    3 files changed, 14 insertions(+), 28 deletions(-)
> >>>>
> >>>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c index
> >>>> 50ef83215c..3f2734a267 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));
> >>>> +    /* count only "real" DIMMs, not NVDIMMs */
> >>>> +    if (!object_dynamic_cast(OBJECT(dimm), TYPE_NVDIMM)) {
> >>>> +        machine->device_memory->dimm_size += vmstate_mr->size;
> >>>> +    }
> >>>>    }
> >>>>
> >>>>    void pc_dimm_unplug(PCDIMMDevice *dimm, MachineState
> *machine)
> >>>
> >>> vmstate_mr->size is Int128 you are not supposed to do math on it.
> >>>
> >>> And generally poking at this struct is a bad idea.
> >>>
> >>> I think memory_region_size will do what you want but not 100% sure.
> >>> Maybe you need to look at the flatview ...
> >>
> >> Good point, we should use memory_region_size().
> >>
> >> --
> >> Thanks,
> >>
> >> David / dhildenb
> >
> > Thanks for the suggestion. The problem will be fixed in the upcoming third
> version by using 'memory_region_size()'.
> >
> > By the way, we found that the size of the object is aligned with
> 'qemu_host_page_size' before allocating the memory (see details in
> 'qemu_ram_alloc_internal()' from softmmu/physmem.c). This means that
> the actual allocated memory may differ from the size defined in the Object.
> As a result, in 'get_current_ram_size()', the original method of counting hot-
> plugged memory from the Object may not be accurate. Now, we count the
> size from the memory region, which is a proper way to get the actual size of
> memory allocated.
> 
> You'd have to have a memory backend size that is not aligned to the host
> page size.
> 
> qemu-system-x86_64 -nographic -nodefaults -object memory-backend-
> ram,id=tmp,size=1K
> 
> While that seems to be possible, it's something very odd to happen (I cannot
> think of a reasonable use case).
> 
> 
> 
> --
> Thanks,
> 
> David / dhildenb

Yes, I agree. Besides, a newer version[PATCH v3] has been sent. I am wondering could you please have a look at that？

Best
Qi