[Qemu-devel] [PATCH V2] balloon: Don't try fetching info if machine is stopped

[Qemu-devel] [PATCH V2] balloon: Don't try fetching info if machine is stopped

[Amit Shah]

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 {
-- 
1.7.2.1

[Qemu-devel] Re: [PATCH V2] balloon: Don't try fetching info if machine is stopped

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH V2] balloon: Don't try fetching info if machine is stopped

[Amit Shah]

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

Re: [Qemu-devel] [PATCH V2] balloon: Don't try fetching info if machine is stopped

[Anthony Liguori]

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 {
>    

Re: [Qemu-devel] [PATCH V2] balloon: Don't try fetching info if machine is stopped

[Daniel P. Berrange]

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
-- 
|: 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 :|

Re: [Qemu-devel] [PATCH V2] balloon: Don't try fetching info if machine is stopped

[Amit Shah]

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

Re: [Qemu-devel] [PATCH V2] balloon: Don't try fetching info if machine is stopped

[Amit Shah]

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.
> 
> I'd suggest using a timeout based on rt_clock.  If the stats request
> times out, print an appropriate message to the user.

This is what I have currently. It would need some timer handling in the
save/load case as well, right?

		Amit

>From efe4a36423d7dec1aa9b142ac14c82c2dc80abe4 Mon Sep 17 00:00:00 2001
Message-Id: <efe4a36423d7dec1aa9b142ac14c82c2dc80abe4.1282802628.git.amit.shah@redhat.com>
From: Amit Shah <amit.shah@redhat.com>
Date: Thu, 26 Aug 2010 11:17:14 +0530
Subject: [PATCH] balloon: Don't try fetching info if guest is unresponsive

If the guest is unresponsive 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.

A new timer is added, which on expiry, just presents the old data to the
monitor callback functions.

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

Reported-by: <lihuang@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-balloon.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index 9fe3886..1ec03b3 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -40,6 +40,7 @@ typedef struct VirtIOBalloon
     size_t stats_vq_offset;
     MonitorCompletion *stats_callback;
     void *stats_opaque_callback_data;
+    QEMUTimer *timer;
 } VirtIOBalloon;
 
 static VirtIOBalloon *to_virtio_balloon(VirtIODevice *vdev)
@@ -137,6 +138,11 @@ static void complete_stats_request(VirtIOBalloon *vb)
     vb->stats_callback = NULL;
 }
 
+static void stats_timer_expired(void *opaque)
+{
+    complete_stats_request(opaque);
+}
+
 static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIOBalloon *s = DO_UPCAST(VirtIOBalloon, vdev, vdev);
@@ -148,6 +154,8 @@ static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
         return;
     }
 
