| Message ID | 20100914140932.GA12878@blackpad.lan.raisama.net |
|---|---|
| State | New |
| Headers | show |
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 { >
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.
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.
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.
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, 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 { >
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.
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 {