Patchwork Disable virtio-balloon memory stats interface

login
register
mail settings
Submitter Adam Litke
Date Aug. 30, 2010, 7:17 p.m.
Message ID <1283195871.2937.26.camel@aglitke>
Download mbox | patch
Permalink /patch/63123/
State New
Headers show

Comments

Adam Litke - Aug. 30, 2010, 7:17 p.m.
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>
Amit Shah - Aug. 31, 2010, 3:42 a.m.
On (Mon) Aug 30 2010 [14:17:51], 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 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.

Rather, the monitor now gets stuck when it originally didn't.

> +     *
> +     * 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;
>  }

Acked-by: Amit Shah <amit.shah@redhat.com>

		Amit
Amit Shah - Sept. 8, 2010, 10:57 a.m.
On (Tue) Aug 31 2010 [09:12:39], Amit Shah wrote:
> On (Mon) Aug 30 2010 [14:17:51], 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 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.
> 
> Rather, the monitor now gets stuck when it originally didn't.

Care to update the comment and the commit message and send as a
top-level patch (and also for 0.13)?

Thanks

		Amit

Patch

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;
 }