Re: [Qemu-devel] [PATCH 1/1] nvdimm: disable balloon

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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 <vsementsov@virtuozzo.com>
>>>
>>> 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 <vsementsov@virtuozzo.com>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>> CC: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>>> CC: "Michael S. Tsirkin" <mst@redhat.com>
>>> CC: Igor Mammedov <imammedo@redhat.com>
>>> CC: Eric Blake <eblake@redhat.com>
>>> CC: Markus Armbruster <armbru@redhat.com>
>>> ---
>>> 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.