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

[Igor Mammedov <imammedo@redhat.com>]

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.


> 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?

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.

> 
> 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