Patchwork Disable virtio-balloon memory stats interface

login
register
mail settings
Submitter Eduardo Habkost
Date Sept. 14, 2010, 2:09 p.m.
Message ID <20100914140932.GA12878@blackpad.lan.raisama.net>
Download mbox | patch
Permalink /patch/64702/
State New
Headers show

Comments

Eduardo Habkost - Sept. 14, 2010, 2:09 p.m.
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:

---
Adam Litke - Sept. 14, 2010, 2:24 p.m.
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 {
>
Eduardo Habkost - Sept. 14, 2010, 2:41 p.m.
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 Habkost - Sept. 14, 2010, 3:42 p.m.
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.
Luiz Capitulino - Sept. 14, 2010, 3:46 p.m.
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.
Adam Litke - Sept. 14, 2010, 3:59 p.m.
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.
Adam Litke - Sept. 14, 2010, 4:01 p.m.
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 {
>
Luiz Capitulino - Sept. 14, 2010, 4:33 p.m.
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.

Patch

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 {