[Qemu-devel] [PATCH] Disable virtio-balloon memory stats interface

[Qemu-devel] [PATCH] Disable virtio-balloon memory stats interface

[Adam Litke]

Just got back from vacation and saw this thread.  I agree with Anthony
that the best thing to do is disable the memory stats interface for
0.13.  We need to fix the underlying problems in qemu with respect to
asynchronous commands first, then we can look at re-enabling the
feature.

The virtio feature negotiation mechanism allows the feature to be
disabled by commenting out a single line of code.  This late in the 0.13
release cycle, this is the safest option.  We can refactor/remove code
in the development tree as needed.

On Mon, 2010-08-30 at 17:01 +0200, Markus Armbruster wrote:
> Anyone care to submit a patch?

Sure...

[PATCH] Disable virtio-balloon memory stats interface

The addition of memory stats reporting to the virtio balloon causes
the 'info balloon' command to become asynchronous.  This is a regression
because management tools that consume this command were not designed to
handle lost or delayed responses.

To fix this regression, the virtio balloon memory stats feature is being
disabled in qemu-0.13.

Signed-off-by: Adam Litke <agl@us.ibm.com>

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 9fe3886..2d80382 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -190,7 +190,18 @@ 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);
+    /*
+     * The addition of memory stats reporting to the virtio balloon causes
+     * the 'info balloon' command to become asynchronous.  This is a regression
+     * because management tools that consume this command were not designed to
+     * handle lost or delayed responses.
+     *
+     * To fix this regression, the virtio balloon memory stats feature is being
+     * disabled in qemu-0.13.
+     *
+     * -aglitke
+     */
+    /* f |= (1 << VIRTIO_BALLOON_F_STATS_VQ); */
     return f;
 }
 


-- 
Thanks,
Adam

[Qemu-devel] [PATCH] Disable virtio-balloon memory stats interface

[Adam Litke]

The addition of memory stats reporting to the virtio balloon causes
the 'info balloon' command to become asynchronous.  This is a regression
because in some cases it can hang the user monitor.

Disable this feature until a better interface for asynchronous commands
can be worked out.

Signed-off-by: Adam Litke <agl@us.ibm.com>
---
 hw/virtio-balloon.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 9fe3886..dcdada6 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -190,7 +190,17 @@ 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);
+    /*
+     * The addition of memory stats reporting to the virtio balloon causes
+     * the 'info balloon' command to become asynchronous.  This is a regression
+     * because in some cases it can hang the user monitor.
+     *
+     * Disable this feature until a better interface for asynchronous commands
+     * can be worked out.
+     *
+     * -aglitke
+     */
+    /* f |= (1 << VIRTIO_BALLOON_F_STATS_VQ); */
     return f;
 }
 
-- 
1.6.1.3

Re: [Qemu-devel] [PATCH] Disable virtio-balloon memory stats interface

[Eduardo Habkost]

On Wed, Sep 08, 2010 at 09:21:16AM -0500, Adam Litke wrote:
> The addition of memory stats reporting to the virtio balloon causes
> the 'info balloon' command to become asynchronous.  This is a regression
> because in some cases it can hang the user monitor.
> 
> Disable this feature until a better interface for asynchronous commands
> can be worked out.
> 
> Signed-off-by: Adam Litke <agl@us.ibm.com>
> ---
>  hw/virtio-balloon.c |   12 +++++++++++-
>  1 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index 9fe3886..dcdada6 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -190,7 +190,17 @@ 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);
> +    /*
> +     * The addition of memory stats reporting to the virtio balloon causes
> +     * the 'info balloon' command to become asynchronous.  This is a regression
> +     * because in some cases it can hang the user monitor.
> +     *
> +     * Disable this feature until a better interface for asynchronous commands
> +     * can be worked out.
> +     *
> +     * -aglitke
> +     */
> +    /* f |= (1 << VIRTIO_BALLOON_F_STATS_VQ); */


This field is guest-visible, won't this cause problems on migration?

Isn't it better to disable it on the "info balloon" side, so the guest
knows that the host may start requesting stat info eventually? I suggest
doing this:

