qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/2] virtio-balloon: resume collecting stats on vmload
@ 2016-09-01 18:13 Roman Kagan
  2016-09-01 18:13 ` [Qemu-devel] [PATCH 1/2] virtio: add virtqueue_rewind Roman Kagan
  2016-09-01 18:14 ` [Qemu-devel] [PATCH 2/2] virtio-balloon: resume collecting stats on vmload Roman Kagan
  0 siblings, 2 replies; 10+ messages in thread
From: Roman Kagan @ 2016-09-01 18:13 UTC (permalink / raw)
  To: qemu-devel; +Cc: Roman Kagan, Michael S. Tsirkin, Ladi Prosek, Stefan Hajnoczi

Fix virtio-balloon stats acquisition which stops upon save/restore.

Roman Kagan (2):
  virtio: add virtqueue_rewind
  virtio-balloon: resume collecting stats on vmload

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 | 21 ++++++++++++++++++++-
 hw/virtio/virtio.c         |  6 ++++++
 include/hw/virtio/virtio.h |  1 +
 3 files changed, 27 insertions(+), 1 deletion(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Qemu-devel] [PATCH 1/2] virtio: add virtqueue_rewind
  2016-09-01 18:13 [Qemu-devel] [PATCH 0/2] virtio-balloon: resume collecting stats on vmload Roman Kagan
@ 2016-09-01 18:13 ` Roman Kagan
  2016-09-01 18:14 ` [Qemu-devel] [PATCH 2/2] virtio-balloon: resume collecting stats on vmload Roman Kagan
  1 sibling, 0 replies; 10+ messages in thread
From: Roman Kagan @ 2016-09-01 18:13 UTC (permalink / raw)
  To: qemu-devel; +Cc: Roman Kagan, Michael S. Tsirkin, Ladi Prosek, Stefan Hajnoczi

If the in-use elements aren't migrated (like is the case with
virtio-balloon stats vq), make virtqueue forget about them and pretend
they haven't been popped yet, to allow to start over on load.

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         | 6 ++++++
 include/hw/virtio/virtio.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 74c085c..844d2a1 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -272,6 +272,12 @@ void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem,
     virtqueue_unmap_sg(vq, elem, len);
 }
 
