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

[zhanghailiang]

Hi Igor,

Thanks for your reviewing...

> On Mon, 15 Sep 2014 20:29:38 +0800
> zhanghailiang <address@hidden> 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 <address@hidden>
> > ---
> >   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
>
>
> .