From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59670) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aNj0k-0000B4-S8 for qemu-devel@nongnu.org; Mon, 25 Jan 2016 10:25:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aNj0g-0008Mh-FH for qemu-devel@nongnu.org; Mon, 25 Jan 2016 10:24:58 -0500 Received: from mx2.parallels.com ([199.115.105.18]:58076) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aNj0g-0008MU-60 for qemu-devel@nongnu.org; Mon, 25 Jan 2016 10:24:54 -0500 Message-ID: <56A63E31.1080307@virtuozzo.com> Date: Mon, 25 Jan 2016 18:24:33 +0300 From: Vladimir Sementsov-Ogievskiy MIME-Version: 1.0 References: <1453117322-30191-1-git-send-email-den@openvz.org> <569DF99C.3040700@linux.intel.com> <569F712E.3030106@virtuozzo.com> In-Reply-To: <569F712E.3030106@virtuozzo.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/1] nvdimm: disable balloon List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Xiao Guangrong , "Denis V. Lunev" Cc: "Michael S. Tsirkin" , Markus Armbruster , qemu-devel@nongnu.org, Stefan Hajnoczi , Igor Mammedov On 20.01.2016 14:36, Vladimir Sementsov-Ogievskiy wrote: > On 19.01.2016 11:53, Xiao Guangrong wrote: >> >> >> On 01/18/2016 07:42 PM, Denis V. Lunev wrote: >>> From: Vladimir Sementsov-Ogievskiy >>> >>> NVDIMM for now is planned to use as a backing store for DAX filesystem >>> in the guest and thus this memory is excluded from guest memory >>> management >>> and LRUs. >>> >>> In this case libvirt running QEMU along with configured ballon almost >>> immediately inflates balloon and effectively kill the guest as >>> qemu counts nvdimm as part of the ram. >>> >> >> It looks good me. >> >> However, it is not related to this patch, why not use the 'total >> memory' reported >> by guest instead? It is more precise as a) BIOS and other components >> will occupy >> available memory and b) guest may limit the memory size it can use... > > What do you mean? fix virtio_balloon_set_config instead of > get_current_ram_size ? Or what? > >> >>> Counting dimm devices as part of the ram for ballooning was started >>> from >>> patch >>> virtio-balloon: Fix balloon not working correctly when hotplug memory >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy >>> Signed-off-by: Denis V. Lunev >>> CC: Stefan Hajnoczi >>> CC: Xiao Guangrong >>> CC: "Michael S. Tsirkin" >>> CC: Igor Mammedov >>> CC: Eric Blake >>> CC: Markus Armbruster >>> --- >>> The patch is submitted start a discussion. It may be technically >>> correct, >>> but for us the situation is a bit shady. >>> >>> hw/mem/nvdimm.c | 4 ++++ >>> hw/mem/pc-dimm.c | 7 ++++++- >>> include/hw/mem/pc-dimm.h | 1 + >>> qapi-schema.json | 5 ++++- >>> 4 files changed, 15 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c >>> index 4fd397f..4f4d29a 100644 >>> --- a/hw/mem/nvdimm.c >>> +++ b/hw/mem/nvdimm.c >>> @@ -27,9 +27,13 @@ >>> static void nvdimm_class_init(ObjectClass *oc, void *data) >>> { >>> DeviceClass *dc = DEVICE_CLASS(oc); >>> + PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc); >>> >>> /* nvdimm hotplug has not been supported yet. */ >>> dc->hotpluggable = false; >>> + >>> + /* ballooning is not supported */ >>> + ddc->in_ram = false; >>> } >>> >>> static TypeInfo nvdimm_info = { >>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c >>> index d5cdab2..e0f869d 100644 >>> --- a/hw/mem/pc-dimm.c >>> +++ b/hw/mem/pc-dimm.c >>> @@ -164,6 +164,7 @@ int qmp_pc_dimm_device_list(Object *obj, void >>> *opaque) >>> MemoryDeviceInfo *info = g_new0(MemoryDeviceInfo, 1); >>> PCDIMMDeviceInfo *di = g_new0(PCDIMMDeviceInfo, 1); >>> DeviceClass *dc = DEVICE_GET_CLASS(obj); >>> + PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj); >>> PCDIMMDevice *dimm = PC_DIMM(obj); >>> >>> if (dev->id) { >>> @@ -172,6 +173,7 @@ int qmp_pc_dimm_device_list(Object *obj, void >>> *opaque) >>> } >>> di->hotplugged = dev->hotplugged; >>> di->hotpluggable = dc->hotpluggable; >>> + di->in_ram = ddc->in_ram; >>> di->addr = dimm->addr; >>> di->slot = dimm->slot; >>> di->node = dimm->node; >>> @@ -205,7 +207,9 @@ ch(void) >>> if (value) { >>> switch (value->type) { >>> case MEMORY_DEVICE_INFO_KIND_DIMM: >>> - size += value->u.dimm->size; >>> + if (value->u.dimm->in_ram) { >>> + size += value->u.dimm->size; >>> + } >> >> Can we use "object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)" to filter out >> NVDIMM device? > > Hm, this will be ok too. and simpler.. If it ok for you, let's just > check it here. No, we cant, there is no access to dev here. only info. But, if not use qmp_pc_dimm_device_list, with it dev -> info transition, and write simple pc_dimm_device_list it would be possible. > >> >>> break; >>> default: >>> break; >>> @@ -444,6 +448,7 @@ static void pc_dimm_class_init(ObjectClass *oc, >>> void *data) >>> dc->props = pc_dimm_properties; >>> dc->desc = "DIMM memory module"; >>> >>> + ddc->in_ram = true; >>> ddc->get_memory_region = pc_dimm_get_memory_region; >>> } >>> >>> diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h >>> index d83bf30..3bcb505 100644 >>> --- a/include/hw/mem/pc-dimm.h >>> +++ b/include/hw/mem/pc-dimm.h >>> @@ -65,6 +65,7 @@ typedef struct PCDIMMDevice { >>> typedef struct PCDIMMDeviceClass { >>> /* private */ >>> DeviceClass parent_class; >>> + bool in_ram; >>> >>> /* public */ >>> MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm); >>> diff --git a/qapi-schema.json b/qapi-schema.json >>> index b3038b2..613b4d5 100644 >>> --- a/qapi-schema.json >>> +++ b/qapi-schema.json >>> @@ -3922,6 +3922,8 @@ >>> # >>> # @hotpluggable: true if device if could be added/removed while >>> machine is running >>> # >>> +# @in-ram: true if device if should be counted in current ram size >>> (since 2.6) >>> +# >>> # Since: 2.1 >>> ## >>> { 'struct': 'PCDIMMDeviceInfo', >>> @@ -3932,7 +3934,8 @@ >>> 'node': 'int', >>> 'memdev': 'str', >>> 'hotplugged': 'bool', >>> - 'hotpluggable': 'bool' >>> + 'hotpluggable': 'bool', >>> + 'in-ram': 'bool' >> >> What is it used for? > > only for check in ram_addr_t get_current_ram_size() > > -- Best regards, Vladimir * now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.