+void virtqueue_rewind(VirtQueue *vq)
+{
+    vq->last_avail_idx -= vq->inuse;
+    vq->inuse = 0;
+}
+
 void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
                     unsigned int len, unsigned int idx)
 {
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index d2490c1..9f49fa4 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -154,6 +154,7 @@ void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
 void virtqueue_flush(VirtQueue *vq, unsigned int count);
 void virtqueue_discard(VirtQueue *vq, const VirtQueueElement *elem,
                        unsigned int len);
+void virtqueue_rewind(VirtQueue *vq);
 void virtqueue_fill(VirtQueue *vq, const VirtQueueElement *elem,
                     unsigned int len, unsigned int idx);
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [Qemu-devel] [PATCH 2/2] virtio-balloon: resume collecting stats on vmload
  2016-09-01 18:13 [Qemu-devel] [PATCH 0/2] virtio-balloon: resume collecting stats on vmload Roman Kagan
  2016-09-01 18:13 ` [Qemu-devel] [PATCH 1/2] virtio: add virtqueue_rewind Roman Kagan
@ 2016-09-01 18:14 ` Roman Kagan
  2016-09-01 19:26   ` Michael S. Tsirkin
  1 sibling, 1 reply; 10+ messages in thread
From: Roman Kagan @ 2016-09-01 18:14 UTC (permalink / raw)
  To: qemu-devel; +Cc: Roman Kagan, Michael S. Tsirkin, Ladi Prosek, Stefan Hajnoczi

Upon save/restore virtio-balloon stats acquisition stops.  The reason is
that the in-use virtqueue element is not saved, and 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.

However, the number of virtqueue in-use elements can be deduced from the
data available on restore (see bccdef6b1a204db0f41ffb6e24ce373e4d7890d4
"virtio: recalculate vq->inuse after migration").

So make the stats virtqueue forget that an element was popped from it,
and start over.  As this tackles the problem on the "load" side, it
is compatible with the state saved by earlier QEMU versions.

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 | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 5af429a..8660052 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -406,7 +406,13 @@ static void virtio_balloon_save_device(VirtIODevice *vdev, QEMUFile *f)
 
 static int virtio_balloon_load(QEMUFile *f, void *opaque, size_t size)
 {
-    return virtio_load(VIRTIO_DEVICE(opaque), f, 1);
+    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
+    VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
+    int ret = virtio_load(vdev, f, 1);
+    /* rewind needs vq->inuse populated which happens in virtio_load() after
+     * vdev->load */
+    virtqueue_rewind(s->svq);
+    return ret;
 }
 
 static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
@@ -468,6 +474,18 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev)
     }
 }
 
+static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
+{
+    VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
+
+    if (vdev->vm_running && balloon_stats_supported(s) &&
+        (status & VIRTIO_CONFIG_S_DRIVER_OK) && !s->stats_vq_elem) {
+        /* poll stats queue for the element we may have discarded when the VM
+         * was stopped */
+        virtio_balloon_receive_stats(vdev, s->svq);
+    }
+}
+
 static void virtio_balloon_instance_init(Object *obj)
 {
     VirtIOBalloon *s = VIRTIO_BALLOON(obj);
@@ -505,6 +523,7 @@ static void virtio_balloon_class_init(ObjectClass *klass, void *data)
     vdc->get_features = virtio_balloon_get_features;
     vdc->save = virtio_balloon_save_device;
     vdc->load = virtio_balloon_load_device;
+    vdc->set_status = virtio_balloon_set_status;
 }
 
 static const TypeInfo virtio_balloon_info = {
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] virtio-balloon: resume collecting stats on vmload
  2016-09-01 18:14 ` [Qemu-devel] [PATCH 2/2] virtio-balloon: resume collecting stats on vmload Roman Kagan
@ 2016-09-01 19:26   ` Michael S. Tsirkin
  2016-09-02  7:21     ` Roman Kagan
  0 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2016-09-01 19:26 UTC (permalink / raw)
  To: Roman Kagan; +Cc: qemu-devel, Ladi Prosek, Stefan Hajnoczi

On Thu, Sep 01, 2016 at 09:14:00PM +0300, Roman Kagan wrote:
> Upon save/restore virtio-balloon stats acquisition stops.  The reason is
> that the in-use virtqueue element is not saved, and 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.
> 
> However, the number of virtqueue in-use elements can be deduced from the
> data available on restore (see bccdef6b1a204db0f41ffb6e24ce373e4d7890d4
> "virtio: recalculate vq->inuse after migration").
> 
> So make the stats virtqueue forget that an element was popped from it,
> and start over.  As this tackles the problem on the "load" side, it
> is compatible with the state saved by earlier QEMU versions.
> 
> 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 | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 5af429a..8660052 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -406,7 +406,13 @@ static void virtio_balloon_save_device(VirtIODevice *vdev, QEMUFile *f)
>  
>  static int virtio_balloon_load(QEMUFile *f, void *opaque, size_t size)
>  {
> -    return virtio_load(VIRTIO_DEVICE(opaque), f, 1);
> +    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
> +    VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
> +    int ret = virtio_load(vdev, f, 1);
> +    /* rewind needs vq->inuse populated which happens in virtio_load() after
> +     * vdev->load */
> +    virtqueue_rewind(s->svq);
> +    return ret;
>  }
>  
>  static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
> @@ -468,6 +474,18 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev)
>      }
>  }
>  
> +static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
> +{
> +    VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
> +
> +    if (vdev->vm_running && balloon_stats_supported(s) &&
> +        (status & VIRTIO_CONFIG_S_DRIVER_OK) && !s->stats_vq_elem) {
> +        /* poll stats queue for the element we may have discarded when the VM
> +         * was stopped */
> +        virtio_balloon_receive_stats(vdev, s->svq);
> +    }
> +}
> +
>  static void virtio_balloon_instance_init(Object *obj)
>  {
>      VirtIOBalloon *s = VIRTIO_BALLOON(obj);
> @@ -505,6 +523,7 @@ static void virtio_balloon_class_init(ObjectClass *klass, void *data)
>      vdc->get_features = virtio_balloon_get_features;
>      vdc->save = virtio_balloon_save_device;
>      vdc->load = virtio_balloon_load_device;
> +    vdc->set_status = virtio_balloon_set_status;
>  }
>  
>  static const TypeInfo virtio_balloon_info = {


I'm sorry - I don't like this patch. This means that
virtio_balloon_receive_stats will be called and will poke
at the ring even if the ring was never kicked.

This is why in my opinion it is cleaner to have
virtqueue_rewind return a true/false status,
and only process the ring if ring was rewinded.

I think Stefan's patches did something similar.

> -- 
> 2.7.4

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] virtio-balloon: resume collecting stats on vmload
  2016-09-01 19:26   ` Michael S. Tsirkin
@ 2016-09-02  7:21     ` Roman Kagan
  2016-09-02 22:53       ` Michael S. Tsirkin
  0 siblings, 1 reply; 10+ messages in thread
From: Roman Kagan @ 2016-09-02  7:21 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: qemu-devel, Ladi Prosek, Stefan Hajnoczi

On Thu, Sep 01, 2016 at 10:26:54PM +0300, Michael S. Tsirkin wrote:
> On Thu, Sep 01, 2016 at 09:14:00PM +0300, Roman Kagan wrote:
> > Upon save/restore virtio-balloon stats acquisition stops.  The reason is
> > that the in-use virtqueue element is not saved, and 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.
> > 
> > However, the number of virtqueue in-use elements can be deduced from the
> > data available on restore (see bccdef6b1a204db0f41ffb6e24ce373e4d7890d4
> > "virtio: recalculate vq->inuse after migration").
> > 
> > So make the stats virtqueue forget that an element was popped from it,
> > and start over.  As this tackles the problem on the "load" side, it
> > is compatible with the state saved by earlier QEMU versions.
> > 
> > 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 | 21 ++++++++++++++++++++-
> >  1 file changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > index 5af429a..8660052 100644
> > --- a/hw/virtio/virtio-balloon.c
> > +++ b/hw/virtio/virtio-balloon.c
> > @@ -406,7 +406,13 @@ static void virtio_balloon_save_device(VirtIODevice *vdev, QEMUFile *f)
> >  
> >  static int virtio_balloon_load(QEMUFile *f, void *opaque, size_t size)
> >  {
> > -    return virtio_load(VIRTIO_DEVICE(opaque), f, 1);
> > +    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
> > +    VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
> > +    int ret = virtio_load(vdev, f, 1);
> > +    /* rewind needs vq->inuse populated which happens in virtio_load() after
> > +     * vdev->load */
> > +    virtqueue_rewind(s->svq);
> > +    return ret;
> >  }
> >  
> >  static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
> > @@ -468,6 +474,18 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev)
> >      }
> >  }
> >  
> > +static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
> > +{
> > +    VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
> > +
> > +    if (vdev->vm_running && balloon_stats_supported(s) &&
> > +        (status & VIRTIO_CONFIG_S_DRIVER_OK) && !s->stats_vq_elem) {
> > +        /* poll stats queue for the element we may have discarded when the VM
> > +         * was stopped */
> > +        virtio_balloon_receive_stats(vdev, s->svq);
> > +    }
> > +}
> > +
> >  static void virtio_balloon_instance_init(Object *obj)
> >  {
> >      VirtIOBalloon *s = VIRTIO_BALLOON(obj);
> > @@ -505,6 +523,7 @@ static void virtio_balloon_class_init(ObjectClass *klass, void *data)
> >      vdc->get_features = virtio_balloon_get_features;
> >      vdc->save = virtio_balloon_save_device;
> >      vdc->load = virtio_balloon_load_device;
> > +    vdc->set_status = virtio_balloon_set_status;
> >  }
> >  
> >  static const TypeInfo virtio_balloon_info = {
> 
> 
> I'm sorry - I don't like this patch. This means that
> virtio_balloon_receive_stats will be called and will poke
> at the ring even if the ring was never kicked.

I'm not sure I understand what the problem is with that:
virtio_balloon_receive_stats just returns early if virtio_queue_empty(),
which is no more poking at the ring than is already done in virtio_load.

> This is why in my opinion it is cleaner to have
> virtqueue_rewind return a true/false status,
> and only process the ring if ring was rewinded.
> 
> I think Stefan's patches did something similar.

I'm fine with Stefan's patches, too.

Roman.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] virtio-balloon: resume collecting stats on vmload
  2016-09-02  7:21     ` Roman Kagan
