Re: [Qemu-devel] [PATCH] virtio-balloon: Fix ballooning not working correctly when hotplug memory

[zhanghailiang <zhang.zhanghailiang@huawei.com>]

Hi Igor,

Thanks for your reviewing...

> On Mon, 15 Sep 2014 20:29:38 +0800
> zhanghailiang <zhang.zhanghailiang@huawei.com> wrote:
>
>> When do memory balloon, it references the ram_size as the real ram size of VM,
>> But here ram_size is not include the hotplugged memory, and the result will
>> be confused.
>>
>> Steps to reproduce:
>> (1)Start VM: qemu -m size=1024,slots=4,maxmem=8G
>> (2)In VM: #free -m : 1024M
>> (3)qmp balloon 512M
>> (4)In VM: #free -m : 512M
>> (5)hotplug pc-dimm 1G
>> (6)In VM: #free -m : 1512M
>> (7)qmp balloon 256M
>> (8)In VM: #free -m :1256M
>>
>> Here we add a new global variable 'vm_ram_size', it will stat
> "qmp balloon" is not performance critical code and instead of a global
> variable, size could be calculated each time by enumerating present memory devices.
>

Good idea, in this way, we don't have to treat hotplug/unplug memory specially.
Will fix like that. Thanks.

>
>> the VM's real ram size which include configured ram and hotplugged ram.
>> virtio-balloon will reference this parameter.
> I know it's not supported yet but what will happen with balloonig
> if dimm device is removed without telling about it to balloon first?
>

Then, calculating the ram size will also be wrong and the result of balloon
will be confused too. 'size be calculated each time' you have suggested above will
solve this problem;)

> I'm not sure if balloon and native memory hotplug should be integrated.
> Native memory hotplug was intended as a replacement for ballooning
> without its drawbacks albeit guest OS memory unplug support is in
> its infancy stage yet.
>

IMHO memory hotplug/unplug can not replace the balloon completely,
Memory hotplug/unplug way may be a little stiff.

Example:
VM's memroy :1G
hotplugged one pc-dimm mem :1G

Now we want to limit the VM's ram size of guest OS to 1512M.
I don't know if we can unplugging a pc-dimm which has a different size to
its original size (hot add stage)? Here is 512M.
If it supports, what about limit ram size to 1100M?;) There seems to
be a minimal size limit for memory hotplug...(If i'm wrong, please let me know;))

Maybe in actual usage scenario we will use them together.
So i think we'd better to fix it.

Thanks,
zhanghailiang

>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>>   hw/i386/pc.c               |  1 +
>>   hw/virtio/virtio-balloon.c | 10 +++++-----
>>   include/exec/cpu-common.h  |  1 +
>>   vl.c                       |  3 +++
>>   4 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index b6c9b61..817810b 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -1606,6 +1606,7 @@ static void pc_dimm_plug(HotplugHandler *hotplug_dev,
>>       memory_region_add_subregion(&pcms->hotplug_memory,
>>                                   addr - pcms->hotplug_memory_base, mr);
>>       vmstate_register_ram(mr, dev);
>> +    vm_ram_size += memory_region_size(mr);
>>
>>       hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev);
>>       hhc->plug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err);
>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
>> index 2c30b3d..205e1fe 100644
>> --- a/hw/virtio/virtio-balloon.c
>> +++ b/hw/virtio/virtio-balloon.c
>> @@ -292,7 +292,7 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
>>       memcpy(&config, config_data, sizeof(struct virtio_balloon_config));
>>       dev->actual = le32_to_cpu(config.actual);
>>       if (dev->actual != oldactual) {
>> -        qapi_event_send_balloon_change(ram_size -
>> +        qapi_event_send_balloon_change(vm_ram_size -
>>                           ((ram_addr_t) dev->actual << VIRTIO_BALLOON_PFN_SHIFT),
>>                           &error_abort);
>>       }
>> @@ -307,7 +307,7 @@ static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f)
>>   static void virtio_balloon_stat(void *opaque, BalloonInfo *info)
>>   {
>>       VirtIOBalloon *dev = opaque;
>> -    info->actual = ram_size - ((uint64_t) dev->actual <<
>> +    info->actual = vm_ram_size - ((uint64_t) dev->actual <<
>>                                  VIRTIO_BALLOON_PFN_SHIFT);
>>   }
>>
>> @@ -316,11 +316,11 @@ static void virtio_balloon_to_target(void *opaque, ram_addr_t target)
>>       VirtIOBalloon *dev = VIRTIO_BALLOON(opaque);
>>       VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>>
>> -    if (target > ram_size) {
>> -        target = ram_size;
>> +    if (target > vm_ram_size) {
>> +        target = vm_ram_size;
>>       }
>>       if (target) {
>> -        dev->num_pages = (ram_size - target) >> VIRTIO_BALLOON_PFN_SHIFT;
>> +        dev->num_pages = (vm_ram_size - target) >> VIRTIO_BALLOON_PFN_SHIFT;
>>           virtio_notify_config(vdev);
>>       }
>>   }
>> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
>> index e3ec4c8..f55db6a 100644
>> --- a/include/exec/cpu-common.h
>> +++ b/include/exec/cpu-common.h
>> @@ -46,6 +46,7 @@ typedef uintptr_t ram_addr_t;
>>   #endif
>>
>>   extern ram_addr_t ram_size;
>> +extern ram_addr_t vm_ram_size;
>>
>>   /* memory API */
>>
>> diff --git a/vl.c b/vl.c
>> index 9c9acf5..5d20d0c 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -132,6 +132,7 @@ DisplayType display_type = DT_DEFAULT;
>>   static int display_remote;
>>   const char* keyboard_layout = NULL;
>>   ram_addr_t ram_size;
>> +ram_addr_t vm_ram_size; /* ram_size + hotplugged ram size */
>>   const char *mem_path = NULL;
>>   int mem_prealloc = 0; /* force preallocation of physical target memory */
>>   int nb_nics;
>> @@ -3015,6 +3016,7 @@ int main(int argc, char **argv, char **envp)
>>       machine_class = find_default_machine();
>>       cpu_model = NULL;
>>       ram_size = default_ram_size;
>> +    vm_ram_size = ram_size;
>>       snapshot = 0;
>>       cyls = heads = secs = 0;
>>       translation = BIOS_ATA_TRANSLATION_AUTO;
>> @@ -3388,6 +3390,7 @@ int main(int argc, char **argv, char **envp)
>>                               "'%s' option\n", slots_str ? "maxmem" : "slots");
>>                       exit(EXIT_FAILURE);
>>                   }
>> +                vm_ram_size = ram_size;
>>                   break;
>>               }
>>   #ifdef CONFIG_TPM
>
>
> .
>