---
diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 1e4dfdd..92e9057 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -30,6 +30,10 @@
 #include <sys/mman.h>
 #endif
 
+/* Disable guest-provided stats by now (bz#623903, bz#626544) */
+#define ENABLE_GUEST_STATS   0
+
+
 typedef struct VirtIOBalloon
 {
     VirtIODevice vdev;
@@ -84,12 +88,14 @@ static QObject *get_stats_qobject(VirtIOBalloon *dev)
                                   VIRTIO_BALLOON_PFN_SHIFT);
 
     stat_put(dict, "actual", actual);
+#if ENABLE_GUEST_STATS
     stat_put(dict, "mem_swapped_in", dev->stats[VIRTIO_BALLOON_S_SWAP_IN]);
     stat_put(dict, "mem_swapped_out", dev->stats[VIRTIO_BALLOON_S_SWAP_OUT]);
     stat_put(dict, "major_page_faults", dev->stats[VIRTIO_BALLOON_S_MAJFLT]);
     stat_put(dict, "minor_page_faults", dev->stats[VIRTIO_BALLOON_S_MINFLT]);
     stat_put(dict, "free_mem", dev->stats[VIRTIO_BALLOON_S_MEMFREE]);
     stat_put(dict, "total_mem", dev->stats[VIRTIO_BALLOON_S_MEMTOT]);
+#endif
 
     return QOBJECT(dict);
 }
@@ -215,7 +221,7 @@ static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
         }
         dev->stats_callback = cb;
         dev->stats_opaque_callback_data = cb_data; 