@ 2016-09-02 22:53       ` Michael S. Tsirkin
  2016-09-05  8:02         ` Roman Kagan
  0 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2016-09-02 22:53 UTC (permalink / raw)
  To: Roman Kagan, qemu-devel, Ladi Prosek, Stefan Hajnoczi

On Fri, Sep 02, 2016 at 10:21:58AM +0300, Roman Kagan wrote:
> On Thu, Sep 01, 2016 at 10:26:54PM +0300, Michael S. Tsirkin wrote:
> > On Thu, Sep 01, 2016 at 09:14:00PM +0300, Roman Kagan wrote:
> > > Upon save/restore virtio-balloon stats acquisition stops.  The reason is
> > > that the in-use virtqueue element is not saved, and 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.
> > > 
> > > However, the number of virtqueue in-use elements can be deduced from the
> > > data available on restore (see bccdef6b1a204db0f41ffb6e24ce373e4d7890d4
> > > "virtio: recalculate vq->inuse after migration").
> > > 
> > > So make the stats virtqueue forget that an element was popped from it,
> > > and start over.  As this tackles the problem on the "load" side, it
> > > is compatible with the state saved by earlier QEMU versions.
> > > 
> > > 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 | 21 ++++++++++++++++++++-
> > >  1 file changed, 20 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> > > index 5af429a..8660052 100644
> > > --- a/hw/virtio/virtio-balloon.c
> > > +++ b/hw/virtio/virtio-balloon.c
> > > @@ -406,7 +406,13 @@ static void virtio_balloon_save_device(VirtIODevice *vdev, QEMUFile *f)
> > >  
> > >  static int virtio_balloon_load(QEMUFile *f, void *opaque, size_t size)
> > >  {
> > > -    return virtio_load(VIRTIO_DEVICE(opaque), f, 1);
> > > +    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
> > > +    VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
> > > +    int ret = virtio_load(vdev, f, 1);
> > > +    /* rewind needs vq->inuse populated which happens in virtio_load() after
> > > +     * vdev->load */
> > > +    virtqueue_rewind(s->svq);
> > > +    return ret;
> > >  }
> > >  
> > >  static int virtio_balloon_load_device(VirtIODevice *vdev, QEMUFile *f,
> > > @@ -468,6 +474,18 @@ static void virtio_balloon_device_reset(VirtIODevice *vdev)
> > >      }
> > >  }
> > >  
> > > +static void virtio_balloon_set_status(VirtIODevice *vdev, uint8_t status)
> > > +{
> > > +    VirtIOBalloon *s = VIRTIO_BALLOON(vdev);
> > > +
> > > +    if (vdev->vm_running && balloon_stats_supported(s) &&
> > > +        (status & VIRTIO_CONFIG_S_DRIVER_OK) && !s->stats_vq_elem) {
> > > +        /* poll stats queue for the element we may have discarded when the VM
> > > +         * was stopped */
> > > +        virtio_balloon_receive_stats(vdev, s->svq);
> > > +    }
> > > +}
> > > +
> > >  static void virtio_balloon_instance_init(Object *obj)
> > >  {
> > >      VirtIOBalloon *s = VIRTIO_BALLOON(obj);
> > > @@ -505,6 +523,7 @@ static void virtio_balloon_class_init(ObjectClass *klass, void *data)
> > >      vdc->get_features = virtio_balloon_get_features;
> > >      vdc->save = virtio_balloon_save_device;
> > >      vdc->load = virtio_balloon_load_device;
> > > +    vdc->set_status = virtio_balloon_set_status;
> > >  }
> > >  
> > >  static const TypeInfo virtio_balloon_info = {
> > 
> > 
> > I'm sorry - I don't like this patch. This means that
> > virtio_balloon_receive_stats will be called and will poke
> > at the ring even if the ring was never kicked.
> 
> I'm not sure I understand what the problem is with that:
> virtio_balloon_receive_stats just returns early if virtio_queue_empty(),
> which is no more poking at the ring than is already done in virtio_load.

Generally we should not look at ring until there was a kick.


> > This is why in my opinion it is cleaner to have
> > virtqueue_rewind return a true/false status,
> > and only process the ring if ring was rewinded.
> > 
> > I think Stefan's patches did something similar.
> 
> I'm fine with Stefan's patches, too.
> 
> Roman.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] virtio-balloon: resume collecting stats on vmload
  2016-09-02 22:53       ` Michael S. Tsirkin
@ 2016-09-05  8:02         ` Roman Kagan
  2016-09-06  1:45           ` Michael S. Tsirkin
  0 siblings, 1 reply; 10+ messages in thread
From: Roman Kagan @ 2016-09-05  8:02 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: qemu-devel, Ladi Prosek, Stefan Hajnoczi

On Sat, Sep 03, 2016 at 01:53:53AM +0300, Michael S. Tsirkin wrote:
> On Fri, Sep 02, 2016 at 10:21:58AM +0300, Roman Kagan wrote:
> > On Thu, Sep 01, 2016 at 10:26:54PM +0300, Michael S. Tsirkin wrote:
> > > I'm sorry - I don't like this patch. This means that
> > > virtio_balloon_receive_stats will be called and will poke
> > > at the ring even if the ring was never kicked.
> > 
> > I'm not sure I understand what the problem is with that:
> > virtio_balloon_receive_stats just returns early if virtio_queue_empty(),
> > which is no more poking at the ring than is already done in virtio_load.
> 
> Generally we should not look at ring until there was a kick.

How else would you recover ->inuse?

Roman.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] virtio-balloon: resume collecting stats on vmload
  2016-09-05  8:02         ` Roman Kagan
