[Qemu-devel] [RESEND PATCH v3 0/2] balloon: add a feature bit to let Guest OS deflate virtio_balloon on OOM

[Qemu-devel] [RESEND PATCH v3 0/2] balloon: add a feature bit to let Guest OS deflate virtio_balloon on OOM

[Denis V. Lunev]

Excessive virtio_balloon inflation can cause invocation of OOM-killer,
when Linux is under severe memory pressure. Various mechanisms are
responsible for correct virtio_balloon memory management. Nevertheless it
is often the case that these control tools does not have enough time to
react on fast changing memory load. As a result OS runs out of memory and
invokes OOM-killer. The balancing of memory by use of the virtio balloon
should not cause the termination of processes while there are pages in the
balloon. Now there is no way for virtio balloon driver to free memory at
the last moment before some process get killed by OOM-killer.

This does not provide a security breach as balloon itself is running
inside Guest OS and is working in the cooperation with the host. Thus
some improvements from Guest side should be considered as normal.

To solve the problem, introduce a virtio_balloon callback which is
expected to be called from the oom notifier call chain in out_of_memory()
function. If virtio balloon could release some memory, it will make the
system to return and retry the allocation that forced the out of memory
killer to run.

This behavior should be enabled if and only if appropriate feature bit
is set on the device. It is off by default.

This functionality was recently merged into vanilla Linux (actually in
linux-next at the moment)

  commit 5a10b7dbf904bfe01bb9fcc6298f7df09eed77d5
  Author: Raushaniya Maksudova <rmaksudova@parallels.com>
  Date:   Mon Nov 10 09:36:29 2014 +1030

This patch adds respective control bits into QEMU. It introduces
deflate-on-oom option for baloon device which do the trick.

Changes from v2:
- fixed mistake with bit number in virtio_balloon_get_features

Changes from v1:
- From: in patch 1 according to the original ownership
- feature processing in patch 2 as suggested by Michael. It could be done
  without additional field, but this will require to move the property
  level up, i.e. to PCI & CCW level.

Signed-off-by: Raushaniya Maksudova <rmaksudova@parallels.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Anthony Liguori <aliguori@amazon.com>
CC: Michael S. Tsirkin <mst@redhat.com>

[Qemu-devel] [RESEND PATCH 1/2] balloon: call qdev_alias_all_properties for proxy dev in balloon class init

[Denis V. Lunev]

The idea is that all other virtio devices are calling this helper
to merge properties of the proxy device. This is the only difference
in between this helper and code in inside virtio_instance_init_common.
The patch should not cause any harm as property list in generic balloon
code is empty.

This also allows to avoid some dummy errors like fixed by this
    commit 91ba21208839643603e7f7fa5864723c3f371ebe
    Author: Gonglei <arei.gonglei@huawei.com>
    Date:   Tue Sep 30 14:10:35 2014 +0800
    virtio-balloon: fix virtio-balloon child refcount in transports

Signed-off-by: Denis V. Lunev <den@openvz.org>
Signed-off-by: Raushaniya Maksudova <rmaksudova@parallels.com>
Revieved-by: Cornelia Huck <cornelia.huck@de.ibm.com>
CC: Christian Borntraeger <borntraeger@de.ibm.com>
CC: Anthony Liguori <aliguori@amazon.com>
CC: Michael S. Tsirkin <mst@redhat.com>
---
 hw/s390x/virtio-ccw.c  | 5 ++---
 hw/virtio/virtio-pci.c | 5 ++---
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index ea236c9..82da894 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -899,9 +899,8 @@ static void balloon_ccw_stats_set_poll_interval(Object *obj, struct Visitor *v,
 static void virtio_ccw_balloon_instance_init(Object *obj)
 {
     VirtIOBalloonCcw *dev = VIRTIO_BALLOON_CCW(obj);
-    object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BALLOON);
-    object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
-    object_unref(OBJECT(&dev->vdev));
+    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
+                                TYPE_VIRTIO_BALLOON);
     object_property_add(obj, "guest-stats", "guest statistics",
                         balloon_ccw_stats_get_all, NULL, NULL, dev, NULL);
 
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index dde1d73..745324b 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1316,9 +1316,8 @@ static void virtio_balloon_pci_class_init(ObjectClass *klass, void *data)
 static void virtio_balloon_pci_instance_init(Object *obj)
 {
     VirtIOBalloonPCI *dev = VIRTIO_BALLOON_PCI(obj);
-    object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BALLOON);
-    object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
-    object_unref(OBJECT(&dev->vdev));
+    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
+                                TYPE_VIRTIO_BALLOON);
     object_property_add(obj, "guest-stats", "guest statistics",
                         balloon_pci_stats_get_all, NULL, NULL, dev,
                         NULL);
-- 
1.9.1

[Qemu-devel] [RESEND PATCH 2/2] balloon: add a feature bit to let Guest OS deflate balloon on oom

[Denis V. Lunev]

Excessive virtio_balloon inflation can cause invocation of OOM-killer,
when Linux is under severe memory pressure. Various mechanisms are
responsible for correct virtio_balloon memory management. Nevertheless it
is often the case that these control tools does not have enough time to
react on fast changing memory load. As a result OS runs out of memory and
invokes OOM-killer. The balancing of memory by use of the virtio balloon
should not cause the termination of processes while there are pages in the
balloon. Now there is no way for virtio balloon driver to free memory at
the last moment before some process get killed by OOM-killer.

This does not provide a security breach as balloon itself is running
inside Guest OS and is working in the cooperation with the host. Thus
some improvements from Guest side should be considered as normal.

To solve the problem, introduce a virtio_balloon callback which is
expected to be called from the oom notifier call chain in out_of_memory()
function. If virtio balloon could release some memory, it will make the
system to return and retry the allocation that forced the out of memory
killer to run.

This behavior should be enabled if and only if appropriate feature bit
is set on the device. It is off by default.

