[Qemu-devel] [PATCH v2 0/6] virtio-balloon: assorted fixes

[Qemu-devel] [PATCH v2 0/6] virtio-balloon: assorted fixes

[Roman Kagan]

This patchset addresses a few problems discovered when analyzing aborts
of (an older version of) QEMU with backported commit
afd9096eb1882f23929f5b5c177898ed231bac66 "virtio: error out if guest
exceeds virtqueue size".  Those problems are present in master, too,
except that they don't trigger an abort and are thus not as easy to
notice.

Roman Kagan (6):
  virtio: assert on ->inuse underflow
  virtio-balloon: make stats virtqueue length 1
  virtio-balloon: don't restart stats timer in callback
  virtio-balloon: keep collecting stats on save/restore
  virtio-balloon: drop ->stats_vq_offset
  virtio-balloon: drop reset handler

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ladi Prosek <lprosek@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>

---
v1 -> v2:
 - added assert in patch 2
 - new patches 5, 6

 hw/virtio/virtio-balloon.c         | 62 ++++++++++++++++++--------------------
 hw/virtio/virtio.c                 |  3 +-
 include/hw/virtio/virtio-balloon.h |  2 +-
 3 files changed, 32 insertions(+), 35 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/6] virtio: assert on ->inuse underflow

[Roman Kagan]

Make sure that ->inuse counter on virtqueue never goes negative.

This complements commit afd9096eb1882f23929f5b5c177898ed231bac66,
"virtio: error out if guest exceeds virtqueue size", which, due to
signed ->inuse comparison against unsigned ->vring.num, manifested a bug
in virtio-balloon where virtqueue_push() was called before the matching
virtqueu_pop(). [That problem will be addressed in followup patches].

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ladi Prosek <lprosek@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 15ee3a7..7a57857 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -92,7 +92,7 @@ struct VirtQueue
 
     uint16_t queue_index;
 
-    int inuse;
+    unsigned int inuse;
 
     uint16_t vector;
     VirtIOHandleOutput handle_output;
@@ -290,6 +290,7 @@ void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
 void virtqueue_flush(VirtQueue *vq, unsigned int count)
 {
     uint16_t old, new;
+    assert(vq->inuse >= count);
     /* Make sure buffer is written before we update index. */
     smp_wmb();
     trace_virtqueue_flush(vq, count);
-- 
2.7.4

[Qemu-devel] [PATCH v2 2/6] virtio-balloon: make stats virtqueue length 1

[Roman Kagan]

The protocol for virtio-balloon stats virtqueue doesn't allow more than
one element in the virtqueue.

So, instead of trying to compensate for guest misbehavior if it sends
new data before the slot has been released by the host, just define the
stats virtqueue length to 1 initially and rely on the generic virtio
code to handle overflows.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ladi Prosek <lprosek@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
---
v1 > v2:
 - added assert in virtio_balloon_receive_stats

 hw/virtio/virtio-balloon.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 5af429a..fb8784e 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -262,12 +262,8 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
         goto out;
     }
 
-    if (s->stats_vq_elem != NULL) {
-        /* This should never happen if the driver follows the spec. */
-        virtqueue_push(vq, s->stats_vq_elem, 0);
-        virtio_notify(vdev, vq);
-        g_free(s->stats_vq_elem);
-    }
+    /* enforced by stats virtqueue depth being 1 */
+    assert(!s->stats_vq_elem);
 
     s->stats_vq_elem = elem;
 
@@ -443,7 +439,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
 
     s->ivq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
     s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
-    s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
+    s->svq = virtio_add_queue(vdev, 1, virtio_balloon_receive_stats);
 
     reset_stats(s);
 }
-- 
2.7.4

[Qemu-devel] [PATCH v2 3/6] virtio-balloon: don't restart stats timer in callback

[Roman Kagan]

There's no need to restart the stats timer in its callback.  If the
callback happens to run when there's nothing to do just do nothing and
return.

The timer is armed either in receive handler or initially when
periodic stats collection is enabled via QMP.

While at this, observe that the presence of ->stats_vq_elem is enough to
indicate there's work to do here, and drop the check for the stats
feature.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ladi Prosek <lprosek@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio-balloon.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index fb8784e..6d4c57c 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -68,12 +68,6 @@ static inline void reset_stats(VirtIOBalloon *dev)
     for (i = 0; i < VIRTIO_BALLOON_S_NR; dev->stats[i++] = -1);
 }
 