@ 2016-09-06  1:45           ` Michael S. Tsirkin
  2016-09-06  6:50             ` Roman Kagan
  0 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2016-09-06  1:45 UTC (permalink / raw)
  To: Roman Kagan, qemu-devel, Ladi Prosek, Stefan Hajnoczi

On Mon, Sep 05, 2016 at 11:02:36AM +0300, Roman Kagan wrote:
> On Sat, Sep 03, 2016 at 01:53:53AM +0300, Michael S. Tsirkin wrote:
> > On Fri, Sep 02, 2016 at 10:21:58AM +0300, Roman Kagan wrote:
> > > On Thu, Sep 01, 2016 at 10:26:54PM +0300, Michael S. Tsirkin wrote:
> > > > I'm sorry - I don't like this patch. This means that
> > > > virtio_balloon_receive_stats will be called and will poke
> > > > at the ring even if the ring was never kicked.
> > > 
> > > I'm not sure I understand what the problem is with that:
> > > virtio_balloon_receive_stats just returns early if virtio_queue_empty(),
> > > which is no more poking at the ring than is already done in virtio_load.
> > 
> > Generally we should not look at ring until there was a kick.
> 
> How else would you recover ->inuse?
> 
> Roman.

We seem to do this:
+            vdev->vq[i].inuse = vdev->vq[i].last_avail_idx -
+                                vdev->vq[i].used_idx;

