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

[Igor Mammedov]

On Mon, 22 Sep 2014 11:36:35 +0800
zhanghailiang <address@hidden> wrote:

> 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.
Yep, mem hotplug is a coarse-grained method of control and requires some 
planning
when plugging/unplugging memory.

I'm not oposed to ballooning, just make sure that guests that have
a ballooning driver will still work as expected when memory plugged/unplugged.

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