-static bool balloon_stats_supported(const VirtIOBalloon *s)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(s);
-    return virtio_vdev_has_feature(vdev, VIRTIO_BALLOON_F_STATS_VQ);
-}
-
 static bool balloon_stats_enabled(const VirtIOBalloon *s)
 {
     return s->stats_poll_interval > 0;
@@ -99,9 +93,10 @@ static void balloon_stats_poll_cb(void *opaque)
     VirtIOBalloon *s = opaque;
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
-    if (s->stats_vq_elem == NULL || !balloon_stats_supported(s)) {
-        /* re-schedule */
-        balloon_stats_change_timer(s, s->stats_poll_interval);
+    if (!s->stats_vq_elem) {
+        /* The guest hasn't sent the stats yet (either not enabled or we came
+         * too early), nothing to do.  Once the guest starts sending stats the
+         * timer will get armed in receive handler */
         return;
     }
 
-- 
2.7.4

[Qemu-devel] [PATCH v2 4/6] virtio-balloon: keep collecting stats on save/restore

[Roman Kagan]

Upon save/restore virtio-balloon stats acquisition stops.  The reason is
that the fact that the (only) virtqueue element is being used by QEMU is
not recorded anywhere on save, so upon restore it's not released to the
guest, making further progress impossible.

Saving the information about the used element would introduce unjustified
vmstate incompatibility.

So instead just make sure the element is pushed before save, leaving the
ball on the guest side.  For that, add vm state change handler to
virtio-ballon which would take care of pushing the element if there is
one.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ladi Prosek <lprosek@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio-balloon.c         | 27 ++++++++++++++++++++++-----
 include/hw/virtio/virtio-balloon.h |  1 +
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 6d4c57c..f00ad8e 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -88,10 +88,19 @@ static void balloon_stats_change_timer(VirtIOBalloon *s, int64_t secs)
     timer_mod(s->stats_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + secs * 1000);
 }
 
+static void balloon_stats_push_elem(VirtIOBalloon *s)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
+    virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
+    virtio_notify(vdev, s->svq);
+    g_free(s->stats_vq_elem);
+    s->stats_vq_elem = NULL;
+}
+
 static void balloon_stats_poll_cb(void *opaque)
 {
     VirtIOBalloon *s = opaque;
-    VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
     if (!s->stats_vq_elem) {
         /* The guest hasn't sent the stats yet (either not enabled or we came
@@ -100,10 +109,7 @@ static void balloon_stats_poll_cb(void *opaque)
         return;
     }
 
-    virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
-    virtio_notify(vdev, s->svq);
-    g_free(s->stats_vq_elem);
-    s->stats_vq_elem = NULL;
+    balloon_stats_push_elem(s);
 }
 
 static void balloon_stats_get_all(Object *obj, Visitor *v, const char *name,
@@ -414,6 +420,15 @@ static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
     return 0;
 }
 
+static void balloon_vm_state_change(void *opaque, int running, RunState state)
+{
+    VirtIOBalloon *s = opaque;
+
+    if (!running && s->stats_vq_elem) {
+        balloon_stats_push_elem(s);
+    }
+}
+
 static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
@@ -436,6 +451,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
     s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
     s->svq = virtio_add_queue(vdev, 1, virtio_balloon_receive_stats);
 
+    s->change = qemu_add_vm_change_state_handler(balloon_vm_state_change, s);
     reset_stats(s);
 }
 
@@ -444,6 +460,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VirtIOBalloon *s = VIRTIO_BALLOON(dev);
 
+    qemu_del_vm_change_state_handler(s->change);
     balloon_stats_destroy_timer(s);
     qemu_remove_balloon_handler(s);
     virtio_cleanup(vdev);
diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
index 1ea13bd..d72ff7f 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -43,6 +43,7 @@ typedef struct VirtIOBalloon {
     int64_t stats_last_update;
     int64_t stats_poll_interval;
     uint32_t host_features;
+    VMChangeStateEntry *change;
 } VirtIOBalloon;
 
 #endif
-- 
2.7.4

[Qemu-devel] [PATCH v2 5/6] virtio-balloon: drop ->stats_vq_offset

[Roman Kagan]

As we don't write anything to guest on stats virtqueue we should pass
len=0 to virtqueue_push().  This makes ->stats_vq_offset unnecessary, so
drop it.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ladi Prosek <lprosek@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio-balloon.c         | 3 +--
 include/hw/virtio/virtio-balloon.h | 1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index f00ad8e..056ae49 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -92,7 +92,7 @@ static void balloon_stats_push_elem(VirtIOBalloon *s)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
-    virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
+    virtqueue_push(s->svq, s->stats_vq_elem, 0);
     virtio_notify(vdev, s->svq);
     g_free(s->stats_vq_elem);
     s->stats_vq_elem = NULL;
@@ -283,7 +283,6 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
         if (tag < VIRTIO_BALLOON_S_NR)
             s->stats[tag] = val;
     }
-    s->stats_vq_offset = offset;
 
     if (qemu_gettimeofday(&tv) < 0) {
         fprintf(stderr, "warning: %s: failed to get time of day\n", __func__);
diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
index d72ff7f..91b0138 100644
--- a/include/hw/virtio/virtio-balloon.h
+++ b/include/hw/virtio/virtio-balloon.h
@@ -38,7 +38,6 @@ typedef struct VirtIOBalloon {
     uint32_t actual;
     uint64_t stats[VIRTIO_BALLOON_S_NR];
     VirtQueueElement *stats_vq_elem;
-    size_t stats_vq_offset;
     QEMUTimer *stats_timer;
     int64_t stats_last_update;
     int64_t stats_poll_interval;
-- 
2.7.4

[Qemu-devel] [PATCH v2 6/6] virtio-ballon: drop reset handler

[Roman Kagan]

Since the release of ->stat_vq_elem is now ensured by the vm state
change handler, there's no need in the reset handler doing it.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ladi Prosek <lprosek@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio-balloon.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 056ae49..1557650 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -465,16 +465,6 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
     virtio_cleanup(vdev);
 }
 
-static void virtio_balloon_device_reset(VirtIODevice *vdev)
-{
-    VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
-
-    if (s->stats_vq_elem != NULL) {
-        g_free(s->stats_vq_elem);
-        s->stats_vq_elem = NULL;
-    }
-}
-
 static void virtio_balloon_instance_init(Object *obj)
 {
     VirtIOBalloon *s = VIRTIO_BALLOON(obj);
@@ -506,7 +496,6 @@ static void virtio_balloon_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     vdc->realize = virtio_balloon_device_realize;
     vdc->unrealize = virtio_balloon_device_unrealize;
-    vdc->reset = virtio_balloon_device_reset;
     vdc->get_config = virtio_balloon_get_config;
     vdc->set_config = virtio_balloon_set_config;
     vdc->get_features = virtio_balloon_get_features;
-- 
2.7.4

[Qemu-devel] [PATCH v2 6/6] virtio-balloon: drop reset handler

[Roman Kagan]

Since the release of ->stat_vq_elem is now ensured by the vm state
change handler, there's no need in the reset handler doing it.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ladi Prosek <lprosek@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/virtio/virtio-balloon.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 056ae49..1557650 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -465,16 +465,6 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
     virtio_cleanup(vdev);
 }
 
-static void virtio_balloon_device_reset(VirtIODevice *vdev)
-{
-    VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
-
-    if (s->stats_vq_elem != NULL) {
-        g_free(s->stats_vq_elem);
-        s->stats_vq_elem = NULL;
-    }
-}
-
 static void virtio_balloon_instance_init(Object *obj)
 {
     VirtIOBalloon *s = VIRTIO_BALLOON(obj);
@@ -506,7 +496,6 @@ static void virtio_balloon_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     vdc->realize = virtio_balloon_device_realize;
     vdc->unrealize = virtio_balloon_device_unrealize;
-    vdc->reset = virtio_balloon_device_reset;
     vdc->get_config = virtio_balloon_get_config;
     vdc->set_config = virtio_balloon_set_config;
     vdc->get_features = virtio_balloon_get_features;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 6/6] virtio-ballon: drop reset handler

[Roman Kagan]

On Fri, Aug 19, 2016 at 04:39:25PM +0300, Roman Kagan wrote:
> Since the release of ->stat_vq_elem is now ensured by the vm state
> change handler, there's no need in the reset handler doing it.

Oops, sorry, this is a duplicate of the same message modulo typo in the
subject, please ignore.

Roman.

Re: [Qemu-devel] [PATCH v2 0/6] virtio-balloon: assorted fixes

[Roman Kagan]

On Fri, Aug 19, 2016 at 04:39:19PM +0300, Roman Kagan wrote:
> This patchset addresses a few problems discovered when analyzing aborts
> of (an older version of) QEMU with backported commit
> afd9096eb1882f23929f5b5c177898ed231bac66 "virtio: error out if guest
> exceeds virtqueue size".  Those problems are present in master, too,
> except that they don't trigger an abort and are thus not as easy to
> notice.

Please disregard; there have been better fixes posted on the list.

Roman.

Re: [Qemu-devel] [PATCH v2 2/6] virtio-balloon: make stats virtqueue length 1

[Michael S. Tsirkin]

On Fri, Aug 19, 2016 at 04:39:21PM +0300, Roman Kagan wrote:
> The protocol for virtio-balloon stats virtqueue doesn't allow more than
> one element in the virtqueue.
> 
> So, instead of trying to compensate for guest misbehavior if it sends
> new data before the slot has been released by the host, just define the
> stats virtqueue length to 1 initially and rely on the generic virtio
> code to handle overflows.
> 
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Ladi Prosek <lprosek@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>


This would need a bunch of compat handlers to
avoid changes for old machine types.

> ---
> v1 > v2:
>  - added assert in virtio_balloon_receive_stats
> 
>  hw/virtio/virtio-balloon.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 5af429a..fb8784e 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -262,12 +262,8 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
>          goto out;
>      }
>  
> -    if (s->stats_vq_elem != NULL) {
> -        /* This should never happen if the driver follows the spec. */
> -        virtqueue_push(vq, s->stats_vq_elem, 0);
> -        virtio_notify(vdev, vq);
> -        g_free(s->stats_vq_elem);
> -    }
> +    /* enforced by stats virtqueue depth being 1 */
> +    assert(!s->stats_vq_elem);
>  
>      s->stats_vq_elem = elem;
>  
> @@ -443,7 +439,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>  
>      s->ivq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
>      s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
> -    s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
> +    s->svq = virtio_add_queue(vdev, 1, virtio_balloon_receive_stats);
>  
>      reset_stats(s);
>  }
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH v2 1/6] virtio: assert on ->inuse underflow

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 827 bytes --]

