From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49654) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XVz6u-0002qx-Ow for qemu-devel@nongnu.org; Mon, 22 Sep 2014 04:36:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XVz6p-0006oH-ST for qemu-devel@nongnu.org; Mon, 22 Sep 2014 04:36:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:31993) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XVz6p-0006nm-KM for qemu-devel@nongnu.org; Mon, 22 Sep 2014 04:36:35 -0400 Date: Mon, 22 Sep 2014 10:14:53 +0200 From: Igor Mammedov Message-ID: <20140922101453.436309cd@nial.usersys.redhat.com> In-Reply-To: <541F9943.4040108@huawei.com> References: <1410784178-9012-1-git-send-email-zhang.zhanghailiang@huawei.com> <20140919150948.7ae4a8a1@nial.usersys.redhat.com> <541F9943.4040108@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] virtio-balloon: Fix ballooning not working correctly when hotplug memory List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: zhanghailiang Cc: peter.maydell@linaro.org, mst@redhat.com, qemu-devel@nongnu.org, luonengjun@huawei.com, peter.huangpeng@huawei.com, lcapitulino@redhat.com On Mon, 22 Sep 2014 11:36:35 +0800 zhanghailiang wrote: > Hi Igor, > > Thanks for your reviewing... > > > On Mon, 15 Sep 2014 20:29:38 +0800 > > zhanghailiang 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 > >> --- > >> 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 > > > > > > . > > > > >