This functionality was recently merged into vanilla Linux.

  commit 5a10b7dbf904bfe01bb9fcc6298f7df09eed77d5
  Author: Raushaniya Maksudova <rmaksudova@parallels.com>
  Date:   Mon Nov 10 09:36:29 2014 +1030

This patch adds respective control bits into QEMU. It introduces
deflate-on-oom option for baloon device which do the trick.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Raushaniya Maksudova <rmaksudova@parallels.com>
CC: Anthony Liguori <aliguori@amazon.com>
CC: Michael S. Tsirkin <mst@redhat.com>
---
 hw/virtio/virtio-balloon.c         | 6 ++++--
 include/hw/virtio/virtio-balloon.h | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 7bfbb75..a54d026 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -305,8 +305,8 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
 
 static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f)
 {
-    f |= (1 << VIRTIO_BALLOON_F_STATS_VQ);
-    return f;
+    VirtIOBalloon *dev = VIRTIO_BALLOON(vdev);
+    return f | (1u << VIRTIO_BALLOON_F_STATS_VQ) | dev->host_features;
 }
 
 static void virtio_balloon_stat(void *opaque, BalloonInfo *info)
@@ -409,6 +409,8 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
 }
 
 static Property virtio_balloon_properties[] = {
+    DEFINE_PROP_BIT("deflate-on-oom", VirtIOBalloon, host_features,
+                    VIRTIO_BALLOON_F_DEFLATE_ON_OOM, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
index f863bfe..2e1ccd9 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -30,6 +30,7 @@
 /* The feature bitmap for virtio balloon */
 #define VIRTIO_BALLOON_F_MUST_TELL_HOST 0 /* Tell before reclaiming pages */
 #define VIRTIO_BALLOON_F_STATS_VQ 1       /* Memory stats virtqueue */
+#define VIRTIO_BALLOON_F_DEFLATE_ON_OOM 2 /* Deflate balloon on OOM */
 
 /* Size of a PFN in the balloon interface. */
 #define VIRTIO_BALLOON_PFN_SHIFT 12
@@ -67,6 +68,7 @@ typedef struct VirtIOBalloon {
     QEMUTimer *stats_timer;
     int64_t stats_last_update;
     int64_t stats_poll_interval;
+    uint32_t host_features;
 } VirtIOBalloon;
 
 #endif
-- 
1.9.1

Re: [Qemu-devel] [RESEND PATCH v3 0/2] balloon: add a feature bit to let Guest OS deflate virtio_balloon on OOM

[Denis V. Lunev]

On 29/01/15 17:24, Denis V. Lunev wrote:
> Excessive virtio_balloon inflation can cause invocation of OOM-killer,
> when Linux is under severe memory pressure. Various mechanisms are
> responsible for correct virtio_balloon memory management. Nevertheless it
> is often the case that these control tools does not have enough time to
> react on fast changing memory load. As a result OS runs out of memory and
> invokes OOM-killer. The balancing of memory by use of the virtio balloon
> should not cause the termination of processes while there are pages in the
> balloon. Now there is no way for virtio balloon driver to free memory at
> the last moment before some process get killed by OOM-killer.
>
> This does not provide a security breach as balloon itself is running
> inside Guest OS and is working in the cooperation with the host. Thus
> some improvements from Guest side should be considered as normal.
>
> To solve the problem, introduce a virtio_balloon callback which is
> expected to be called from the oom notifier call chain in out_of_memory()
> function. If virtio balloon could release some memory, it will make the
> system to return and retry the allocation that forced the out of memory
> killer to run.
>
> This behavior should be enabled if and only if appropriate feature bit
> is set on the device. It is off by default.
>
> This functionality was recently merged into vanilla Linux (actually in
> linux-next at the moment)
>
>    commit 5a10b7dbf904bfe01bb9fcc6298f7df09eed77d5
>    Author: Raushaniya Maksudova <rmaksudova@parallels.com>
>    Date:   Mon Nov 10 09:36:29 2014 +1030
>
> This patch adds respective control bits into QEMU. It introduces
> deflate-on-oom option for baloon device which do the trick.
>
> Changes from v2:
> - fixed mistake with bit number in virtio_balloon_get_features
>
> Changes from v1:
> - From: in patch 1 according to the original ownership
> - feature processing in patch 2 as suggested by Michael. It could be done
>    without additional field, but this will require to move the property
>    level up, i.e. to PCI & CCW level.
>
> Signed-off-by: Raushaniya Maksudova <rmaksudova@parallels.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Anthony Liguori <aliguori@amazon.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
>
ping

any opinion on this? At least patch 1 should be committed.

Re: [Qemu-devel] [RESEND PATCH 1/2] balloon: call qdev_alias_all_properties for proxy dev in balloon class init

[Michael S. Tsirkin]

On Thu, Jan 29, 2015 at 05:24:41PM +0300, Denis V. Lunev wrote:
> The idea is that all other virtio devices are calling this helper
> to merge properties of the proxy device. This is the only difference
> in between this helper and code in inside virtio_instance_init_common.
> The patch should not cause any harm as property list in generic balloon
> code is empty.
> 
> This also allows to avoid some dummy errors like fixed by this
>     commit 91ba21208839643603e7f7fa5864723c3f371ebe
>     Author: Gonglei <arei.gonglei@huawei.com>
>     Date:   Tue Sep 30 14:10:35 2014 +0800
>     virtio-balloon: fix virtio-balloon child refcount in transports
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Signed-off-by: Raushaniya Maksudova <rmaksudova@parallels.com>
> Revieved-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> CC: Christian Borntraeger <borntraeger@de.ibm.com>
> CC: Anthony Liguori <aliguori@amazon.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/s390x/virtio-ccw.c  | 5 ++---
>  hw/virtio/virtio-pci.c | 5 ++---
>  2 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index ea236c9..82da894 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -899,9 +899,8 @@ static void balloon_ccw_stats_set_poll_interval(Object *obj, struct Visitor *v,
>  static void virtio_ccw_balloon_instance_init(Object *obj)
>  {
>      VirtIOBalloonCcw *dev = VIRTIO_BALLOON_CCW(obj);
> -    object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BALLOON);
> -    object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
> -    object_unref(OBJECT(&dev->vdev));
> +    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
> +                                TYPE_VIRTIO_BALLOON);
>      object_property_add(obj, "guest-stats", "guest statistics",
>                          balloon_ccw_stats_get_all, NULL, NULL, dev, NULL);
>  
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index dde1d73..745324b 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -1316,9 +1316,8 @@ static void virtio_balloon_pci_class_init(ObjectClass *klass, void *data)
>  static void virtio_balloon_pci_instance_init(Object *obj)
>  {
>      VirtIOBalloonPCI *dev = VIRTIO_BALLOON_PCI(obj);
> -    object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BALLOON);
> -    object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
> -    object_unref(OBJECT(&dev->vdev));
> +    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
> +                                TYPE_VIRTIO_BALLOON);
>      object_property_add(obj, "guest-stats", "guest statistics",
>                          balloon_pci_stats_get_all, NULL, NULL, dev,
>                          NULL);

OK, but what about this guest-stats property?
Should it get the same treatment?

> -- 
> 1.9.1

Re: [Qemu-devel] [RESEND PATCH 1/2] balloon: call qdev_alias_all_properties for proxy dev in balloon class init

[Denis V. Lunev]

On 19/02/15 12:25, Michael S. Tsirkin wrote:
> On Thu, Jan 29, 2015 at 05:24:41PM +0300, Denis V. Lunev wrote:
>> The idea is that all other virtio devices are calling this helper
>> to merge properties of the proxy device. This is the only difference
>> in between this helper and code in inside virtio_instance_init_common.
>> The patch should not cause any harm as property list in generic balloon
>> code is empty.
>>
>> This also allows to avoid some dummy errors like fixed by this
>>      commit 91ba21208839643603e7f7fa5864723c3f371ebe
>>      Author: Gonglei <arei.gonglei@huawei.com>
>>      Date:   Tue Sep 30 14:10:35 2014 +0800
>>      virtio-balloon: fix virtio-balloon child refcount in transports
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> Signed-off-by: Raushaniya Maksudova <rmaksudova@parallels.com>
>> Revieved-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>> CC: Christian Borntraeger <borntraeger@de.ibm.com>
>> CC: Anthony Liguori <aliguori@amazon.com>
>> CC: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>   hw/s390x/virtio-ccw.c  | 5 ++---
>>   hw/virtio/virtio-pci.c | 5 ++---
>>   2 files changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
>> index ea236c9..82da894 100644
>> --- a/hw/s390x/virtio-ccw.c
>> +++ b/hw/s390x/virtio-ccw.c
>> @@ -899,9 +899,8 @@ static void balloon_ccw_stats_set_poll_interval(Object *obj, struct Visitor *v,
>>   static void virtio_ccw_balloon_instance_init(Object *obj)
>>   {
>>       VirtIOBalloonCcw *dev = VIRTIO_BALLOON_CCW(obj);
>> -    object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BALLOON);
>> -    object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
>> -    object_unref(OBJECT(&dev->vdev));
>> +    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
>> +                                TYPE_VIRTIO_BALLOON);
>>       object_property_add(obj, "guest-stats", "guest statistics",
>>                           balloon_ccw_stats_get_all, NULL, NULL, dev, NULL);
>>   
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index dde1d73..745324b 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -1316,9 +1316,8 @@ static void virtio_balloon_pci_class_init(ObjectClass *klass, void *data)
>>   static void virtio_balloon_pci_instance_init(Object *obj)
>>   {
>>       VirtIOBalloonPCI *dev = VIRTIO_BALLOON_PCI(obj);
>> -    object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BALLOON);
>> -    object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
>> -    object_unref(OBJECT(&dev->vdev));
>> +    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
>> +                                TYPE_VIRTIO_BALLOON);
>>       object_property_add(obj, "guest-stats", "guest statistics",
>>                           balloon_pci_stats_get_all, NULL, NULL, dev,
>>                           NULL);
> OK, but what about this guest-stats property?
> Should it get the same treatment?
>
>> -- 
>> 1.9.1
hmm, IMHO no. init_common is actually do the following