On Fri, Aug 19, 2016 at 04:39:20PM +0300, Roman Kagan wrote:
> Make sure that ->inuse counter on virtqueue never goes negative.
> 
> This complements commit afd9096eb1882f23929f5b5c177898ed231bac66,
> "virtio: error out if guest exceeds virtqueue size", which, due to
> signed ->inuse comparison against unsigned ->vring.num, manifested a bug
> in virtio-balloon where virtqueue_push() was called before the matching
> virtqueu_pop(). [That problem will be addressed in followup patches].
> 
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Ladi Prosek <lprosek@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/virtio/virtio.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 1/6] virtio: assert on ->inuse underflow

[Michael S. Tsirkin]

On Tue, Aug 23, 2016 at 05:03:32PM -0400, Stefan Hajnoczi wrote:
> On Fri, Aug 19, 2016 at 04:39:20PM +0300, Roman Kagan wrote:
> > Make sure that ->inuse counter on virtqueue never goes negative.
> > 
> > This complements commit afd9096eb1882f23929f5b5c177898ed231bac66,
> > "virtio: error out if guest exceeds virtqueue size", which, due to
> > signed ->inuse comparison against unsigned ->vring.num, manifested a bug
> > in virtio-balloon where virtqueue_push() was called before the matching
> > virtqueu_pop(). [That problem will be addressed in followup patches].
> > 
> > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: Ladi Prosek <lprosek@redhat.com>
> > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  hw/virtio/virtio.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