Is this wrong?

-- 
MST

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] virtio-balloon: resume collecting stats on vmload
  2016-09-06  1:45           ` Michael S. Tsirkin
@ 2016-09-06  6:50             ` Roman Kagan
  2016-09-06 10:15               ` Michael S. Tsirkin
  0 siblings, 1 reply; 10+ messages in thread
From: Roman Kagan @ 2016-09-06  6:50 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: qemu-devel, Ladi Prosek, Stefan Hajnoczi

On Tue, Sep 06, 2016 at 04:45:13AM +0300, Michael S. Tsirkin wrote:
> On Mon, Sep 05, 2016 at 11:02:36AM +0300, Roman Kagan wrote:
> > On Sat, Sep 03, 2016 at 01:53:53AM +0300, Michael S. Tsirkin wrote:
> > > On Fri, Sep 02, 2016 at 10:21:58AM +0300, Roman Kagan wrote:
> > > > On Thu, Sep 01, 2016 at 10:26:54PM +0300, Michael S. Tsirkin wrote:
> > > > > I'm sorry - I don't like this patch. This means that
> > > > > virtio_balloon_receive_stats will be called and will poke
> > > > > at the ring even if the ring was never kicked.
> > > > 
> > > > I'm not sure I understand what the problem is with that:
> > > > virtio_balloon_receive_stats just returns early if virtio_queue_empty(),
> > > > which is no more poking at the ring than is already done in virtio_load.
> > > 
> > > Generally we should not look at ring until there was a kick.
> > 
> > How else would you recover ->inuse?
> > 
> > Roman.
> 
> We seem to do this:
> +            vdev->vq[i].inuse = vdev->vq[i].last_avail_idx -
> +                                vdev->vq[i].used_idx;

Right, but .used_idx is fetched from vring two lines above:

	vdev->vq[i].used_idx = vring_used_idx(&vdev->vq[i]);

> Is this wrong?

And no, I don't think it is wrong :)  I guess there's just no other way
to obtain it.

Roman.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] virtio-balloon: resume collecting stats on vmload
  2016-09-06  6:50             ` Roman Kagan