void virtio_instance_init_common(Object *proxy_obj, void *data,
                                  size_t vdev_size, const char *vdev_name)
{
     DeviceState *vdev = data;

     object_initialize(vdev, vdev_size, vdev_name);
     object_property_add_child(proxy_obj, "virtio-backend", 
OBJECT(vdev), NULL);
     object_unref(OBJECT(vdev));
     qdev_alias_all_properties(vdev, proxy_obj);
}

on the other hand there is the following code in s390

static void s390_virtio_net_instance_init(Object *obj)
{
     VirtIONetS390 *dev = VIRTIO_NET_S390(obj);

     virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
                                 TYPE_VIRTIO_NET);
     object_property_add_alias(obj, "bootindex", OBJECT(&dev->vdev),
                               "bootindex", &error_abort);
}

which does not contain guest-stats property.

Re: [Qemu-devel] [RESEND PATCH 1/2] balloon: call qdev_alias_all_properties for proxy dev in balloon class init

[Michael S. Tsirkin]

On Thu, Feb 19, 2015 at 12:36:37PM +0300, Denis V. Lunev wrote:
> On 19/02/15 12:25, Michael S. Tsirkin wrote:
> >On Thu, Jan 29, 2015 at 05:24:41PM +0300, Denis V. Lunev wrote:
> >>The idea is that all other virtio devices are calling this helper
> >>to merge properties of the proxy device. This is the only difference
> >>in between this helper and code in inside virtio_instance_init_common.
> >>The patch should not cause any harm as property list in generic balloon
> >>code is empty.
> >>
> >>This also allows to avoid some dummy errors like fixed by this
> >>     commit 91ba21208839643603e7f7fa5864723c3f371ebe
> >>     Author: Gonglei <arei.gonglei@huawei.com>
> >>     Date:   Tue Sep 30 14:10:35 2014 +0800
> >>     virtio-balloon: fix virtio-balloon child refcount in transports
> >>
> >>Signed-off-by: Denis V. Lunev <den@openvz.org>
> >>Signed-off-by: Raushaniya Maksudova <rmaksudova@parallels.com>
> >>Revieved-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> >>CC: Christian Borntraeger <borntraeger@de.ibm.com>
> >>CC: Anthony Liguori <aliguori@amazon.com>
> >>CC: Michael S. Tsirkin <mst@redhat.com>
> >>---
> >>  hw/s390x/virtio-ccw.c  | 5 ++---
> >>  hw/virtio/virtio-pci.c | 5 ++---
> >>  2 files changed, 4 insertions(+), 6 deletions(-)
> >>
> >>diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> >>index ea236c9..82da894 100644
> >>--- a/hw/s390x/virtio-ccw.c
> >>+++ b/hw/s390x/virtio-ccw.c
> >>@@ -899,9 +899,8 @@ static void balloon_ccw_stats_set_poll_interval(Object *obj, struct Visitor *v,
> >>  static void virtio_ccw_balloon_instance_init(Object *obj)
> >>  {
> >>      VirtIOBalloonCcw *dev = VIRTIO_BALLOON_CCW(obj);
> >>-    object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BALLOON);
> >>-    object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
> >>-    object_unref(OBJECT(&dev->vdev));
> >>+    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
> >>+                                TYPE_VIRTIO_BALLOON);
> >>      object_property_add(obj, "guest-stats", "guest statistics",
> >>                          balloon_ccw_stats_get_all, NULL, NULL, dev, NULL);
> >>diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> >>index dde1d73..745324b 100644
> >>--- a/hw/virtio/virtio-pci.c
> >>+++ b/hw/virtio/virtio-pci.c
> >>@@ -1316,9 +1316,8 @@ static void virtio_balloon_pci_class_init(ObjectClass *klass, void *data)
> >>  static void virtio_balloon_pci_instance_init(Object *obj)
> >>  {
> >>      VirtIOBalloonPCI *dev = VIRTIO_BALLOON_PCI(obj);
> >>-    object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BALLOON);
> >>-    object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
> >>-    object_unref(OBJECT(&dev->vdev));
> >>+    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
> >>+                                TYPE_VIRTIO_BALLOON);
> >>      object_property_add(obj, "guest-stats", "guest statistics",
> >>                          balloon_pci_stats_get_all, NULL, NULL, dev,
> >>                          NULL);
> >OK, but what about this guest-stats property?
> >Should it get the same treatment?
> >
> >>-- 
> >>1.9.1
> hmm, IMHO no. init_common is actually do the following
> 
> void virtio_instance_init_common(Object *proxy_obj, void *data,
>                                  size_t vdev_size, const char *vdev_name)
> {
>     DeviceState *vdev = data;
> 
>     object_initialize(vdev, vdev_size, vdev_name);
>     object_property_add_child(proxy_obj, "virtio-backend", OBJECT(vdev),
> NULL);
>     object_unref(OBJECT(vdev));
>     qdev_alias_all_properties(vdev, proxy_obj);
> }
> 
> on the other hand there is the following code in s390
> 
> static void s390_virtio_net_instance_init(Object *obj)
> {
>     VirtIONetS390 *dev = VIRTIO_NET_S390(obj);
> 
>     virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
>                                 TYPE_VIRTIO_NET);
>     object_property_add_alias(obj, "bootindex", OBJECT(&dev->vdev),
>                               "bootindex", &error_abort);
> }
> 
> which does not contain guest-stats property.

