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

[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

[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.

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

[Qemu-devel] [PATCH v3 0/3] virtio-balloon: Don't wait indefinitely for guest response

[Amit Shah]

Since I didn't get negative reactions to the patch I posted yesterday,
here's the complete series. It adds a qerror message that mentions the
machine is stopped or the guest is slow to respond, so the stats
should be assumed to be old.

The error report is sent out before the stats, I don't think we do
that for any other command. libvirt should be prepared to handle
this. If not, please suggest a better alternative.

Differences from v2:
- Add a timeout for uncooperative/hung guests
- Add an qerror message for letting users/management know of guest
  stopped status.

Amit Shah (3):
  balloon: Don't try fetching info if guest is unresponsive
  qerror: Add a new MACHINE_STOPPED error message
  balloon: Don't try fetching info if machine is stopped

 balloon.c           |   11 +++++++----
 balloon.h           |    6 ++++--
 hw/virtio-balloon.c |   21 +++++++++++++++++++--
 qerror.c            |    4 ++++
 qerror.h            |    3 +++
 5 files changed, 37 insertions(+), 8 deletions(-)

-- 
1.7.2.2

[Qemu-devel] [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message

[Amit Shah]

This error message denotes some command was not successful in completing
as the guest was unresponsive.

Use it in the virtio-balloon code when showing older, cached data.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-balloon.c |    1 +
 qerror.c            |    4 ++++
 qerror.h            |    3 +++
 3 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index d6c66cf..309c343 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -140,6 +140,7 @@ static void complete_stats_request(VirtIOBalloon *vb)
 
 static void show_old_stats(void *opaque)
 {
+    qerror_report(QERR_MACHINE_STOPPED);
     complete_stats_request(opaque);
 }
 
diff --git a/qerror.c b/qerror.c
index 0af3ab3..b7a9f7f 100644
--- a/qerror.c
+++ b/qerror.c
@@ -141,6 +141,10 @@ static const QErrorStringTable qerror_table[] = {
         .desc      = "Using KVM without %(capability), %(feature) unavailable",
     },
     {
+        .error_fmt = QERR_MACHINE_STOPPED,
+        .desc      = "The machine is stopped or the guest is taking too long to respond",
+    },
+    {
         .error_fmt = QERR_MIGRATION_EXPECTED,
         .desc      = "An incoming migration is expected before this command can be executed",
     },
diff --git a/qerror.h b/qerror.h
index 62802ea..470577a 100644
--- a/qerror.h
+++ b/qerror.h
@@ -121,6 +121,9 @@ QError *qobject_to_qerror(const QObject *obj);
 #define QERR_KVM_MISSING_CAP \
     "{ 'class': 'KVMMissingCap', 'data': { 'capability': %s, 'feature': %s } }"
 
+#define QERR_MACHINE_STOPPED \
+    "{ 'class': 'MachineStopped', 'data': {} }"
+
 #define QERR_MIGRATION_EXPECTED \
     "{ 'class': 'MigrationExpected', 'data': {} }"
 
-- 
1.7.2.2

[Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message

[Daniel P. Berrange]

On Fri, Aug 27, 2010 at 10:57:10AM +0530, Amit Shah wrote:
> This error message denotes some command was not successful in completing
> as the guest was unresponsive.
> 
> Use it in the virtio-balloon code when showing older, cached data.
> 
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  hw/virtio-balloon.c |    1 +
>  qerror.c            |    4 ++++
>  qerror.h            |    3 +++
>  3 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index d6c66cf..309c343 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -140,6 +140,7 @@ static void complete_stats_request(VirtIOBalloon *vb)
>  
>  static void show_old_stats(void *opaque)
>  {
> +    qerror_report(QERR_MACHINE_STOPPED);
>      complete_stats_request(opaque);
>  }


NACK. It has always been allowed & valid to call query-balloon
to get the current balloon level. We must not throw an error
just because the recently added mem stats can't be refreshed.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

[Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message

[Anthony Liguori]

On 08/27/2010 04:29 AM, Daniel P. Berrange wrote:
> On Fri, Aug 27, 2010 at 10:57:10AM +0530, Amit Shah wrote:
>    
>> This error message denotes some command was not successful in completing
>> as the guest was unresponsive.
>>
>> Use it in the virtio-balloon code when showing older, cached data.
>>
>> Signed-off-by: Amit Shah<amit.shah@redhat.com>
>> ---
>>   hw/virtio-balloon.c |    1 +
>>   qerror.c            |    4 ++++
>>   qerror.h            |    3 +++
>>   3 files changed, 8 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
>> index d6c66cf..309c343 100644
>> --- a/hw/virtio-balloon.c
>> +++ b/hw/virtio-balloon.c
>> @@ -140,6 +140,7 @@ static void complete_stats_request(VirtIOBalloon *vb)
>>
>>   static void show_old_stats(void *opaque)
>>   {
>> +    qerror_report(QERR_MACHINE_STOPPED);
>>       complete_stats_request(opaque);
>>   }
>>      
>
> NACK. It has always been allowed&  valid to call query-balloon
> to get the current balloon level. We must not throw an error
> just because the recently added mem stats can't be refreshed.
>    

I think that's a fair comment but why even bother fixing the command.  
Let's introduce a new command that just gets a single piece of 
information instead of having a command return lots of information.

Regards,

Anthony Liguori

> Regards,
> Daniel
>    

[Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message

[Amit Shah]

On (Fri) Aug 27 2010 [07:39:37], Anthony Liguori wrote:
> >
> >NACK. It has always been allowed&  valid to call query-balloon
> >to get the current balloon level. We must not throw an error
> >just because the recently added mem stats can't be refreshed.
> 
> I think that's a fair comment but why even bother fixing the
> command.  Let's introduce a new command that just gets a single
> piece of information instead of having a command return lots of
> information.

Instead, let's have query-balloon do the same thing as it did in 0.12
and add a new command for 0.13 (query-balloon-stats) that does the stats
with timeout?

That way we won't have regressions from 0.12 and have the new behaviour
only for 0.13, which we can say is 'experimental'.

		Amit

Re: [Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> On (Fri) Aug 27 2010 [07:39:37], Anthony Liguori wrote:
>> >
>> >NACK. It has always been allowed&  valid to call query-balloon
>> >to get the current balloon level. We must not throw an error
>> >just because the recently added mem stats can't be refreshed.
>> 
>> I think that's a fair comment but why even bother fixing the
>> command.  Let's introduce a new command that just gets a single
>> piece of information instead of having a command return lots of
>> information.
>
> Instead, let's have query-balloon do the same thing as it did in 0.12
> and add a new command for 0.13 (query-balloon-stats) that does the stats
> with timeout?
>
> That way we won't have regressions from 0.12 and have the new behaviour
> only for 0.13, which we can say is 'experimental'.

Sounds sensible to me.  Not too late for fixing 0.13?  Anthony?

Re: [Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message

[Anthony Liguori]

On 08/30/2010 03:30 AM, Markus Armbruster wrote:
> Amit Shah<amit.shah@redhat.com>  writes:
>
>    
>> On (Fri) Aug 27 2010 [07:39:37], Anthony Liguori wrote:
>>      
>>>> NACK. It has always been allowed&   valid to call query-balloon
>>>> to get the current balloon level. We must not throw an error
>>>> just because the recently added mem stats can't be refreshed.
>>>>          
>>> I think that's a fair comment but why even bother fixing the
>>> command.  Let's introduce a new command that just gets a single
>>> piece of information instead of having a command return lots of
>>> information.
>>>        
>> Instead, let's have query-balloon do the same thing as it did in 0.12
>> and add a new command for 0.13 (query-balloon-stats) that does the stats
>> with timeout?
>>
>> That way we won't have regressions from 0.12 and have the new behaviour
>> only for 0.13, which we can say is 'experimental'.
>>      
> Sounds sensible to me.  Not too late for fixing 0.13?  Anthony?
>    

No new commands for 0.13.  I think the only option is to revert the 
query-balloon behavior.  A new command is too risky as -rc1 is imminent.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH v3 2/3] qerror: Add a new MACHINE_STOPPED error message

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 08/30/2010 03:30 AM, Markus Armbruster wrote:
>> Amit Shah<amit.shah@redhat.com>  writes:
>>
>>    
>>> On (Fri) Aug 27 2010 [07:39:37], Anthony Liguori wrote:
>>>      
>>>>> NACK. It has always been allowed&   valid to call query-balloon
>>>>> to get the current balloon level. We must not throw an error
>>>>> just because the recently added mem stats can't be refreshed.
>>>>>          
>>>> I think that's a fair comment but why even bother fixing the
>>>> command.  Let's introduce a new command that just gets a single
>>>> piece of information instead of having a command return lots of
>>>> information.
>>>>        
>>> Instead, let's have query-balloon do the same thing as it did in 0.12
>>> and add a new command for 0.13 (query-balloon-stats) that does the stats
>>> with timeout?
>>>
>>> That way we won't have regressions from 0.12 and have the new behaviour
>>> only for 0.13, which we can say is 'experimental'.
>>>      
>> Sounds sensible to me.  Not too late for fixing 0.13?  Anthony?
>>    
>
> No new commands for 0.13.  I think the only option is to revert the
> query-balloon behavior.  A new command is too risky as -rc1 is
> imminent.

Anyone care to submit a patch?

[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