+    qemu_del_timer(s->timer);
+
     /* Initialize the stats to get rid of any stale values.  This is only
      * needed to handle the case where a guest supports fewer stats than it
      * used to (ie. it has booted into an old kernel).
@@ -215,6 +223,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)) {
+            qemu_mod_timer(dev->timer, qemu_get_clock(rt_clock) + 5000);
             virtqueue_push(dev->svq, &dev->stats_vq_elem, dev->stats_vq_offset);
             virtio_notify(&dev->vdev, dev->svq);
         } else {
@@ -267,6 +276,8 @@ VirtIODevice *virtio_balloon_init(DeviceState *dev)
     s->dvq = virtio_add_queue(&s->vdev, 128, virtio_balloon_handle_output);
     s->svq = virtio_add_queue(&s->vdev, 128, virtio_balloon_receive_stats);
 
+    s->timer = qemu_new_timer(rt_clock, stats_timer_expired, s);
+
     reset_stats(s);
     qemu_add_balloon_handler(virtio_balloon_to_target, s);
 
-- 
1.7.2.2

Re: [Qemu-devel] [PATCH V2] balloon: Don't try fetching info if machine is stopped

[Paolo Bonzini]

On 08/26/2010 08:05 AM, Amit Shah wrote:
> This is what I have currently. It would need some timer handling in
> the save/load case as well, right?

When loading you won't have any pending "info balloon" command, so I 
think the timer need not be preserved across migration.

Also, 5 seconds for a stopped guest is actually a lot, so maybe Amit's 
original patch or a variant thereof would make sense anyway.

Paolo

Re: [Qemu-devel] [PATCH V2] balloon: Don't try fetching info if machine is stopped

[Daniel P. Berrange]

On Thu, Aug 26, 2010 at 10:05:44AM +0200, Paolo Bonzini wrote:
> On 08/26/2010 08:05 AM, Amit Shah wrote:
> >This is what I have currently. It would need some timer handling in
> >the save/load case as well, right?
> 
> When loading you won't have any pending "info balloon" command, so I 
> think the timer need not be preserved across migration.
> 
> Also, 5 seconds for a stopped guest is actually a lot, so maybe Amit's 
> original patch or a variant thereof would make sense anyway.

We should have a combination of both. If we know the guest is stopped
we should return immediately, otherwise we should use the timer as a
way to cope with a crashed/evil guest.

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 :|

Re: [Qemu-devel] [PATCH V2] balloon: Don't try fetching info if machine is stopped

[Anthony Liguori]

On 08/26/2010 03:14 AM, Daniel P. Berrange wrote:
> On Thu, Aug 26, 2010 at 10:05:44AM +0200, Paolo Bonzini wrote:
>    
>> On 08/26/2010 08:05 AM, Amit Shah wrote:
>>      
>>> This is what I have currently. It would need some timer handling in
>>> the save/load case as well, right?
>>>        
>> When loading you won't have any pending "info balloon" command, so I
>> think the timer need not be preserved across migration.
>>
>> Also, 5 seconds for a stopped guest is actually a lot, so maybe Amit's
>> original patch or a variant thereof would make sense anyway.
>>      
> We should have a combination of both. If we know the guest is stopped
> we should return immediately, otherwise we should use the timer as a
> way to cope with a crashed/evil guest.
>    

Stopped doesn't necessarily mean that it's permanently stopped or even 
that a user has stopped it.

We stop a guest during live migration and in some other cases (like on 
disk error).

Returning immediately is an optimization on something that should be a 
proper fix.  Otherwise, you have a guest initiated DoS attack on 
management tools.

Regards,

Anthony Liguori

> Daniel
>    

Re: [Qemu-devel] [PATCH V2] balloon: Don't try fetching info if machine is stopped

[Amit Shah]

On (Thu) Aug 26 2010 [10:05:44], Paolo Bonzini wrote:
> On 08/26/2010 08:05 AM, Amit Shah wrote:
> >This is what I have currently. It would need some timer handling in
> >the save/load case as well, right?
> 
> When loading you won't have any pending "info balloon" command, so I
> think the timer need not be preserved across migration.
> 
> Also, 5 seconds for a stopped guest is actually a lot,

That's the problem; it's policy. Where and how to specify it?

> so maybe
> Amit's original patch or a variant thereof would make sense anyway.

This seems to be needed though -- as Anthony mentioned, a guest which
has oopsed or similar, incapable of servicing the stats request, is
going to block the monitor command from returning forever.

So it's better to have a timeout, just that we need to decide how much
it should be.

		Amit

Re: [Qemu-devel] [PATCH V2] balloon: Don't try fetching info if machine is stopped

[Paolo Bonzini]

On 08/26/2010 10:17 AM, Amit Shah wrote:
>> >
>> >  Also, 5 seconds for a stopped guest is actually a lot,
> That's the problem; it's policy. Where and how to specify it?

For a crashed/oopsed guest even 10 seconds may be okay, as long as it's 
0 for a stopped guest.  We need both patches.

Paolo

Re: [Qemu-devel] [PATCH V2] balloon: Don't try fetching info if machine is stopped

[Daniel P. Berrange]

On Thu, Aug 26, 2010 at 01:47:50PM +0530, Amit Shah wrote:
> On (Thu) Aug 26 2010 [10:05:44], Paolo Bonzini wrote:
> > On 08/26/2010 08:05 AM, Amit Shah wrote:
> > >This is what I have currently. It would need some timer handling in
> > >the save/load case as well, right?
> > 
> > When loading you won't have any pending "info balloon" command, so I
> > think the timer need not be preserved across migration.
> > 
> > Also, 5 seconds for a stopped guest is actually a lot,
> 
> That's the problem; it's policy. Where and how to specify it?

It is unfortunate that this is policy, but we just have to accept
that the current query-balloon command is a flawed design. IMHO
we  should just hardcode the timeout at 5 seconds as you do (plus
immediate return for paused guests). Then focus on adding new 
monitor commands/events to deal with balloon query in a way 
that doesn't require this kind of policy in QEMU, and deprecate 
the existing query-balloon command.

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 :|

Re: [Qemu-devel] [PATCH V2] balloon: Don't try fetching info if machine is stopped

[Luiz Capitulino]

On Thu, 26 Aug 2010 09:28:42 +0100
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Thu, Aug 26, 2010 at 01:47:50PM +0530, Amit Shah wrote:
> > On (Thu) Aug 26 2010 [10:05:44], Paolo Bonzini wrote:
> > > On 08/26/2010 08:05 AM, Amit Shah wrote:
> > > >This is what I have currently. It would need some timer handling in
> > > >the save/load case as well, right?
> > > 
> > > When loading you won't have any pending "info balloon" command, so I
> > > think the timer need not be preserved across migration.
> > > 
> > > Also, 5 seconds for a stopped guest is actually a lot,
> > 
> > That's the problem; it's policy. Where and how to specify it?
> 
> It is unfortunate that this is policy, but we just have to accept
> that the current query-balloon command is a flawed design. IMHO
> we  should just hardcode the timeout at 5 seconds as you do (plus
> immediate return for paused guests). Then focus on adding new 
> monitor commands/events to deal with balloon query in a way 
> that doesn't require this kind of policy in QEMU, and deprecate 
> the existing query-balloon command.

Agreed, but it's not just that: we've never correctly specified how
commands that talk with the guest should behave.

  *brain dump warning*

We were talking about making all commands work as synchronous and
asynchronous. If we do that, then we'll need a 'global' timeout
for all synchronous commands. We could have a default value and a
command to set it.

 *brain dump warning ends*

I really don't know what to do 0.13. Probably the hard-coded timer is
the best solution we have, but I'm wondering if it's going to cause
problems in the near future, when we get proper asynchronous command
support.

Re: [Qemu-devel] [PATCH V2] balloon: Don't try fetching info if machine is stopped

[Paolo Bonzini]

On 08/26/2010 02:57 PM, Luiz Capitulino wrote:
> I really don't know what to do 0.13. Probably the hard-coded timer is
> the best solution we have, but I'm wondering if it's going to cause
> problems in the near future, when we get proper asynchronous command
> support.

Just make it a different command, or make it dependent on whether the 
initial handshaking activated the asynchronous command capability.

Paolo