But why doesn't it?
Seems like an obvious omission?

-- 
MST

Re: [Qemu-devel] [RESEND PATCH 1/2] balloon: call qdev_alias_all_properties for proxy dev in balloon class init

[Denis V. Lunev]

On 19/02/15 12:39, Michael S. Tsirkin wrote:
> On Thu, Feb 19, 2015 at 12:36:37PM +0300, Denis V. Lunev wrote:
>> On 19/02/15 12:25, Michael S. Tsirkin wrote:
>>> On Thu, Jan 29, 2015 at 05:24:41PM +0300, Denis V. Lunev wrote:
>>>> The idea is that all other virtio devices are calling this helper
>>>> to merge properties of the proxy device. This is the only difference
>>>> in between this helper and code in inside virtio_instance_init_common.
>>>> The patch should not cause any harm as property list in generic balloon
>>>> code is empty.
>>>>
>>>> This also allows to avoid some dummy errors like fixed by this
>>>>      commit 91ba21208839643603e7f7fa5864723c3f371ebe
>>>>      Author: Gonglei <arei.gonglei@huawei.com>
>>>>      Date:   Tue Sep 30 14:10:35 2014 +0800
>>>>      virtio-balloon: fix virtio-balloon child refcount in transports
>>>>
>>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>>> Signed-off-by: Raushaniya Maksudova <rmaksudova@parallels.com>
>>>> Revieved-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>>>> CC: Christian Borntraeger <borntraeger@de.ibm.com>
>>>> CC: Anthony Liguori <aliguori@amazon.com>
>>>> CC: Michael S. Tsirkin <mst@redhat.com>
>>>> ---
>>>>   hw/s390x/virtio-ccw.c  | 5 ++---
>>>>   hw/virtio/virtio-pci.c | 5 ++---
>>>>   2 files changed, 4 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
>>>> index ea236c9..82da894 100644
>>>> --- a/hw/s390x/virtio-ccw.c
>>>> +++ b/hw/s390x/virtio-ccw.c
>>>> @@ -899,9 +899,8 @@ static void balloon_ccw_stats_set_poll_interval(Object *obj, struct Visitor *v,
>>>>   static void virtio_ccw_balloon_instance_init(Object *obj)
>>>>   {
>>>>       VirtIOBalloonCcw *dev = VIRTIO_BALLOON_CCW(obj);
>>>> -    object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BALLOON);
>>>> -    object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
>>>> -    object_unref(OBJECT(&dev->vdev));
>>>> +    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
>>>> +                                TYPE_VIRTIO_BALLOON);
>>>>       object_property_add(obj, "guest-stats", "guest statistics",
>>>>                           balloon_ccw_stats_get_all, NULL, NULL, dev, NULL);
>>>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>>>> index dde1d73..745324b 100644
>>>> --- a/hw/virtio/virtio-pci.c
>>>> +++ b/hw/virtio/virtio-pci.c
>>>> @@ -1316,9 +1316,8 @@ static void virtio_balloon_pci_class_init(ObjectClass *klass, void *data)
>>>>   static void virtio_balloon_pci_instance_init(Object *obj)
>>>>   {
>>>>       VirtIOBalloonPCI *dev = VIRTIO_BALLOON_PCI(obj);
>>>> -    object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BALLOON);
>>>> -    object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
>>>> -    object_unref(OBJECT(&dev->vdev));
>>>> +    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
>>>> +                                TYPE_VIRTIO_BALLOON);
>>>>       object_property_add(obj, "guest-stats", "guest statistics",
>>>>                           balloon_pci_stats_get_all, NULL, NULL, dev,
>>>>                           NULL);
>>> OK, but what about this guest-stats property?
>>> Should it get the same treatment?
>>>
>>>> -- 
>>>> 1.9.1
>> hmm, IMHO no. init_common is actually do the following
>>
>> void virtio_instance_init_common(Object *proxy_obj, void *data,
>>                                   size_t vdev_size, const char *vdev_name)
>> {
>>      DeviceState *vdev = data;
>>
>>      object_initialize(vdev, vdev_size, vdev_name);
>>      object_property_add_child(proxy_obj, "virtio-backend", OBJECT(vdev),
>> NULL);
>>      object_unref(OBJECT(vdev));
>>      qdev_alias_all_properties(vdev, proxy_obj);
>> }
>>
>> on the other hand there is the following code in s390
>>
>> static void s390_virtio_net_instance_init(Object *obj)
>> {
>>      VirtIONetS390 *dev = VIRTIO_NET_S390(obj);
>>
>>      virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
>>                                  TYPE_VIRTIO_NET);
>>      object_property_add_alias(obj, "bootindex", OBJECT(&dev->vdev),
>>                                "bootindex", &error_abort);
>> }
>>
>> which does not contain guest-stats property.
> But why doesn't it?
> Seems like an obvious omission?
>
no it is not