@ 2016-09-06 10:15               ` Michael S. Tsirkin
  0 siblings, 0 replies; 10+ messages in thread
From: Michael S. Tsirkin @ 2016-09-06 10:15 UTC (permalink / raw)
  To: Roman Kagan, qemu-devel, Ladi Prosek, Stefan Hajnoczi

On Tue, Sep 06, 2016 at 09:50:00AM +0300, Roman Kagan wrote:
> On Tue, Sep 06, 2016 at 04:45:13AM +0300, Michael S. Tsirkin wrote:
> > On Mon, Sep 05, 2016 at 11:02:36AM +0300, Roman Kagan wrote:
> > > On Sat, Sep 03, 2016 at 01:53:53AM +0300, Michael S. Tsirkin wrote:
> > > > On Fri, Sep 02, 2016 at 10:21:58AM +0300, Roman Kagan wrote:
> > > > > On Thu, Sep 01, 2016 at 10:26:54PM +0300, Michael S. Tsirkin wrote:
> > > > > > I'm sorry - I don't like this patch. This means that
> > > > > > virtio_balloon_receive_stats will be called and will poke
> > > > > > at the ring even if the ring was never kicked.
> > > > > 
> > > > > I'm not sure I understand what the problem is with that:
> > > > > virtio_balloon_receive_stats just returns early if virtio_queue_empty(),
> > > > > which is no more poking at the ring than is already done in virtio_load.
> > > > 
> > > > Generally we should not look at ring until there was a kick.
> > > 
> > > How else would you recover ->inuse?
> > > 
> > > Roman.
> > 
> > We seem to do this:
> > +            vdev->vq[i].inuse = vdev->vq[i].last_avail_idx -
> > +                                vdev->vq[i].used_idx;
> 
> Right, but .used_idx is fetched from vring two lines above:
> 
> 	vdev->vq[i].used_idx = vring_used_idx(&vdev->vq[i]);
> 
> > Is this wrong?
> 
> And no, I don't think it is wrong :)  I guess there's just no other way
> to obtain it.
> 
> Roman.

Hmm right. It actually has a comment:
/* XXX virtio-1 devices */

So we do need to fix it.
Let's not add more stuff like this though.

-- 
MST

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2016-09-06 10:16 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-01 18:13 [Qemu-devel] [PATCH 0/2] virtio-balloon: resume collecting stats on vmload Roman Kagan
2016-09-01 18:13 ` [Qemu-devel] [PATCH 1/2] virtio: add virtqueue_rewind Roman Kagan
2016-09-01 18:14 ` [Qemu-devel] [PATCH 2/2] virtio-balloon: resume collecting stats on vmload Roman Kagan
2016-09-01 19:26   ` Michael S. Tsirkin
2016-09-02  7:21     ` Roman Kagan
2016-09-02 22:53       ` Michael S. Tsirkin
2016-09-05  8:02         ` Roman Kagan
2016-09-06  1:45           ` Michael S. Tsirkin
2016-09-06  6:50             ` Roman Kagan
2016-09-06 10:15               ` Michael S. Tsirkin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).