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

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
> > >
> > > 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 <address@hidden>
> > > Signed-off-by: Denis V. Lunev <address@hidden>
> > > CC: Stefan Hajnoczi <address@hidden>
> > > CC: Xiao Guangrong <address@hidden>
> > > CC: "Michael S. Tsirkin" <address@hidden>
> > > CC: Igor Mammedov <address@hidden>
> > > CC: Eric Blake <address@hidden>
> > > CC: Markus Armbruster <address@hidden>
> > > ---
> > > 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.