cfind . | xargs fgrep "guest-stats"
./hw/virtio/virtio-pci.c: object_property_get(OBJECT(&dev->vdev), v, 
"guest-stats", errp);
./hw/virtio/virtio-pci.c: object_property_get(OBJECT(&dev->vdev), v, 
"guest-stats-polling-interval",
./hw/virtio/virtio-pci.c: object_property_set(OBJECT(&dev->vdev), v, 
"guest-stats-polling-interval",
./hw/virtio/virtio-pci.c:    object_property_add(obj, "guest-stats", 
"guest statistics",
./hw/virtio/virtio-pci.c:    object_property_add(obj, 
"guest-stats-polling-interval", "int",
./hw/virtio/virtio-balloon.c:    visit_start_struct(v, NULL, 
"guest-stats", name, 0, &err);
./hw/virtio/virtio-balloon.c:    object_property_add(OBJECT(dev), 
"guest-stats", "guest statistics",
./hw/virtio/virtio-balloon.c:    object_property_add(OBJECT(dev), 
"guest-stats-polling-interval", "int",
./hw/s390x/virtio-ccw.c: object_property_get(OBJECT(&dev->vdev), v, 
"guest-stats", errp);
./hw/s390x/virtio-ccw.c: object_property_get(OBJECT(&dev->vdev), v, 
"guest-stats-polling-interval",
./hw/s390x/virtio-ccw.c: object_property_set(OBJECT(&dev->vdev), v, 
"guest-stats-polling-interval",
./hw/s390x/virtio-ccw.c:    object_property_add(obj, "guest-stats", 
"guest statistics",
./hw/s390x/virtio-ccw.c:    object_property_add(obj, 
"guest-stats-polling-interval", "int",

looking into details this property is registered and defined for balloon 
only
and provides information about guest memory subsystem. May be the name
is toooo generic, but it is private to baloon code.

Thus no cure us needed at my opinion

Re: [Qemu-devel] [RESEND PATCH 1/2] balloon: call qdev_alias_all_properties for proxy dev in balloon class init

[Michael S. Tsirkin]

On Thu, Feb 19, 2015 at 12:46:34PM +0300, Denis V. Lunev wrote:
> On 19/02/15 12:39, Michael S. Tsirkin wrote:
> >On Thu, Feb 19, 2015 at 12:36:37PM +0300, Denis V. Lunev wrote:
> >>On 19/02/15 12:25, Michael S. Tsirkin wrote:
> >>>On Thu, Jan 29, 2015 at 05:24:41PM +0300, Denis V. Lunev wrote:
> >>>>The idea is that all other virtio devices are calling this helper
> >>>>to merge properties of the proxy device. This is the only difference
> >>>>in between this helper and code in inside virtio_instance_init_common.
> >>>>The patch should not cause any harm as property list in generic balloon
> >>>>code is empty.
> >>>>
> >>>>This also allows to avoid some dummy errors like fixed by this
> >>>>     commit 91ba21208839643603e7f7fa5864723c3f371ebe
> >>>>     Author: Gonglei <arei.gonglei@huawei.com>
> >>>>     Date:   Tue Sep 30 14:10:35 2014 +0800
> >>>>     virtio-balloon: fix virtio-balloon child refcount in transports
> >>>>
> >>>>Signed-off-by: Denis V. Lunev <den@openvz.org>
> >>>>Signed-off-by: Raushaniya Maksudova <rmaksudova@parallels.com>
> >>>>Revieved-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> >>>>CC: Christian Borntraeger <borntraeger@de.ibm.com>
> >>>>CC: Anthony Liguori <aliguori@amazon.com>
> >>>>CC: Michael S. Tsirkin <mst@redhat.com>
> >>>>---
> >>>>  hw/s390x/virtio-ccw.c  | 5 ++---
> >>>>  hw/virtio/virtio-pci.c | 5 ++---
> >>>>  2 files changed, 4 insertions(+), 6 deletions(-)
> >>>>
> >>>>diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> >>>>index ea236c9..82da894 100644
> >>>>--- a/hw/s390x/virtio-ccw.c
> >>>>+++ b/hw/s390x/virtio-ccw.c
> >>>>@@ -899,9 +899,8 @@ static void balloon_ccw_stats_set_poll_interval(Object *obj, struct Visitor *v,
> >>>>  static void virtio_ccw_balloon_instance_init(Object *obj)
> >>>>  {
> >>>>      VirtIOBalloonCcw *dev = VIRTIO_BALLOON_CCW(obj);
> >>>>-    object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BALLOON);
> >>>>-    object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
> >>>>-    object_unref(OBJECT(&dev->vdev));
> >>>>+    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
> >>>>+                                TYPE_VIRTIO_BALLOON);
> >>>>      object_property_add(obj, "guest-stats", "guest statistics",
> >>>>                          balloon_ccw_stats_get_all, NULL, NULL, dev, NULL);
> >>>>diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> >>>>index dde1d73..745324b 100644
> >>>>--- a/hw/virtio/virtio-pci.c
> >>>>+++ b/hw/virtio/virtio-pci.c
> >>>>@@ -1316,9 +1316,8 @@ static void virtio_balloon_pci_class_init(ObjectClass *klass, void *data)
> >>>>  static void virtio_balloon_pci_instance_init(Object *obj)
> >>>>  {
> >>>>      VirtIOBalloonPCI *dev = VIRTIO_BALLOON_PCI(obj);
> >>>>-    object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BALLOON);
> >>>>-    object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
> >>>>-    object_unref(OBJECT(&dev->vdev));
> >>>>+    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
> >>>>+                                TYPE_VIRTIO_BALLOON);
> >>>>      object_property_add(obj, "guest-stats", "guest statistics",
> >>>>                          balloon_pci_stats_get_all, NULL, NULL, dev,
> >>>>                          NULL);
> >>>OK, but what about this guest-stats property?
> >>>Should it get the same treatment?
> >>>
> >>>>-- 
> >>>>1.9.1
> >>hmm, IMHO no. init_common is actually do the following
> >>
> >>void virtio_instance_init_common(Object *proxy_obj, void *data,
> >>                                  size_t vdev_size, const char *vdev_name)
> >>{
> >>     DeviceState *vdev = data;
> >>
> >>     object_initialize(vdev, vdev_size, vdev_name);
> >>     object_property_add_child(proxy_obj, "virtio-backend", OBJECT(vdev),
> >>NULL);
> >>     object_unref(OBJECT(vdev));
> >>     qdev_alias_all_properties(vdev, proxy_obj);
> >>}
> >>
> >>on the other hand there is the following code in s390
> >>
> >>static void s390_virtio_net_instance_init(Object *obj)
> >>{
> >>     VirtIONetS390 *dev = VIRTIO_NET_S390(obj);
> >>
> >>     virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
> >>                                 TYPE_VIRTIO_NET);
> >>     object_property_add_alias(obj, "bootindex", OBJECT(&dev->vdev),
> >>                               "bootindex", &error_abort);
> >>}
> >>
> >>which does not contain guest-stats property.
> >But why doesn't it?
> >Seems like an obvious omission?
> >
> no it is not
> 
> cfind . | xargs fgrep "guest-stats"
> ./hw/virtio/virtio-pci.c: object_property_get(OBJECT(&dev->vdev), v,
> "guest-stats", errp);
> ./hw/virtio/virtio-pci.c: object_property_get(OBJECT(&dev->vdev), v,
> "guest-stats-polling-interval",
> ./hw/virtio/virtio-pci.c: object_property_set(OBJECT(&dev->vdev), v,
> "guest-stats-polling-interval",
> ./hw/virtio/virtio-pci.c:    object_property_add(obj, "guest-stats", "guest
> statistics",
> ./hw/virtio/virtio-pci.c:    object_property_add(obj,
> "guest-stats-polling-interval", "int",
> ./hw/virtio/virtio-balloon.c:    visit_start_struct(v, NULL, "guest-stats",
> name, 0, &err);
> ./hw/virtio/virtio-balloon.c:    object_property_add(OBJECT(dev),
> "guest-stats", "guest statistics",
> ./hw/virtio/virtio-balloon.c:    object_property_add(OBJECT(dev),
> "guest-stats-polling-interval", "int",
> ./hw/s390x/virtio-ccw.c: object_property_get(OBJECT(&dev->vdev), v,
> "guest-stats", errp);
> ./hw/s390x/virtio-ccw.c: object_property_get(OBJECT(&dev->vdev), v,
> "guest-stats-polling-interval",
> ./hw/s390x/virtio-ccw.c: object_property_set(OBJECT(&dev->vdev), v,
> "guest-stats-polling-interval",
> ./hw/s390x/virtio-ccw.c:    object_property_add(obj, "guest-stats", "guest
> statistics",
> ./hw/s390x/virtio-ccw.c:    object_property_add(obj,
> "guest-stats-polling-interval", "int",
> 
> looking into details this property is registered and defined for balloon
> only
> and provides information about guest memory subsystem. May be the name
> is toooo generic, but it is private to baloon code.
> 
> Thus no cure us needed at my opinion

The problem is code duplication: all transports need to know
about these balloon-specific property.
Why isn't it handled by virtio_instance_init_common?

-- 
MST

Re: [Qemu-devel] [RESEND PATCH 1/2] balloon: call qdev_alias_all_properties for proxy dev in balloon class init

[Denis V. Lunev]

On 19/02/15 13:17, Michael S. Tsirkin wrote:
> On Thu, Feb 19, 2015 at 12:46:34PM +0300, Denis V. Lunev wrote:
>> On 19/02/15 12:39, Michael S. Tsirkin wrote:
>>> On Thu, Feb 19, 2015 at 12:36:37PM +0300, Denis V. Lunev wrote:
>>>> On 19/02/15 12:25, Michael S. Tsirkin wrote:
>>>>> On Thu, Jan 29, 2015 at 05:24:41PM +0300, Denis V. Lunev wrote:
>>>>>> The idea is that all other virtio devices are calling this helper
>>>>>> to merge properties of the proxy device. This is the only difference
>>>>>> in between this helper and code in inside virtio_instance_init_common.
>>>>>> The patch should not cause any harm as property list in generic balloon
>>>>>> code is empty.
>>>>>>
>>>>>> This also allows to avoid some dummy errors like fixed by this
>>>>>>      commit 91ba21208839643603e7f7fa5864723c3f371ebe
>>>>>>      Author: Gonglei <arei.gonglei@huawei.com>
>>>>>>      Date:   Tue Sep 30 14:10:35 2014 +0800
>>>>>>      virtio-balloon: fix virtio-balloon child refcount in transports
>>>>>>
>>>>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>>>>> Signed-off-by: Raushaniya Maksudova <rmaksudova@parallels.com>
>>>>>> Revieved-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>>>>>> CC: Christian Borntraeger <borntraeger@de.ibm.com>
>>>>>> CC: Anthony Liguori <aliguori@amazon.com>
>>>>>> CC: Michael S. Tsirkin <mst@redhat.com>
>>>>>> ---
>>>>>>   hw/s390x/virtio-ccw.c  | 5 ++---
>>>>>>   hw/virtio/virtio-pci.c | 5 ++---
>>>>>>   2 files changed, 4 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
>>>>>> index ea236c9..82da894 100644
>>>>>> --- a/hw/s390x/virtio-ccw.c
>>>>>> +++ b/hw/s390x/virtio-ccw.c
>>>>>> @@ -899,9 +899,8 @@ static void balloon_ccw_stats_set_poll_interval(Object *obj, struct Visitor *v,
>>>>>>   static void virtio_ccw_balloon_instance_init(Object *obj)
>>>>>>   {
>>>>>>       VirtIOBalloonCcw *dev = VIRTIO_BALLOON_CCW(obj);
>>>>>> -    object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BALLOON);
>>>>>> -    object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
>>>>>> -    object_unref(OBJECT(&dev->vdev));
>>>>>> +    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
>>>>>> +                                TYPE_VIRTIO_BALLOON);
>>>>>>       object_property_add(obj, "guest-stats", "guest statistics",
>>>>>>                           balloon_ccw_stats_get_all, NULL, NULL, dev, NULL);
>>>>>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>>>>>> index dde1d73..745324b 100644
>>>>>> --- a/hw/virtio/virtio-pci.c
>>>>>> +++ b/hw/virtio/virtio-pci.c
>>>>>> @@ -1316,9 +1316,8 @@ static void virtio_balloon_pci_class_init(ObjectClass *klass, void *data)
>>>>>>   static void virtio_balloon_pci_instance_init(Object *obj)
>>>>>>   {
>>>>>>       VirtIOBalloonPCI *dev = VIRTIO_BALLOON_PCI(obj);
>>>>>> -    object_initialize(&dev->vdev, sizeof(dev->vdev), TYPE_VIRTIO_BALLOON);
>>>>>> -    object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
>>>>>> -    object_unref(OBJECT(&dev->vdev));
>>>>>> +    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
>>>>>> +                                TYPE_VIRTIO_BALLOON);
>>>>>>       object_property_add(obj, "guest-stats", "guest statistics",
>>>>>>                           balloon_pci_stats_get_all, NULL, NULL, dev,
>>>>>>                           NULL);
>>>>> OK, but what about this guest-stats property?
>>>>> Should it get the same treatment?
>>>>>
>>>>>> -- 
>>>>>> 1.9.1
>>>> hmm, IMHO no. init_common is actually do the following
>>>>
>>>> void virtio_instance_init_common(Object *proxy_obj, void *data,
>>>>                                   size_t vdev_size, const char *vdev_name)
>>>> {
>>>>      DeviceState *vdev = data;
>>>>
>>>>      object_initialize(vdev, vdev_size, vdev_name);
>>>>      object_property_add_child(proxy_obj, "virtio-backend", OBJECT(vdev),
>>>> NULL);
>>>>      object_unref(OBJECT(vdev));
>>>>      qdev_alias_all_properties(vdev, proxy_obj);
>>>> }
>>>>
>>>> on the other hand there is the following code in s390
>>>>
>>>> static void s390_virtio_net_instance_init(Object *obj)
>>>> {
>>>>      VirtIONetS390 *dev = VIRTIO_NET_S390(obj);
>>>>
>>>>      virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
>>>>                                  TYPE_VIRTIO_NET);
>>>>      object_property_add_alias(obj, "bootindex", OBJECT(&dev->vdev),
>>>>                                "bootindex", &error_abort);
>>>> }
>>>>
>>>> which does not contain guest-stats property.
>>> But why doesn't it?
>>> Seems like an obvious omission?
>>>
>> no it is not
>>
>> cfind . | xargs fgrep "guest-stats"
>> ./hw/virtio/virtio-pci.c: object_property_get(OBJECT(&dev->vdev), v,
>> "guest-stats", errp);
>> ./hw/virtio/virtio-pci.c: object_property_get(OBJECT(&dev->vdev), v,
>> "guest-stats-polling-interval",
>> ./hw/virtio/virtio-pci.c: object_property_set(OBJECT(&dev->vdev), v,
>> "guest-stats-polling-interval",
>> ./hw/virtio/virtio-pci.c:    object_property_add(obj, "guest-stats", "guest
>> statistics",
>> ./hw/virtio/virtio-pci.c:    object_property_add(obj,
>> "guest-stats-polling-interval", "int",
>> ./hw/virtio/virtio-balloon.c:    visit_start_struct(v, NULL, "guest-stats",
>> name, 0, &err);
>> ./hw/virtio/virtio-balloon.c:    object_property_add(OBJECT(dev),
>> "guest-stats", "guest statistics",
>> ./hw/virtio/virtio-balloon.c:    object_property_add(OBJECT(dev),
>> "guest-stats-polling-interval", "int",
>> ./hw/s390x/virtio-ccw.c: object_property_get(OBJECT(&dev->vdev), v,
>> "guest-stats", errp);
>> ./hw/s390x/virtio-ccw.c: object_property_get(OBJECT(&dev->vdev), v,
>> "guest-stats-polling-interval",
>> ./hw/s390x/virtio-ccw.c: object_property_set(OBJECT(&dev->vdev), v,
>> "guest-stats-polling-interval",
>> ./hw/s390x/virtio-ccw.c:    object_property_add(obj, "guest-stats", "guest
>> statistics",
>> ./hw/s390x/virtio-ccw.c:    object_property_add(obj,
>> "guest-stats-polling-interval", "int",
>>
>> looking into details this property is registered and defined for balloon
>> only
>> and provides information about guest memory subsystem. May be the name
>> is toooo generic, but it is private to baloon code.
>>
>> Thus no cure us needed at my opinion
> The problem is code duplication: all transports need to know
> about these balloon-specific property.
> Why isn't it handled by virtio_instance_init_common?
>
why it should?

virtio_instance_init_common is common for all virtio devices
including VirtIO net, VirtIO block, VirtIO SCSI. Thus the patch
move initialization of all common stuff into the common
code.

Initialization of virtio properties on the top of different transports
is a bit different problem which should be touched differently.
The same approach is used in the other devices.

We can invent here new helper but this is a matter of independent
changes set at my taste.

Re: [Qemu-devel] [RESEND PATCH 1/2] balloon: call qdev_alias_all_properties for proxy dev in balloon class init

[Michael S. Tsirkin]

On Thu, Feb 19, 2015 at 01:23:03PM +0300, Denis V. Lunev wrote:
> >The problem is code duplication: all transports need to know
> >about these balloon-specific property.
> >Why isn't it handled by virtio_instance_init_common?
> >
> why it should?
> 
> virtio_instance_init_common is common for all virtio devices
> including VirtIO net, VirtIO block, VirtIO SCSI. Thus the patch
> move initialization of all common stuff into the common
> code.

The problem seems common enough, virtio_instance_init_common
already works for all properties, why not for these ones?

Re: [Qemu-devel] [RESEND PATCH 1/2] balloon: call qdev_alias_all_properties for proxy dev in balloon class init

[Cornelia Huck]

On Thu, 19 Feb 2015 11:29:27 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Feb 19, 2015 at 01:23:03PM +0300, Denis V. Lunev wrote:
> > >The problem is code duplication: all transports need to know
> > >about these balloon-specific property.
> > >Why isn't it handled by virtio_instance_init_common?
> > >
> > why it should?
> > 
> > virtio_instance_init_common is common for all virtio devices
> > including VirtIO net, VirtIO block, VirtIO SCSI. Thus the patch
> > move initialization of all common stuff into the common
> > code.
> 
> The problem seems common enough, virtio_instance_init_common
> already works for all properties, why not for these ones?

It only works for the properties that are common amongst transports and
all devices.

Adding a virtio_balloon_init_common() that adds the guest_stats (or a
virtio_rng_init_common() that adds rng) is probably not a bad idea, but
I'd prefer it as patches on top of these.

Re: [Qemu-devel] [RESEND PATCH 1/2] balloon: call qdev_alias_all_properties for proxy dev in balloon class init

[Denis V. Lunev]

On 19/02/15 14:26, Cornelia Huck wrote:
> On Thu, 19 Feb 2015 11:29:27 +0100
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> On Thu, Feb 19, 2015 at 01:23:03PM +0300, Denis V. Lunev wrote:
>>>> The problem is code duplication: all transports need to know
>>>> about these balloon-specific property.
>>>> Why isn't it handled by virtio_instance_init_common?
>>>>
>>> why it should?
>>>
>>> virtio_instance_init_common is common for all virtio devices
>>> including VirtIO net, VirtIO block, VirtIO SCSI. Thus the patch
>>> move initialization of all common stuff into the common
>>> code.
>> The problem seems common enough, virtio_instance_init_common
>> already works for all properties, why not for these ones?
> It only works for the properties that are common amongst transports and
> all devices.
>
> Adding a virtio_balloon_init_common() that adds the guest_stats (or a
> virtio_rng_init_common() that adds rng) is probably not a bad idea, but
> I'd prefer it as patches on top of these.
>
ping. Michael, what is the decision?

Re: [Qemu-devel] [RESEND PATCH 1/2] balloon: call qdev_alias_all_properties for proxy dev in balloon class init

[Michael S. Tsirkin]

On Thu, Feb 26, 2015 at 08:01:08PM +0300, Denis V. Lunev wrote:
> On 19/02/15 14:26, Cornelia Huck wrote:
> >On Thu, 19 Feb 2015 11:29:27 +0100
> >"Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> >>On Thu, Feb 19, 2015 at 01:23:03PM +0300, Denis V. Lunev wrote:
> >>>>The problem is code duplication: all transports need to know
> >>>>about these balloon-specific property.
> >>>>Why isn't it handled by virtio_instance_init_common?
> >>>>
> >>>why it should?
> >>>
> >>>virtio_instance_init_common is common for all virtio devices
> >>>including VirtIO net, VirtIO block, VirtIO SCSI. Thus the patch
> >>>move initialization of all common stuff into the common
> >>>code.
> >>The problem seems common enough, virtio_instance_init_common
> >>already works for all properties, why not for these ones?
> >It only works for the properties that are common amongst transports and
> >all devices.
> >
> >Adding a virtio_balloon_init_common() that adds the guest_stats (or a
> >virtio_rng_init_common() that adds rng) is probably not a bad idea, but
> >I'd prefer it as patches on top of these.
> >
> ping. Michael, what is the decision?

Pls address this by sending patches on top.

-- 
MST