I'm not merging any asserts before 2.7. Please resubmit when 2.7
is out.

Re: [Qemu-devel] [PATCH v2 4/6] virtio-balloon: keep collecting stats on save/restore

[Ladi Prosek]

On Fri, Aug 19, 2016 at 3:39 PM, Roman Kagan <rkagan@virtuozzo.com> wrote:
> Upon save/restore virtio-balloon stats acquisition stops.  The reason is
> that the fact that the (only) virtqueue element is being used by QEMU is
> not recorded anywhere on save, so upon restore it's not released to the
> guest, making further progress impossible.
>
> Saving the information about the used element would introduce unjustified
> vmstate incompatibility.
>
> So instead just make sure the element is pushed before save, leaving the
> ball on the guest side.  For that, add vm state change handler to
> virtio-ballon which would take care of pushing the element if there is
> one.
>
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Ladi Prosek <lprosek@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/virtio/virtio-balloon.c         | 27 ++++++++++++++++++++++-----
>  include/hw/virtio/virtio-balloon.h |  1 +
>  2 files changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 6d4c57c..f00ad8e 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -88,10 +88,19 @@ static void balloon_stats_change_timer(VirtIOBalloon *s, int64_t secs)
>      timer_mod(s->stats_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + secs * 1000);
>  }
>
> +static void balloon_stats_push_elem(VirtIOBalloon *s)
> +{
> +    VirtIODevice *vdev = VIRTIO_DEVICE(s);
> +
> +    virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
> +    virtio_notify(vdev, s->svq);
> +    g_free(s->stats_vq_elem);
> +    s->stats_vq_elem = NULL;
> +}
> +
>  static void balloon_stats_poll_cb(void *opaque)
>  {
>      VirtIOBalloon *s = opaque;
> -    VirtIODevice *vdev = VIRTIO_DEVICE(s);
>
>      if (!s->stats_vq_elem) {
>          /* The guest hasn't sent the stats yet (either not enabled or we came
> @@ -100,10 +109,7 @@ static void balloon_stats_poll_cb(void *opaque)
>          return;
>      }
>
> -    virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
> -    virtio_notify(vdev, s->svq);
> -    g_free(s->stats_vq_elem);
> -    s->stats_vq_elem = NULL;
> +    balloon_stats_push_elem(s);
>  }
>
>  static void balloon_stats_get_all(Object *obj, Visitor *v, const char *name,
> @@ -414,6 +420,15 @@ static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
>      return 0;
>  }
>
> +static void balloon_vm_state_change(void *opaque, int running, RunState state)
> +{
> +    VirtIOBalloon *s = opaque;
> +
> +    if (!running && s->stats_vq_elem) {
> +        balloon_stats_push_elem(s);
> +    }
> +}
> +
>  static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> @@ -436,6 +451,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>      s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
>      s->svq = virtio_add_queue(vdev, 1, virtio_balloon_receive_stats);
>
> +    s->change = qemu_add_vm_change_state_handler(balloon_vm_state_change, s);
>      reset_stats(s);
>  }
>
> @@ -444,6 +460,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      VirtIOBalloon *s = VIRTIO_BALLOON(dev);
>
> +    qemu_del_vm_change_state_handler(s->change);
>      balloon_stats_destroy_timer(s);
>      qemu_remove_balloon_handler(s);
>      virtio_cleanup(vdev);
> diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
> index 1ea13bd..d72ff7f 100644
> --- a/include/hw/virtio/virtio-balloon.h
> +++ b/include/hw/virtio/virtio-balloon.h
> @@ -43,6 +43,7 @@ typedef struct VirtIOBalloon {
>      int64_t stats_last_update;
>      int64_t stats_poll_interval;
>      uint32_t host_features;
> +    VMChangeStateEntry *change;
>  } VirtIOBalloon;
>
>  #endif
> --
> 2.7.4
>

Hi Roman,

I talked to Michael Tsirkin and he agrees with merging this patch for
2.7. Could you please resubmit and use the set_status callback instead
of adding another VM state change handler?

static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
{
    VirtIOBalloon *s = VIRTIO_BALLOON(vdev);

    if (!vdev->vm_running && s->stats_vq_elem) {
        balloon_stats_push_elem(s);
    }
}

and

    vdc->set_status = virtio_balloon_set_status;
in virtio_balloon_class_init.

This is somewhat urgent because 2.7 will be out soon. If you're busy
or if I don't hear from you I'll post it on your behalf.

Thanks!
Ladi

Re: [Qemu-devel] [PATCH v2 4/6] virtio-balloon: keep collecting stats on save/restore

[Roman Kagan]

On Thu, Sep 01, 2016 at 10:35:49AM +0200, Ladi Prosek wrote:
> On Fri, Aug 19, 2016 at 3:39 PM, Roman Kagan <rkagan@virtuozzo.com> wrote:
> > Upon save/restore virtio-balloon stats acquisition stops.  The reason is
> > that the fact that the (only) virtqueue element is being used by QEMU is
> > not recorded anywhere on save, so upon restore it's not released to the
> > guest, making further progress impossible.
> >
> > Saving the information about the used element would introduce unjustified
> > vmstate incompatibility.
> >
> > So instead just make sure the element is pushed before save, leaving the
> > ball on the guest side.  For that, add vm state change handler to
> > virtio-ballon which would take care of pushing the element if there is
> > one.
> >
> > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: Ladi Prosek <lprosek@redhat.com>
> > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  hw/virtio/virtio-balloon.c         | 27 ++++++++++++++++++++++-----
> >  include/hw/virtio/virtio-balloon.h |  1 +
> >  2 files changed, 23 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > index 6d4c57c..f00ad8e 100644
> > --- a/hw/virtio/virtio-balloon.c
> > +++ b/hw/virtio/virtio-balloon.c
> > @@ -88,10 +88,19 @@ static void balloon_stats_change_timer(VirtIOBalloon *s, int64_t secs)
> >      timer_mod(s->stats_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + secs * 1000);
> >  }
> >
> > +static void balloon_stats_push_elem(VirtIOBalloon *s)
> > +{
> > +    VirtIODevice *vdev = VIRTIO_DEVICE(s);
> > +
> > +    virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
> > +    virtio_notify(vdev, s->svq);
> > +    g_free(s->stats_vq_elem);
> > +    s->stats_vq_elem = NULL;
> > +}
> > +
> >  static void balloon_stats_poll_cb(void *opaque)
> >  {
> >      VirtIOBalloon *s = opaque;
> > -    VirtIODevice *vdev = VIRTIO_DEVICE(s);
> >
> >      if (!s->stats_vq_elem) {
> >          /* The guest hasn't sent the stats yet (either not enabled or we came
> > @@ -100,10 +109,7 @@ static void balloon_stats_poll_cb(void *opaque)
> >          return;
> >      }
> >
> > -    virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
> > -    virtio_notify(vdev, s->svq);
> > -    g_free(s->stats_vq_elem);
> > -    s->stats_vq_elem = NULL;
> > +    balloon_stats_push_elem(s);
> >  }
> >
> >  static void balloon_stats_get_all(Object *obj, Visitor *v, const char *name,
> > @@ -414,6 +420,15 @@ static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
> >      return 0;
> >  }
> >
> > +static void balloon_vm_state_change(void *opaque, int running, RunState state)
> > +{
> > +    VirtIOBalloon *s = opaque;
> > +
> > +    if (!running && s->stats_vq_elem) {
> > +        balloon_stats_push_elem(s);
> > +    }
> > +}
> > +
> >  static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
> >  {
> >      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > @@ -436,6 +451,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
> >      s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
> >      s->svq = virtio_add_queue(vdev, 1, virtio_balloon_receive_stats);
> >
> > +    s->change = qemu_add_vm_change_state_handler(balloon_vm_state_change, s);
> >      reset_stats(s);
> >  }
> >
> > @@ -444,6 +460,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
> >      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >      VirtIOBalloon *s = VIRTIO_BALLOON(dev);
> >
> > +    qemu_del_vm_change_state_handler(s->change);
> >      balloon_stats_destroy_timer(s);
> >      qemu_remove_balloon_handler(s);
> >      virtio_cleanup(vdev);
> > diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
> > index 1ea13bd..d72ff7f 100644
> > --- a/include/hw/virtio/virtio-balloon.h
> > +++ b/include/hw/virtio/virtio-balloon.h
> > @@ -43,6 +43,7 @@ typedef struct VirtIOBalloon {
> >      int64_t stats_last_update;
> >      int64_t stats_poll_interval;
> >      uint32_t host_features;
> > +    VMChangeStateEntry *change;
> >  } VirtIOBalloon;
> >
> >  #endif
> > --
> > 2.7.4
> >
> 
> Hi Roman,
> 
> I talked to Michael Tsirkin and he agrees with merging this patch for
> 2.7.

I'm not happy with this patch: it tries to solve the problem on the
"save" side and therefore doesn't fix the bug when migrating from an
earlier QEMU version.

I wonder if we can do better and solve it on the "load" side.  (At first
I thought that your patch did that but on a closer look it turned out
not the case).

In particular, with Stefan's patch to restore VirtQueue->inuse, we
should be able to just rewind ->last_avail_idx by ->inuse during "load",
which AFAICS would also fix the bug.  What do you think?

> Could you please resubmit and use the set_status callback instead
> of adding another VM state change handler?
> 
> static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
> {
>     VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
> 
>     if (!vdev->vm_running && s->stats_vq_elem) {
>         balloon_stats_push_elem(s);
>     }
> }
> 
> and
> 
>     vdc->set_status = virtio_balloon_set_status;
> in virtio_balloon_class_init.

If the scheme I described above works out this won't be needed at all.

> This is somewhat urgent because 2.7 will be out soon. If you're busy
> or if I don't hear from you I'll post it on your behalf.

OK will focus on it now.

Thanks,
Roman.

Re: [Qemu-devel] [PATCH v2 4/6] virtio-balloon: keep collecting stats on save/restore

[Ladi Prosek]

On Thu, Sep 1, 2016 at 4:17 PM, Roman Kagan <rkagan@virtuozzo.com> wrote:
> On Thu, Sep 01, 2016 at 10:35:49AM +0200, Ladi Prosek wrote:
>> On Fri, Aug 19, 2016 at 3:39 PM, Roman Kagan <rkagan@virtuozzo.com> wrote:
>> > Upon save/restore virtio-balloon stats acquisition stops.  The reason is
>> > that the fact that the (only) virtqueue element is being used by QEMU is
>> > not recorded anywhere on save, so upon restore it's not released to the
>> > guest, making further progress impossible.
>> >
>> > Saving the information about the used element would introduce unjustified
>> > vmstate incompatibility.
>> >
>> > So instead just make sure the element is pushed before save, leaving the
>> > ball on the guest side.  For that, add vm state change handler to
>> > virtio-ballon which would take care of pushing the element if there is
>> > one.
>> >
>> > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
>> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> > Cc: Ladi Prosek <lprosek@redhat.com>
>> > Cc: Stefan Hajnoczi <stefanha@redhat.com>
>> > ---
>> >  hw/virtio/virtio-balloon.c         | 27 ++++++++++++++++++++++-----
>> >  include/hw/virtio/virtio-balloon.h |  1 +
>> >  2 files changed, 23 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
>> > index 6d4c57c..f00ad8e 100644
>> > --- a/hw/virtio/virtio-balloon.c
>> > +++ b/hw/virtio/virtio-balloon.c
>> > @@ -88,10 +88,19 @@ static void balloon_stats_change_timer(VirtIOBalloon *s, int64_t secs)
>> >      timer_mod(s->stats_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + secs * 1000);
>> >  }
>> >
>> > +static void balloon_stats_push_elem(VirtIOBalloon *s)
>> > +{
>> > +    VirtIODevice *vdev = VIRTIO_DEVICE(s);
>> > +
>> > +    virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
>> > +    virtio_notify(vdev, s->svq);
>> > +    g_free(s->stats_vq_elem);
>> > +    s->stats_vq_elem = NULL;
>> > +}
>> > +
>> >  static void balloon_stats_poll_cb(void *opaque)
>> >  {
>> >      VirtIOBalloon *s = opaque;
>> > -    VirtIODevice *vdev = VIRTIO_DEVICE(s);
>> >
>> >      if (!s->stats_vq_elem) {
>> >          /* The guest hasn't sent the stats yet (either not enabled or we came
>> > @@ -100,10 +109,7 @@ static void balloon_stats_poll_cb(void *opaque)
>> >          return;
>> >      }
>> >
>> > -    virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
>> > -    virtio_notify(vdev, s->svq);
>> > -    g_free(s->stats_vq_elem);
>> > -    s->stats_vq_elem = NULL;
>> > +    balloon_stats_push_elem(s);
>> >  }
>> >
>> >  static void balloon_stats_get_all(Object *obj, Visitor *v, const char *name,
>> > @@ -414,6 +420,15 @@ static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
>> >      return 0;
>> >  }
>> >
>> > +static void balloon_vm_state_change(void *opaque, int running, RunState state)
>> > +{
>> > +    VirtIOBalloon *s = opaque;
>> > +
>> > +    if (!running && s->stats_vq_elem) {
>> > +        balloon_stats_push_elem(s);
>> > +    }
>> > +}
>> > +
>> >  static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>> >  {
>> >      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>> > @@ -436,6 +451,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
>> >      s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
>> >      s->svq = virtio_add_queue(vdev, 1, virtio_balloon_receive_stats);
>> >
>> > +    s->change = qemu_add_vm_change_state_handler(balloon_vm_state_change, s);
>> >      reset_stats(s);
>> >  }
>> >
>> > @@ -444,6 +460,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
>> >      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>> >      VirtIOBalloon *s = VIRTIO_BALLOON(dev);
>> >
>> > +    qemu_del_vm_change_state_handler(s->change);
>> >      balloon_stats_destroy_timer(s);
>> >      qemu_remove_balloon_handler(s);
>> >      virtio_cleanup(vdev);
>> > diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
>> > index 1ea13bd..d72ff7f 100644
>> > --- a/include/hw/virtio/virtio-balloon.h
>> > +++ b/include/hw/virtio/virtio-balloon.h
>> > @@ -43,6 +43,7 @@ typedef struct VirtIOBalloon {
>> >      int64_t stats_last_update;
>> >      int64_t stats_poll_interval;
>> >      uint32_t host_features;
>> > +    VMChangeStateEntry *change;
>> >  } VirtIOBalloon;
>> >
>> >  #endif
>> > --
>> > 2.7.4
>> >
>>
>> Hi Roman,
>>
>> I talked to Michael Tsirkin and he agrees with merging this patch for
>> 2.7.
>
> I'm not happy with this patch: it tries to solve the problem on the
> "save" side and therefore doesn't fix the bug when migrating from an
> earlier QEMU version.
>
> I wonder if we can do better and solve it on the "load" side.  (At first
> I thought that your patch did that but on a closer look it turned out
> not the case).
>
> In particular, with Stefan's patch to restore VirtQueue->inuse, we
> should be able to just rewind ->last_avail_idx by ->inuse during "load",
> which AFAICS would also fix the bug.  What do you think?

Stefan was actually going to do exactly that in

https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg02455.html

but then dropped the patch in favor of this "save" side approach.
Going with Stefan's fix seems kind of unfortunate from a future-proof
perspective. It wouldn't be easy to revert it if we ever decide that
we want to leave something in the queue. But I understand that solving
the problem on the "load" side would be more helpful for real-world
deployments. Maybe we could do both but gate the load side hack with a
source QEMU version check? I wonder what Stefan thinks.

>> Could you please resubmit and use the set_status callback instead
>> of adding another VM state change handler?
>>
>> static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
>> {
>>     VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
>>
>>     if (!vdev->vm_running && s->stats_vq_elem) {
>>         balloon_stats_push_elem(s);
>>     }
>> }
>>
>> and
>>
>>     vdc->set_status = virtio_balloon_set_status;
>> in virtio_balloon_class_init.
>
> If the scheme I described above works out this won't be needed at all.
>
>> This is somewhat urgent because 2.7 will be out soon. If you're busy
>> or if I don't hear from you I'll post it on your behalf.
>
> OK will focus on it now.
>
> Thanks,
> Roman.

Re: [Qemu-devel] [PATCH v2 4/6] virtio-balloon: keep collecting stats on save/restore

[Michael S. Tsirkin]

On Thu, Sep 01, 2016 at 05:43:05PM +0200, Ladi Prosek wrote:
> On Thu, Sep 1, 2016 at 4:17 PM, Roman Kagan <rkagan@virtuozzo.com> wrote:
> > On Thu, Sep 01, 2016 at 10:35:49AM +0200, Ladi Prosek wrote:
> >> On Fri, Aug 19, 2016 at 3:39 PM, Roman Kagan <rkagan@virtuozzo.com> wrote:
> >> > Upon save/restore virtio-balloon stats acquisition stops.  The reason is
> >> > that the fact that the (only) virtqueue element is being used by QEMU is
> >> > not recorded anywhere on save, so upon restore it's not released to the
> >> > guest, making further progress impossible.
> >> >
> >> > Saving the information about the used element would introduce unjustified
> >> > vmstate incompatibility.
> >> >
> >> > So instead just make sure the element is pushed before save, leaving the
> >> > ball on the guest side.  For that, add vm state change handler to
> >> > virtio-ballon which would take care of pushing the element if there is
> >> > one.
> >> >
> >> > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> >> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> >> > Cc: Ladi Prosek <lprosek@redhat.com>
> >> > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> >> > ---
> >> >  hw/virtio/virtio-balloon.c         | 27 ++++++++++++++++++++++-----
> >> >  include/hw/virtio/virtio-balloon.h |  1 +
> >> >  2 files changed, 23 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> >> > index 6d4c57c..f00ad8e 100644
> >> > --- a/hw/virtio/virtio-balloon.c
> >> > +++ b/hw/virtio/virtio-balloon.c
> >> > @@ -88,10 +88,19 @@ static void balloon_stats_change_timer(VirtIOBalloon *s, int64_t secs)
> >> >      timer_mod(s->stats_timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + secs * 1000);
> >> >  }
> >> >
> >> > +static void balloon_stats_push_elem(VirtIOBalloon *s)
> >> > +{
> >> > +    VirtIODevice *vdev = VIRTIO_DEVICE(s);
> >> > +
> >> > +    virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
> >> > +    virtio_notify(vdev, s->svq);
> >> > +    g_free(s->stats_vq_elem);
> >> > +    s->stats_vq_elem = NULL;
> >> > +}
> >> > +
> >> >  static void balloon_stats_poll_cb(void *opaque)
> >> >  {
> >> >      VirtIOBalloon *s = opaque;
> >> > -    VirtIODevice *vdev = VIRTIO_DEVICE(s);
> >> >
> >> >      if (!s->stats_vq_elem) {
> >> >          /* The guest hasn't sent the stats yet (either not enabled or we came
> >> > @@ -100,10 +109,7 @@ static void balloon_stats_poll_cb(void *opaque)
> >> >          return;
> >> >      }
> >> >
> >> > -    virtqueue_push(s->svq, s->stats_vq_elem, s->stats_vq_offset);
> >> > -    virtio_notify(vdev, s->svq);
> >> > -    g_free(s->stats_vq_elem);
> >> > -    s->stats_vq_elem = NULL;
> >> > +    balloon_stats_push_elem(s);
> >> >  }
> >> >
> >> >  static void balloon_stats_get_all(Object *obj, Visitor *v, const char *name,
> >> > @@ -414,6 +420,15 @@ static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
> >> >      return 0;
> >> >  }
> >> >
> >> > +static void balloon_vm_state_change(void *opaque, int running, RunState state)
> >> > +{
> >> > +    VirtIOBalloon *s = opaque;
> >> > +
> >> > +    if (!running && s->stats_vq_elem) {
> >> > +        balloon_stats_push_elem(s);
> >> > +    }
> >> > +}
> >> > +
> >> >  static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
> >> >  {
> >> >      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >> > @@ -436,6 +451,7 @@ static void virtio_balloon_device_realize(DeviceState *dev, Error **errp)
> >> >      s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
> >> >      s->svq = virtio_add_queue(vdev, 1, virtio_balloon_receive_stats);
> >> >
> >> > +    s->change = qemu_add_vm_change_state_handler(balloon_vm_state_change, s);
> >> >      reset_stats(s);
> >> >  }
> >> >
> >> > @@ -444,6 +460,7 @@ static void virtio_balloon_device_unrealize(DeviceState *dev, Error **errp)
> >> >      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >> >      VirtIOBalloon *s = VIRTIO_BALLOON(dev);
> >> >
> >> > +    qemu_del_vm_change_state_handler(s->change);
> >> >      balloon_stats_destroy_timer(s);
> >> >      qemu_remove_balloon_handler(s);
> >> >      virtio_cleanup(vdev);
> >> > diff --git a/include/hw/virtio/virtio-balloon.h b/include/hw/virtio/virtio-balloon.h
> >> > index 1ea13bd..d72ff7f 100644
> >> > --- a/include/hw/virtio/virtio-balloon.h
> >> > +++ b/include/hw/virtio/virtio-balloon.h
> >> > @@ -43,6 +43,7 @@ typedef struct VirtIOBalloon {
> >> >      int64_t stats_last_update;
> >> >      int64_t stats_poll_interval;
> >> >      uint32_t host_features;
> >> > +    VMChangeStateEntry *change;
> >> >  } VirtIOBalloon;
> >> >
> >> >  #endif
> >> > --
> >> > 2.7.4
> >> >
> >>
> >> Hi Roman,
> >>
> >> I talked to Michael Tsirkin and he agrees with merging this patch for
> >> 2.7.
> >
> > I'm not happy with this patch: it tries to solve the problem on the
> > "save" side and therefore doesn't fix the bug when migrating from an
> > earlier QEMU version.
> >
> > I wonder if we can do better and solve it on the "load" side.  (At first
> > I thought that your patch did that but on a closer look it turned out
> > not the case).
> >
> > In particular, with Stefan's patch to restore VirtQueue->inuse, we
> > should be able to just rewind ->last_avail_idx by ->inuse during "load",
> > which AFAICS would also fix the bug.  What do you think?
> 
> Stefan was actually going to do exactly that in
> 
> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg02455.html
> 
> but then dropped the patch in favor of this "save" side approach.
> Going with Stefan's fix seems kind of unfortunate from a future-proof
> perspective. It wouldn't be easy to revert it if we ever decide that
> we want to leave something in the queue. But I understand that solving
> the problem on the "load" side would be more helpful for real-world
> deployments. Maybe we could do both but gate the load side hack with a
> source QEMU version check? I wonder what Stefan thinks.

It's a valid point. Try to resubmit Stefan's patches and we'll discuss?

> >> Could you please resubmit and use the set_status callback instead
> >> of adding another VM state change handler?
> >>
> >> static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
> >> {
> >>     VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
> >>
> >>     if (!vdev->vm_running && s->stats_vq_elem) {
> >>         balloon_stats_push_elem(s);
> >>     }
> >> }
> >>
> >> and
> >>
> >>     vdc->set_status = virtio_balloon_set_status;
> >> in virtio_balloon_class_init.
> >
> > If the scheme I described above works out this won't be needed at all.
> >
> >> This is somewhat urgent because 2.7 will be out soon. If you're busy
> >> or if I don't hear from you I'll post it on your behalf.
> >
> > OK will focus on it now.
> >
> > Thanks,
> > Roman.

Re: [Qemu-devel] [PATCH v2 4/6] virtio-balloon: keep collecting stats on save/restore

[Roman Kagan]

On Thu, Sep 01, 2016 at 05:43:05PM +0200, Ladi Prosek wrote:
> On Thu, Sep 1, 2016 at 4:17 PM, Roman Kagan <rkagan@virtuozzo.com> wrote:
> > I'm not happy with this patch: it tries to solve the problem on the
> > "save" side and therefore doesn't fix the bug when migrating from an
> > earlier QEMU version.
> >
> > I wonder if we can do better and solve it on the "load" side.  (At first
> > I thought that your patch did that but on a closer look it turned out
> > not the case).
> >
> > In particular, with Stefan's patch to restore VirtQueue->inuse, we
> > should be able to just rewind ->last_avail_idx by ->inuse during "load",
> > which AFAICS would also fix the bug.  What do you think?
> 
> Stefan was actually going to do exactly that in
> 
> https://lists.gnu.org/archive/html/qemu-devel/2016-08/msg02455.html

Hmm, obviously my mail search skills need to be improved :(  I already
coded a patch before I saw this.

> but then dropped the patch in favor of this "save" side approach.
> Going with Stefan's fix seems kind of unfortunate from a future-proof
> perspective. It wouldn't be easy to revert it if we ever decide that
> we want to leave something in the queue. But I understand that solving
> the problem on the "load" side would be more helpful for real-world
> deployments. Maybe we could do both but gate the load side hack with a
> source QEMU version check? I wonder what Stefan thinks.

I woudn't say it looked like a hack so I wouldn't bother with a version
check.  Anyway let's look at the code and decide.

> >> Could you please resubmit and use the set_status callback instead
> >> of adding another VM state change handler?
> >>
> >> static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
> >> {
> >>     VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
> >>
> >>     if (!vdev->vm_running && s->stats_vq_elem) {
> >>         balloon_stats_push_elem(s);
> >>     }
> >> }
> >>
> >> and
> >>
> >>     vdc->set_status = virtio_balloon_set_status;
> >> in virtio_balloon_class_init.
> >
> > If the scheme I described above works out this won't be needed at all.

Sure I was wrong here, I used it to kick the stats collection on load.

I'll post the patch in a minute.

Roman.