-        if (dev->vdev.guest_features & (1 << VIRTIO_BALLOON_F_STATS_VQ)) {
+        if (ENABLE_GUEST_STATS && (dev->vdev.guest_features & (1 << VIRTIO_BALLOON_F_STATS_VQ))) {
             virtqueue_push(dev->svq, &dev->stats_vq_elem, dev->stats_vq_offset);
             virtio_notify(&dev->vdev, dev->svq);
         } else {

-- 
Eduardo

Re: [Qemu-devel] [PATCH] Disable virtio-balloon memory stats interface

[Adam Litke]

On Tue, 2010-09-14 at 11:09 -0300, Eduardo Habkost wrote:
> On Wed, Sep 08, 2010 at 09:21:16AM -0500, Adam Litke wrote:
> > The addition of memory stats reporting to the virtio balloon causes
> > the 'info balloon' command to become asynchronous.  This is a regression
> > because in some cases it can hang the user monitor.
> > 
> > Disable this feature until a better interface for asynchronous commands
> > can be worked out.
> > 
> > Signed-off-by: Adam Litke <agl@us.ibm.com>
> > ---
> >  hw/virtio-balloon.c |   12 +++++++++++-
> >  1 files changed, 11 insertions(+), 1 deletions(-)
> > 
> > diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> > index 9fe3886..dcdada6 100644
> > --- a/hw/virtio-balloon.c
> > +++ b/hw/virtio-balloon.c
> > @@ -190,7 +190,17 @@ 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);
> > +    /*
> > +     * The addition of memory stats reporting to the virtio balloon causes
> > +     * the 'info balloon' command to become asynchronous.  This is a regression
> > +     * because in some cases it can hang the user monitor.
> > +     *
> > +     * Disable this feature until a better interface for asynchronous commands
> > +     * can be worked out.
> > +     *
> > +     * -aglitke
> > +     */
> > +    /* f |= (1 << VIRTIO_BALLOON_F_STATS_VQ); */
> 
> 
> This field is guest-visible, won't this cause problems on migration?

I haven't followed migration very closely, but isn't this a common
problem whenever one migrates a vm to a newer version of qemu that has
more features.  I thought that virtio feature negotiation would ensure
that stats have been disabled at the device level and would remain
disabled post migration.  Please correct me if I am mistaken.

> Isn't it better to disable it on the "info balloon" side, so the guest
> knows that the host may start requesting stat info eventually? I suggest
> doing this:

While I think this method would also work, I would really like to use
the feature bit if possible, since that is what the mechanism is
designed for.

> ---
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index 1e4dfdd..92e9057 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -30,6 +30,10 @@
>  #include <sys/mman.h>
>  #endif
> 
> +/* Disable guest-provided stats by now (bz#623903, bz#626544) */
> +#define ENABLE_GUEST_STATS   0
> +
> +
>  typedef struct VirtIOBalloon
>  {
>      VirtIODevice vdev;
> @@ -84,12 +88,14 @@ static QObject *get_stats_qobject(VirtIOBalloon *dev)
>                                    VIRTIO_BALLOON_PFN_SHIFT);
> 
>      stat_put(dict, "actual", actual);
> +#if ENABLE_GUEST_STATS
>      stat_put(dict, "mem_swapped_in", dev->stats[VIRTIO_BALLOON_S_SWAP_IN]);
>      stat_put(dict, "mem_swapped_out", dev->stats[VIRTIO_BALLOON_S_SWAP_OUT]);
>      stat_put(dict, "major_page_faults", dev->stats[VIRTIO_BALLOON_S_MAJFLT]);
>      stat_put(dict, "minor_page_faults", dev->stats[VIRTIO_BALLOON_S_MINFLT]);
>      stat_put(dict, "free_mem", dev->stats[VIRTIO_BALLOON_S_MEMFREE]);
>      stat_put(dict, "total_mem", dev->stats[VIRTIO_BALLOON_S_MEMTOT]);
> +#endif
> 
>      return QOBJECT(dict);
>  }
> @@ -215,7 +221,7 @@ static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
>          }
>          dev->stats_callback = cb;
>          dev->stats_opaque_callback_data = cb_data; 
> -        if (dev->vdev.guest_features & (1 << VIRTIO_BALLOON_F_STATS_VQ)) {
> +        if (ENABLE_GUEST_STATS && (dev->vdev.guest_features & (1 << VIRTIO_BALLOON_F_STATS_VQ))) {
>              virtqueue_push(dev->svq, &dev->stats_vq_elem, dev->stats_vq_offset);
>              virtio_notify(&dev->vdev, dev->svq);
>          } else {
> 

-- 
Thanks,
Adam

Re: [Qemu-devel] [PATCH] Disable virtio-balloon memory stats interface

[Eduardo Habkost]

On Tue, Sep 14, 2010 at 09:24:11AM -0500, Adam Litke wrote:
> On Tue, 2010-09-14 at 11:09 -0300, Eduardo Habkost wrote:
> > On Wed, Sep 08, 2010 at 09:21:16AM -0500, Adam Litke wrote:
[...]
> > >  static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f)
> > >  {
> > > -    f |= (1 << VIRTIO_BALLOON_F_STATS_VQ);
> > > +    /*
> > > +     * The addition of memory stats reporting to the virtio balloon causes
> > > +     * the 'info balloon' command to become asynchronous.  This is a regression
> > > +     * because in some cases it can hang the user monitor.
> > > +     *
> > > +     * Disable this feature until a better interface for asynchronous commands
> > > +     * can be worked out.
> > > +     *
> > > +     * -aglitke
> > > +     */
> > > +    /* f |= (1 << VIRTIO_BALLOON_F_STATS_VQ); */
> > 
> > 
> > This field is guest-visible, won't this cause problems on migration?
> 
> I haven't followed migration very closely, but isn't this a common
> problem whenever one migrates a vm to a newer version of qemu that has
> more features.  I thought that virtio feature negotiation would ensure
> that stats have been disabled at the device level and would remain
> disabled post migration.  Please correct me if I am mistaken.

If this is the case, then it's another reason to keep the feature bit
enabled: the feature bit just let the guest know that the host may
request balloon stats at any moment, and it's better to keep that
capability in case the guest is migrated, isn't it?

Also, what happens if we try to migrate from a qemu version that had the
feature bit set to a qemu version without this feature bit?

I don't know the details either, but even if this works gracefully for
migration in both directions, it sound much simpler to not change the
guest-visible machine model and just change the "info balloon" behavior.


> 
> > Isn't it better to disable it on the "info balloon" side, so the guest
> > knows that the host may start requesting stat info eventually? I suggest
> > doing this:
> 
> While I think this method would also work, I would really like to use
> the feature bit if possible, since that is what the mechanism is
> designed for.

My point is that if we can avoid making guest-visible changes easily, we
should do that. It doesn't matter for the guest what the "info balloon"
command does.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] Disable virtio-balloon memory stats interface

[Eduardo Habkost]

On Tue, Sep 14, 2010 at 11:41:55AM -0300, Eduardo Habkost wrote:
> On Tue, Sep 14, 2010 at 09:24:11AM -0500, Adam Litke wrote:
> > On Tue, 2010-09-14 at 11:09 -0300, Eduardo Habkost wrote:
> > > On Wed, Sep 08, 2010 at 09:21:16AM -0500, Adam Litke wrote:
> [...]
> > > >  static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f)
> > > >  {
> > > > -    f |= (1 << VIRTIO_BALLOON_F_STATS_VQ);
> > > > +    /*
> > > > +     * The addition of memory stats reporting to the virtio balloon causes
> > > > +     * the 'info balloon' command to become asynchronous.  This is a regression
> > > > +     * because in some cases it can hang the user monitor.
> > > > +     *
> > > > +     * Disable this feature until a better interface for asynchronous commands
> > > > +     * can be worked out.
> > > > +     *
> > > > +     * -aglitke
> > > > +     */
> > > > +    /* f |= (1 << VIRTIO_BALLOON_F_STATS_VQ); */
> > > 
> > > 
> > > This field is guest-visible, won't this cause problems on migration?
> > 
> > I haven't followed migration very closely, but isn't this a common
> > problem whenever one migrates a vm to a newer version of qemu that has
> > more features.  I thought that virtio feature negotiation would ensure
> > that stats have been disabled at the device level and would remain
> > disabled post migration.  Please correct me if I am mistaken.
> 
> If this is the case, then it's another reason to keep the feature bit
> enabled: the feature bit just let the guest know that the host may
> request balloon stats at any moment, and it's better to keep that
> capability in case the guest is migrated, isn't it?
> 
> Also, what happens if we try to migrate from a qemu version that had the
> feature bit set to a qemu version without this feature bit?
> 
> I don't know the details either, but even if this works gracefully for
> migration in both directions, it sound much simpler to not change the
> guest-visible machine model and just change the "info balloon" behavior.

It looks worse than that: by disabling this feature you will break migration
from older qemu versions that had the old "info balloon" behavior.

This is the incoming virtio migration code:

int virtio_load(VirtIODevice *vdev, QEMUFile *f)
{
[...]
    uint32_t supported_features =
        vdev->binding->get_features(vdev->binding_opaque);
[...]
    qemu_get_8s(f, &vdev->status);
    qemu_get_8s(f, &vdev->isr);
    qemu_get_be16s(f, &vdev->queue_sel);
    qemu_get_be32s(f, &features);
    if (features & ~supported_features) {
        fprintf(stderr, "Features 0x%x unsupported. Allowed features: 0x%x\n",
                features, supported_features);
        return -1;
    }
[...]

To make things worse: virtio_net_load() doesn't check the return value of
virtio_load(), so it will probably crash in an unpredictable way.

I keep my suggestion: if all we need is to change the way "info balloon"
behaves, then we can simply change the "info balloon" behavior, intead
of changing the guest-visible machine model.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] Disable virtio-balloon memory stats interface

[Luiz Capitulino]

On Tue, 14 Sep 2010 12:42:00 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> I keep my suggestion: if all we need is to change the way "info balloon"
> behaves, then we can simply change the "info balloon" behavior, intead
> of changing the guest-visible machine model.

Adam, what problems do you see with this solution?

We need to resolve this asap for 0.13.

Re: [Qemu-devel] [PATCH] Disable virtio-balloon memory stats interface

[Adam Litke]

On Tue, 2010-09-14 at 12:46 -0300, Luiz Capitulino wrote:
> On Tue, 14 Sep 2010 12:42:00 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > I keep my suggestion: if all we need is to change the way "info balloon"
> > behaves, then we can simply change the "info balloon" behavior, intead
> > of changing the guest-visible machine model.
> 
> Adam, what problems do you see with this solution?

I have been sufficiently convinced that the feature bits won't work
nicely in the face of migration. 

> We need to resolve this asap for 0.13.

Let's go ahead with Eduardo's patch then.

-- 
Thanks,
Adam

Re: [Qemu-devel] [PATCH] Disable virtio-balloon memory stats interface

[Adam Litke]

Ok, I can see how this will work better for the migration case.

Acked-by: Adam Litke <agl@us.ibm.com>

On Tue, 2010-09-14 at 11:09 -0300, Eduardo Habkost wrote:
> This field is guest-visible, won't this cause problems on migration?
> 
> Isn't it better to disable it on the "info balloon" side, so the guest
> knows that the host may start requesting stat info eventually? I suggest
> doing this:
> 
> ---
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index 1e4dfdd..92e9057 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -30,6 +30,10 @@
>  #include <sys/mman.h>
>  #endif
> 
> +/* Disable guest-provided stats by now (bz#623903, bz#626544) */
> +#define ENABLE_GUEST_STATS   0
> +
> +
>  typedef struct VirtIOBalloon
>  {
>      VirtIODevice vdev;
> @@ -84,12 +88,14 @@ static QObject *get_stats_qobject(VirtIOBalloon *dev)
>                                    VIRTIO_BALLOON_PFN_SHIFT);
> 
>      stat_put(dict, "actual", actual);
> +#if ENABLE_GUEST_STATS
>      stat_put(dict, "mem_swapped_in", dev->stats[VIRTIO_BALLOON_S_SWAP_IN]);
>      stat_put(dict, "mem_swapped_out", dev->stats[VIRTIO_BALLOON_S_SWAP_OUT]);
>      stat_put(dict, "major_page_faults", dev->stats[VIRTIO_BALLOON_S_MAJFLT]);
>      stat_put(dict, "minor_page_faults", dev->stats[VIRTIO_BALLOON_S_MINFLT]);
>      stat_put(dict, "free_mem", dev->stats[VIRTIO_BALLOON_S_MEMFREE]);
>      stat_put(dict, "total_mem", dev->stats[VIRTIO_BALLOON_S_MEMTOT]);
> +#endif
> 
>      return QOBJECT(dict);
>  }
> @@ -215,7 +221,7 @@ static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
>          }
>          dev->stats_callback = cb;
>          dev->stats_opaque_callback_data = cb_data; 
> -        if (dev->vdev.guest_features & (1 << VIRTIO_BALLOON_F_STATS_VQ)) {
> +        if (ENABLE_GUEST_STATS && (dev->vdev.guest_features & (1 << VIRTIO_BALLOON_F_STATS_VQ))) {
>              virtqueue_push(dev->svq, &dev->stats_vq_elem, dev->stats_vq_offset);
>              virtio_notify(&dev->vdev, dev->svq);
>          } else {
> 

-- 
Thanks,
Adam

Re: [Qemu-devel] [PATCH] Disable virtio-balloon memory stats interface

[Luiz Capitulino]

On Tue, 14 Sep 2010 10:59:56 -0500
Adam Litke <agl@us.ibm.com> wrote:

> On Tue, 2010-09-14 at 12:46 -0300, Luiz Capitulino wrote:
> > On Tue, 14 Sep 2010 12:42:00 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > I keep my suggestion: if all we need is to change the way "info balloon"
> > > behaves, then we can simply change the "info balloon" behavior, intead
> > > of changing the guest-visible machine model.
> > 
> > Adam, what problems do you see with this solution?
> 
> I have been sufficiently convinced that the feature bits won't work
> nicely in the face of migration. 
> 
> > We need to resolve this asap for 0.13.
> 
> Let's go ahead with Eduardo's patch then.

Ok, Eduardo can you send your patch again? As a new thread, so that
it's easier to Anthony's scripts.