[V2] balloon: Don't try fetching info if machine is stopped

Submitted by Amit Shah on Aug. 20, 2010, 12:48 a.m.

Details

Message ID 8450ea8e785c9049f0b3edddc01650a8c4b9ffe7.1282265244.git.amit.shah@redhat.com
State New
Headers show

Commit Message

Amit Shah Aug. 20, 2010, 12:48 a.m.
If the machine is stopped and 'info balloon' is invoked, the monitor
process just hangs waiting for info from the guest. Return the most
recent balloon data in that case.

See https://bugzilla.redhat.com/show_bug.cgi?id=623903

Reported-by: <lihuang@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
v2: simplify call to qemu_balloon_status - Paulo

 balloon.c           |   11 +++++++----
 balloon.h           |    6 ++++--
 hw/virtio-balloon.c |    9 +++++++--
 3 files changed, 18 insertions(+), 8 deletions(-)

Comments

Paolo Bonzini Aug. 20, 2010, 10:13 a.m.
On 08/20/2010 02:48 AM, Amit Shah wrote:
> If the machine is stopped and 'info balloon' is invoked, the monitor
> process just hangs waiting for info from the guest. Return the most
> recent balloon data in that case.
>
> See https://bugzilla.redhat.com/show_bug.cgi?id=623903
>
> Reported-by:<lihuang@redhat.com>
> Signed-off-by: Amit Shah<amit.shah@redhat.com>
> ---
> v2: simplify call to qemu_balloon_status - Paulo
>
>   balloon.c           |   11 +++++++----
>   balloon.h           |    6 ++++--
>   hw/virtio-balloon.c |    9 +++++++--
>   3 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/balloon.c b/balloon.c
> index 8e0b7f1..8b05d20 100644
> --- a/balloon.c
> +++ b/balloon.c
> @@ -43,17 +43,20 @@ void qemu_add_balloon_handler(QEMUBalloonEvent *func, void *opaque)
>   int qemu_balloon(ram_addr_t target, MonitorCompletion cb, void *opaque)
>   {
>       if (qemu_balloon_event) {
> -        qemu_balloon_event(qemu_balloon_event_opaque, target, cb, opaque);
> +        qemu_balloon_event(qemu_balloon_event_opaque, target, cb, opaque,
> +                           false);
>           return 1;
>       } else {
>           return 0;
>       }
>   }
>
> -int qemu_balloon_status(MonitorCompletion cb, void *opaque)
> +int qemu_balloon_status(MonitorCompletion cb, void *opaque,
> +                        bool get_cached_data)
>   {
>       if (qemu_balloon_event) {
> -        qemu_balloon_event(qemu_balloon_event_opaque, 0, cb, opaque);
> +        qemu_balloon_event(qemu_balloon_event_opaque, 0, cb, opaque,
> +                           get_cached_data);
>           return 1;
>       } else {
>           return 0;
> @@ -113,7 +116,7 @@ int do_info_balloon(Monitor *mon, MonitorCompletion cb, void *opaque)
>           return -1;
>       }
>
> -    ret = qemu_balloon_status(cb, opaque);
> +    ret = qemu_balloon_status(cb, opaque, !vm_running);
>       if (!ret) {
>           qerror_report(QERR_DEVICE_NOT_ACTIVE, "balloon");
>           return -1;
> diff --git a/balloon.h b/balloon.h
> index d478e28..729631c 100644
> --- a/balloon.h
> +++ b/balloon.h
> @@ -17,13 +17,15 @@
>   #include "monitor.h"
>
>   typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target,
> -                                MonitorCompletion cb, void *cb_data);
> +                                MonitorCompletion cb, void *cb_data,
> +                                bool get_cached_data);
>
>   void qemu_add_balloon_handler(QEMUBalloonEvent *func, void *opaque);
>
>   int qemu_balloon(ram_addr_t target, MonitorCompletion cb, void *opaque);
>
> -int qemu_balloon_status(MonitorCompletion cb, void *opaque);
> +int qemu_balloon_status(MonitorCompletion cb, void *opaque,
> +                        bool get_cached_data);
>
>   void monitor_print_balloon(Monitor *mon, const QObject *data);
>   int do_info_balloon(Monitor *mon, MonitorCompletion cb, void *opaque);
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index 9fe3886..68df891 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -195,7 +195,8 @@ static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f)
>   }
>
>   static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
> -                                     MonitorCompletion cb, void *cb_data)
> +                                     MonitorCompletion cb, void *cb_data,
> +                                     bool get_cached_data)
>   {
>       VirtIOBalloon *dev = opaque;
>
> @@ -213,8 +214,12 @@ static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
>               return;
>           }
>           dev->stats_callback = cb;
> -        dev->stats_opaque_callback_data = cb_data;
> +        dev->stats_opaque_callback_data = cb_data;
>           if (dev->vdev.guest_features&  (1<<  VIRTIO_BALLOON_F_STATS_VQ)) {
> +            if (get_cached_data) {
> +                complete_stats_request(dev);
> +                return;
> +            }
>               virtqueue_push(dev->svq,&dev->stats_vq_elem, dev->stats_vq_offset);
>               virtio_notify(&dev->vdev, dev->svq);
>           } else {

Acked-By: Paolo Bonzini <pbonzini@redhat.com>

Paolo
Amit Shah Aug. 20, 2010, 12:01 p.m.
On (Fri) Aug 20 2010 [06:18:11], Amit Shah wrote:
> If the machine is stopped and 'info balloon' is invoked, the monitor
> process just hangs waiting for info from the guest. Return the most
> recent balloon data in that case.
> 
> See https://bugzilla.redhat.com/show_bug.cgi?id=623903
> 
> Reported-by: <lihuang@redhat.com>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
> v2: simplify call to qemu_balloon_status - Paulo
> 
>  balloon.c           |   11 +++++++----
>  balloon.h           |    6 ++++--
>  hw/virtio-balloon.c |    9 +++++++--
>  3 files changed, 18 insertions(+), 8 deletions(-)

Please pick this for 0.13 as well.

Thanks,

		Amit
Anthony Liguori Aug. 22, 2010, 9:54 p.m.
On 08/19/2010 07:48 PM, Amit Shah wrote:
> If the machine is stopped and 'info balloon' is invoked, the monitor
> process just hangs waiting for info from the guest. Return the most
> recent balloon data in that case.
>
> See https://bugzilla.redhat.com/show_bug.cgi?id=623903
>
> Reported-by:<lihuang@redhat.com>
> Signed-off-by: Amit Shah<amit.shah@redhat.com>
>    

!vm_running is just a special case of an unresponsive guest.  Even if 
the guest was running, if it was oops'd and the administrator didn't 
know, you would have the same issue.

I'd suggest using a timeout based on rt_clock.  If the stats request 
times out, print an appropriate message to the user.

Regards,

Anthony Liguori

> ---
> v2: simplify call to qemu_balloon_status - Paulo
>
>   balloon.c           |   11 +++++++----
>   balloon.h           |    6 ++++--
>   hw/virtio-balloon.c |    9 +++++++--
>   3 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/balloon.c b/balloon.c
> index 8e0b7f1..8b05d20 100644
> --- a/balloon.c
> +++ b/balloon.c
> @@ -43,17 +43,20 @@ void qemu_add_balloon_handler(QEMUBalloonEvent *func, void *opaque)
>   int qemu_balloon(ram_addr_t target, MonitorCompletion cb, void *opaque)
>   {
>       if (qemu_balloon_event) {
> -        qemu_balloon_event(qemu_balloon_event_opaque, target, cb, opaque);
> +        qemu_balloon_event(qemu_balloon_event_opaque, target, cb, opaque,
> +                           false);
>           return 1;
>       } else {
>           return 0;
>       }
>   }
>
> -int qemu_balloon_status(MonitorCompletion cb, void *opaque)
> +int qemu_balloon_status(MonitorCompletion cb, void *opaque,
> +                        bool get_cached_data)
>   {
>       if (qemu_balloon_event) {
> -        qemu_balloon_event(qemu_balloon_event_opaque, 0, cb, opaque);
> +        qemu_balloon_event(qemu_balloon_event_opaque, 0, cb, opaque,
> +                           get_cached_data);
>           return 1;
>       } else {
>           return 0;
> @@ -113,7 +116,7 @@ int do_info_balloon(Monitor *mon, MonitorCompletion cb, void *opaque)
>           return -1;
>       }
>
> -    ret = qemu_balloon_status(cb, opaque);
> +    ret = qemu_balloon_status(cb, opaque, !vm_running);
>       if (!ret) {
>           qerror_report(QERR_DEVICE_NOT_ACTIVE, "balloon");
>           return -1;
> diff --git a/balloon.h b/balloon.h
> index d478e28..729631c 100644
> --- a/balloon.h
> +++ b/balloon.h
> @@ -17,13 +17,15 @@
>   #include "monitor.h"
>
>   typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target,
> -                                MonitorCompletion cb, void *cb_data);
> +                                MonitorCompletion cb, void *cb_data,
> +                                bool get_cached_data);
>
>   void qemu_add_balloon_handler(QEMUBalloonEvent *func, void *opaque);
>
>   int qemu_balloon(ram_addr_t target, MonitorCompletion cb, void *opaque);
>
> -int qemu_balloon_status(MonitorCompletion cb, void *opaque);
> +int qemu_balloon_status(MonitorCompletion cb, void *opaque,
> +                        bool get_cached_data);
>
>   void monitor_print_balloon(Monitor *mon, const QObject *data);
>   int do_info_balloon(Monitor *mon, MonitorCompletion cb, void *opaque);
> diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
> index 9fe3886..68df891 100644
> --- a/hw/virtio-balloon.c
> +++ b/hw/virtio-balloon.c
> @@ -195,7 +195,8 @@ static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f)
>   }
>
>   static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
> -                                     MonitorCompletion cb, void *cb_data)
> +                                     MonitorCompletion cb, void *cb_data,
> +                                     bool get_cached_data)
>   {
>       VirtIOBalloon *dev = opaque;
>
> @@ -213,8 +214,12 @@ static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
>               return;
>           }
>           dev->stats_callback = cb;
> -        dev->stats_opaque_callback_data = cb_data;
> +        dev->stats_opaque_callback_data = cb_data;
>           if (dev->vdev.guest_features&  (1<<  VIRTIO_BALLOON_F_STATS_VQ)) {
> +            if (get_cached_data) {
> +                complete_stats_request(dev);
> +                return;
> +            }
>               virtqueue_push(dev->svq,&dev->stats_vq_elem, dev->stats_vq_offset);
>               virtio_notify(&dev->vdev, dev->svq);
>           } else {
>
Daniel P. Berrange Aug. 23, 2010, 9:24 a.m.
On Sun, Aug 22, 2010 at 04:54:06PM -0500, Anthony Liguori wrote:
> On 08/19/2010 07:48 PM, Amit Shah wrote:
> >If the machine is stopped and 'info balloon' is invoked, the monitor
> >process just hangs waiting for info from the guest. Return the most
> >recent balloon data in that case.
> >
> >See https://bugzilla.redhat.com/show_bug.cgi?id=623903
> >
> >Reported-by:<lihuang@redhat.com>
> >Signed-off-by: Amit Shah<amit.shah@redhat.com>
> >   
> 
> !vm_running is just a special case of an unresponsive guest.  Even if 
> the guest was running, if it was oops'd and the administrator didn't 
> know, you would have the same issue.
> 
> I'd suggest using a timeout based on rt_clock.  If the stats request 
> times out, print an appropriate message to the user.

By all means add a timeout to cope with a unresponsive guest,
but in this case where we can be sure the guest can't respond
there's no point waiting on a timeout.

On a tangent, a future todo item I mentioned to some people at
the summit, is that I'd like to see the interaction with the
balloon driver change/enhanced. It is currently hard/impossible
to use this in an efficient manner. We should never have overloaded
the existing balloon command with guest memstats refresh + data,
because it has made 'query balloon' a significantly slower command
than it was, in particular you can no longer rely on it being a
'fast' response, or even responding at all. This is bad when all
you care about is the balloon actual level, and not the guest 
memstats. I'd like to see something along the lines of:

 - 'query balloon' to return the balloon + memstats values that
   are *currently* known to QEMU. Probably need a new name
   to avoid changing semantics of the existing command

 - 'balloon changed' event to indicate when the balloon actual
   level has changed (so we never need to poll on 'query balloon')

 - 'update memstats' to request an guest kernel mem stats update

 - 'memstats updated' event (or update memstats async reply) to
   indicate when the guest kernel has refreshed the mem stats.

Primarily I'd like to avoid having to invoke anything at all
to get the main balloon driver actual memory level. Being able
to fetch the memstats without also triggering an update is 
fairly useful too though.

Daniel
Amit Shah Aug. 26, 2010, 5:25 a.m.
On (Sun) Aug 22 2010 [16:54:06], Anthony Liguori wrote:
> On 08/19/2010 07:48 PM, Amit Shah wrote:
> >If the machine is stopped and 'info balloon' is invoked, the monitor
> >process just hangs waiting for info from the guest. Return the most
> >recent balloon data in that case.
> >
> >See https://bugzilla.redhat.com/show_bug.cgi?id=623903
> >
> >Reported-by:<lihuang@redhat.com>
> >Signed-off-by: Amit Shah<amit.shah@redhat.com>
> 
> !vm_running is just a special case of an unresponsive guest.  Even
> if the guest was running, if it was oops'd and the administrator
> didn't know, you would have the same issue.

True.

> I'd suggest using a timeout based on rt_clock.  If the stats request
> times out, print an appropriate message to the user.

What should the timeout be?

A heavily-loaded guest could reply later which would cause problems --
I don't think we handle async input from the guest today.

		Amit

Patch hide | download patch | download mbox

diff --git a/balloon.c b/balloon.c
index 8e0b7f1..8b05d20 100644
--- a/balloon.c
+++ b/balloon.c
@@ -43,17 +43,20 @@  void qemu_add_balloon_handler(QEMUBalloonEvent *func, void *opaque)
 int qemu_balloon(ram_addr_t target, MonitorCompletion cb, void *opaque)
 {
     if (qemu_balloon_event) {
-        qemu_balloon_event(qemu_balloon_event_opaque, target, cb, opaque);
+        qemu_balloon_event(qemu_balloon_event_opaque, target, cb, opaque,
+                           false);
         return 1;
     } else {
         return 0;
     }
 }
 
-int qemu_balloon_status(MonitorCompletion cb, void *opaque)
+int qemu_balloon_status(MonitorCompletion cb, void *opaque,
+                        bool get_cached_data)
 {
     if (qemu_balloon_event) {
-        qemu_balloon_event(qemu_balloon_event_opaque, 0, cb, opaque);
+        qemu_balloon_event(qemu_balloon_event_opaque, 0, cb, opaque,
+                           get_cached_data);
         return 1;
     } else {
         return 0;
@@ -113,7 +116,7 @@  int do_info_balloon(Monitor *mon, MonitorCompletion cb, void *opaque)
         return -1;
     }
 
-    ret = qemu_balloon_status(cb, opaque);
+    ret = qemu_balloon_status(cb, opaque, !vm_running);
     if (!ret) {
         qerror_report(QERR_DEVICE_NOT_ACTIVE, "balloon");
         return -1;
diff --git a/balloon.h b/balloon.h
index d478e28..729631c 100644
--- a/balloon.h
+++ b/balloon.h
@@ -17,13 +17,15 @@ 
 #include "monitor.h"
 
 typedef void (QEMUBalloonEvent)(void *opaque, ram_addr_t target,
-                                MonitorCompletion cb, void *cb_data);
+                                MonitorCompletion cb, void *cb_data,
+                                bool get_cached_data);
 
 void qemu_add_balloon_handler(QEMUBalloonEvent *func, void *opaque);
 
 int qemu_balloon(ram_addr_t target, MonitorCompletion cb, void *opaque);
 
-int qemu_balloon_status(MonitorCompletion cb, void *opaque);
+int qemu_balloon_status(MonitorCompletion cb, void *opaque,
+                        bool get_cached_data);
 
 void monitor_print_balloon(Monitor *mon, const QObject *data);
 int do_info_balloon(Monitor *mon, MonitorCompletion cb, void *opaque);
diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 9fe3886..68df891 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -195,7 +195,8 @@  static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f)
 }
 
 static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
-                                     MonitorCompletion cb, void *cb_data)
+                                     MonitorCompletion cb, void *cb_data,
+                                     bool get_cached_data)
 {
     VirtIOBalloon *dev = opaque;
 
@@ -213,8 +214,12 @@  static void virtio_balloon_to_target(void *opaque, ram_addr_t target,
             return;
         }
         dev->stats_callback = cb;
-        dev->stats_opaque_callback_data = cb_data; 
+        dev->stats_opaque_callback_data = cb_data;
         if (dev->vdev.guest_features & (1 << VIRTIO_BALLOON_F_STATS_VQ)) {
+            if (get_cached_data) {
+                complete_stats_request(dev);
+                return;
+            }
             virtqueue_push(dev->svq, &dev->stats_vq_elem, dev->stats_vq_offset);
             virtio_notify(&dev->vdev, dev->svq);
         } else {