[Qemu-devel] [PATCH v2 0/3] Event notifications for balloon driver

[Qemu-devel] [PATCH v2 0/3] Event notifications for balloon driver

[Daniel P. Berrange]

This series is a followup to 2 previously posted patches

 * BALLOON_CHANGE QMP event:
   http://lists.nongnu.org/archive/html/qemu-devel/2012-05/msg02215.html

 * query-events QMP command: 
   http://lists.nongnu.org/archive/html/qemu-devel/2012-05/msg02255.html

Changes since v1:

 - Use a static array of strings for QMP event ID -> string conversion
 - Add BALLOON_CHANGE to qmp-events.txt

There is also a new patch in this series, which introduces the ability
todo simple rate limiting of stateless monitor events. With the ballooning
of a 1.8 GB guest, down to 0.9 GB this reduced the number of events
emitted from ~50 down to just 4, spread across a 4 second time window.
It is important to note that events are only discarded when they are
obsoleted by a newer event. So an application is guarenteed to see the
final balloon event, with at worst a 1 second delay.

This series is being tested with a corresponding patch to libvirt
for handling balloon events

https://www.redhat.com/archives/libvir-list/2012-May/msg00868.html

[Qemu-devel] [PATCH v2 1/3] Add 'query-events' command to QMP to query async events

[Daniel P. Berrange]

From: "Daniel P. Berrange" <berrange@redhat.com>

Sometimes it is neccessary for an application to determine
whether a particular QMP event is available, so they can
decide whether to use compatibility code instead. This
introduces a new 'query-events' command to QMP to do just
that

 { "execute": "query-events" }
 {"return": [{"name": "WAKEUP"},
             {"name": "SUSPEND"},
             {"name": "DEVICE_TRAY_MOVED"},
             {"name": "BLOCK_JOB_CANCELLED"},
             {"name": "BLOCK_JOB_COMPLETED"},
             ...snip...
             {"name": "SHUTDOWN"}]}

* monitor.c: Turn MonitorEvent -> string conversion
  into a lookup from a static table of constant strings.
  Add impl of qmp_query_events monitor command handler
* qapi-schema.json, qmp-commands.hx: Define contract of
  query-events command

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 monitor.c        |  107 +++++++++++++++++++++++-------------------------------
 monitor.h        |    4 ++
 qapi-schema.json |   22 +++++++++++
 qmp-commands.hx  |   37 +++++++++++++++++++
 4 files changed, 108 insertions(+), 62 deletions(-)

diff --git a/monitor.c b/monitor.c
index 12a6fe2..a3bc2c7 100644
--- a/monitor.c
+++ b/monitor.c
@@ -422,6 +422,30 @@ static void timestamp_put(QDict *qdict)
     qdict_put_obj(qdict, "timestamp", obj);
 }
 
+
+static const char *monitor_event_names[] = {
+    [QEVENT_SHUTDOWN] = "SHUTDOWN",
+    [QEVENT_RESET] = "RESET",
+    [QEVENT_POWERDOWN] = "POWERDOWN",
+    [QEVENT_STOP] = "STOP",
+    [QEVENT_RESUME] = "RESUME",
+    [QEVENT_VNC_CONNECTED] = "VNC_CONNECTED",
+    [QEVENT_VNC_INITIALIZED] = "VNC_INITIALIZED",
+    [QEVENT_VNC_DISCONNECTED] = "VNC_DISCONNECTED",
+    [QEVENT_BLOCK_IO_ERROR] = "BLOCK_IO_ERROR",
+    [QEVENT_RTC_CHANGE] = "RTC_CHANGE",
+    [QEVENT_WATCHDOG] = "WATCHDOG",
+    [QEVENT_SPICE_CONNECTED] = "SPICE_CONNECTED",
+    [QEVENT_SPICE_INITIALIZED] = "SPICE_INITIALIZED",
+    [QEVENT_SPICE_DISCONNECTED] = "SPICE_DISCONNECTED",
+    [QEVENT_BLOCK_JOB_COMPLETED] = "BLOCK_JOB_COMPLETED",
+    [QEVENT_BLOCK_JOB_CANCELLED] = "BLOCK_JOB_CANCELLED",
+    [QEVENT_DEVICE_TRAY_MOVED] = "DEVICE_TRAY_MOVED",
+    [QEVENT_SUSPEND] = "SUSPEND",
+    [QEVENT_WAKEUP] = "WAKEUP",
+};
+QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX)
+
 /**
  * monitor_protocol_event(): Generate a Monitor event
  *
@@ -435,68 +459,8 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
 
     assert(event < QEVENT_MAX);
 
-    switch (event) {
-        case QEVENT_SHUTDOWN:
-            event_name = "SHUTDOWN";
-            break;
-        case QEVENT_RESET:
-            event_name = "RESET";
-            break;
-        case QEVENT_POWERDOWN:
-            event_name = "POWERDOWN";
-            break;
-        case QEVENT_STOP:
-            event_name = "STOP";
-            break;
-        case QEVENT_RESUME:
-            event_name = "RESUME";
-            break;
-        case QEVENT_VNC_CONNECTED:
-            event_name = "VNC_CONNECTED";
-            break;
-        case QEVENT_VNC_INITIALIZED:
-            event_name = "VNC_INITIALIZED";
-            break;
-        case QEVENT_VNC_DISCONNECTED:
-            event_name = "VNC_DISCONNECTED";
-            break;
-        case QEVENT_BLOCK_IO_ERROR:
-            event_name = "BLOCK_IO_ERROR";
-            break;
-        case QEVENT_RTC_CHANGE:
-            event_name = "RTC_CHANGE";
-            break;
-        case QEVENT_WATCHDOG:
-            event_name = "WATCHDOG";
-            break;
-        case QEVENT_SPICE_CONNECTED:
-            event_name = "SPICE_CONNECTED";
-            break;
-        case QEVENT_SPICE_INITIALIZED:
-            event_name = "SPICE_INITIALIZED";
-            break;
-        case QEVENT_SPICE_DISCONNECTED:
-            event_name = "SPICE_DISCONNECTED";
-            break;
-        case QEVENT_BLOCK_JOB_COMPLETED:
-            event_name = "BLOCK_JOB_COMPLETED";
-            break;
-        case QEVENT_BLOCK_JOB_CANCELLED:
-            event_name = "BLOCK_JOB_CANCELLED";
-            break;
-        case QEVENT_DEVICE_TRAY_MOVED:
-             event_name = "DEVICE_TRAY_MOVED";
-            break;
-        case QEVENT_SUSPEND:
-            event_name = "SUSPEND";
-            break;
-        case QEVENT_WAKEUP:
-            event_name = "WAKEUP";
-            break;
-        default:
-            abort();
-            break;
-    }
+    event_name = monitor_event_names[event];
+    assert(event_name != NULL);
 
     qmp = qdict_new();
     timestamp_put(qmp);
@@ -738,6 +702,25 @@ CommandInfoList *qmp_query_commands(Error **errp)
     return cmd_list;
 }
 
+EventInfoList *qmp_query_events(Error **errp)
+{
+    EventInfoList *info, *ev_list = NULL;
+    MonitorEvent e;
+
+    for (e = 0 ; e < QEVENT_MAX ; e++) {
+        const char *event_name = monitor_event_names[e];
+        assert(event_name != NULL);
+        info = g_malloc0(sizeof(*info));
+        info->value = g_malloc0(sizeof(*info->value));
+        info->value->name = g_strdup(event_name);
+
+        info->next = ev_list;
+        ev_list = info;
+    }
+
+    return ev_list;
+}
+
 /* set the current CPU defined by the user */
 int monitor_set_cpu(int cpu_index)
 {
diff --git a/monitor.h b/monitor.h
index 0d49800..cd1d878 100644
--- a/monitor.h
+++ b/monitor.h
@@ -41,6 +41,10 @@ typedef enum MonitorEvent {
     QEVENT_DEVICE_TRAY_MOVED,
     QEVENT_SUSPEND,
     QEVENT_WAKEUP,
+
+    /* Add to 'monitor_event_names' array in monitor.c when
+     * defining new events here */
+
     QEVENT_MAX,
 } MonitorEvent;
 
diff --git a/qapi-schema.json b/qapi-schema.json
index 2ca7195..94aa0ae 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -228,6 +228,28 @@
 { 'command': 'query-commands', 'returns': ['CommandInfo'] }
 
 ##
+# @EventInfo:
+#
+# Information about a QMP event
+#
+# @name: The event name
+#
+# Since: 1.2.0
+##
+{ 'type': 'EventInfo', 'data': {'name': 'str'} }
+
+##
+# @query-events:
+#
+# Return a list of supported QMP events by this server
+#
+# Returns: A list of @EventInfo for all supported events
+#
+# Since: 1.2.0
+##
+{ 'command': 'query-events', 'returns': ['EventInfo'] }
+
+##
 # @MigrationStats
 #
 # Detailed migration status.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index db980fa..9c15d3c 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1179,6 +1179,43 @@ EQMP
     },
 
 SQMP
+query-events
+--------------
+
+List QMP available events.
+
+Each event is represented by a json-object, the returned value is a json-array
+of all events.
+
+Each json-object contains:
+
+- "name": event's name (json-string)
+
+Example:
+
+-> { "execute": "query-events" }
+<- {
+      "return":[
+         {
+            "name":"SHUTDOWN"
+         },
+         {
+            "name":"RESET"
+         }
+      ]
+   }
+
+Note: This example has been shortened as the real response is too long.
+
+EQMP
+
+    {
+        .name       = "query-events",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_query_events,
+    },
+
+SQMP
 query-chardev
 -------------
 
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH v2 1/3] Add 'query-events' command to QMP to query async events

[Luiz Capitulino]

On Mon, 21 May 2012 17:59:51 +0100
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> From: "Daniel P. Berrange" <berrange@redhat.com>
> 
> Sometimes it is neccessary for an application to determine
> whether a particular QMP event is available, so they can
> decide whether to use compatibility code instead. This
> introduces a new 'query-events' command to QMP to do just
> that
> 
>  { "execute": "query-events" }
>  {"return": [{"name": "WAKEUP"},
>              {"name": "SUSPEND"},
>              {"name": "DEVICE_TRAY_MOVED"},
>              {"name": "BLOCK_JOB_CANCELLED"},
>              {"name": "BLOCK_JOB_COMPLETED"},
>              ...snip...
>              {"name": "SHUTDOWN"}]}
> 
> * monitor.c: Turn MonitorEvent -> string conversion
>   into a lookup from a static table of constant strings.
>   Add impl of qmp_query_events monitor command handler
> * qapi-schema.json, qmp-commands.hx: Define contract of
>   query-events command
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

Looks good to me. As this independent of the other patches I've applied it
to the qmp-next branch, so you don't need to include this if you respin.

> ---
>  monitor.c        |  107 +++++++++++++++++++++++-------------------------------
>  monitor.h        |    4 ++
>  qapi-schema.json |   22 +++++++++++
>  qmp-commands.hx  |   37 +++++++++++++++++++
>  4 files changed, 108 insertions(+), 62 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 12a6fe2..a3bc2c7 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -422,6 +422,30 @@ static void timestamp_put(QDict *qdict)
>      qdict_put_obj(qdict, "timestamp", obj);
>  }
>  
> +
> +static const char *monitor_event_names[] = {
> +    [QEVENT_SHUTDOWN] = "SHUTDOWN",
> +    [QEVENT_RESET] = "RESET",
> +    [QEVENT_POWERDOWN] = "POWERDOWN",
> +    [QEVENT_STOP] = "STOP",
> +    [QEVENT_RESUME] = "RESUME",
> +    [QEVENT_VNC_CONNECTED] = "VNC_CONNECTED",
> +    [QEVENT_VNC_INITIALIZED] = "VNC_INITIALIZED",
> +    [QEVENT_VNC_DISCONNECTED] = "VNC_DISCONNECTED",
> +    [QEVENT_BLOCK_IO_ERROR] = "BLOCK_IO_ERROR",
> +    [QEVENT_RTC_CHANGE] = "RTC_CHANGE",
> +    [QEVENT_WATCHDOG] = "WATCHDOG",
> +    [QEVENT_SPICE_CONNECTED] = "SPICE_CONNECTED",
> +    [QEVENT_SPICE_INITIALIZED] = "SPICE_INITIALIZED",
> +    [QEVENT_SPICE_DISCONNECTED] = "SPICE_DISCONNECTED",
> +    [QEVENT_BLOCK_JOB_COMPLETED] = "BLOCK_JOB_COMPLETED",
> +    [QEVENT_BLOCK_JOB_CANCELLED] = "BLOCK_JOB_CANCELLED",
> +    [QEVENT_DEVICE_TRAY_MOVED] = "DEVICE_TRAY_MOVED",
> +    [QEVENT_SUSPEND] = "SUSPEND",
> +    [QEVENT_WAKEUP] = "WAKEUP",
> +};
> +QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX)
> +
>  /**
>   * monitor_protocol_event(): Generate a Monitor event
>   *
> @@ -435,68 +459,8 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
>  
>      assert(event < QEVENT_MAX);
>  
> -    switch (event) {
> -        case QEVENT_SHUTDOWN:
> -            event_name = "SHUTDOWN";
> -            break;
> -        case QEVENT_RESET:
> -            event_name = "RESET";
> -            break;
> -        case QEVENT_POWERDOWN:
> -            event_name = "POWERDOWN";
> -            break;
> -        case QEVENT_STOP:
> -            event_name = "STOP";
> -            break;
> -        case QEVENT_RESUME:
> -            event_name = "RESUME";
> -            break;
> -        case QEVENT_VNC_CONNECTED:
> -            event_name = "VNC_CONNECTED";
> -            break;
> -        case QEVENT_VNC_INITIALIZED:
> -            event_name = "VNC_INITIALIZED";
> -            break;
> -        case QEVENT_VNC_DISCONNECTED:
> -            event_name = "VNC_DISCONNECTED";
> -            break;
> -        case QEVENT_BLOCK_IO_ERROR:
> -            event_name = "BLOCK_IO_ERROR";
> -            break;
> -        case QEVENT_RTC_CHANGE:
> -            event_name = "RTC_CHANGE";
> -            break;
> -        case QEVENT_WATCHDOG:
> -            event_name = "WATCHDOG";
> -            break;
> -        case QEVENT_SPICE_CONNECTED:
> -            event_name = "SPICE_CONNECTED";
> -            break;
> -        case QEVENT_SPICE_INITIALIZED:
> -            event_name = "SPICE_INITIALIZED";
> -            break;
> -        case QEVENT_SPICE_DISCONNECTED:
> -            event_name = "SPICE_DISCONNECTED";
> -            break;
> -        case QEVENT_BLOCK_JOB_COMPLETED:
> -            event_name = "BLOCK_JOB_COMPLETED";
> -            break;
> -        case QEVENT_BLOCK_JOB_CANCELLED:
> -            event_name = "BLOCK_JOB_CANCELLED";
> -            break;
> -        case QEVENT_DEVICE_TRAY_MOVED:
> -             event_name = "DEVICE_TRAY_MOVED";
> -            break;
> -        case QEVENT_SUSPEND:
> -            event_name = "SUSPEND";
> -            break;
> -        case QEVENT_WAKEUP:
> -            event_name = "WAKEUP";
> -            break;
> -        default:
> -            abort();
> -            break;
> -    }
> +    event_name = monitor_event_names[event];
> +    assert(event_name != NULL);
>  
>      qmp = qdict_new();
>      timestamp_put(qmp);
> @@ -738,6 +702,25 @@ CommandInfoList *qmp_query_commands(Error **errp)
>      return cmd_list;
>  }
>  
> +EventInfoList *qmp_query_events(Error **errp)
> +{
> +    EventInfoList *info, *ev_list = NULL;
> +    MonitorEvent e;
> +
> +    for (e = 0 ; e < QEVENT_MAX ; e++) {
> +        const char *event_name = monitor_event_names[e];
> +        assert(event_name != NULL);
> +        info = g_malloc0(sizeof(*info));
> +        info->value = g_malloc0(sizeof(*info->value));
> +        info->value->name = g_strdup(event_name);
> +
> +        info->next = ev_list;
> +        ev_list = info;
> +    }
> +
> +    return ev_list;
> +}
> +
>  /* set the current CPU defined by the user */
>  int monitor_set_cpu(int cpu_index)
>  {
> diff --git a/monitor.h b/monitor.h
> index 0d49800..cd1d878 100644
> --- a/monitor.h
> +++ b/monitor.h
> @@ -41,6 +41,10 @@ typedef enum MonitorEvent {
>      QEVENT_DEVICE_TRAY_MOVED,
>      QEVENT_SUSPEND,
>      QEVENT_WAKEUP,
> +
> +    /* Add to 'monitor_event_names' array in monitor.c when
> +     * defining new events here */
> +
>      QEVENT_MAX,
>  } MonitorEvent;
>  
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 2ca7195..94aa0ae 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -228,6 +228,28 @@
>  { 'command': 'query-commands', 'returns': ['CommandInfo'] }
>  
>  ##
> +# @EventInfo:
> +#
> +# Information about a QMP event
> +#
> +# @name: The event name
> +#
> +# Since: 1.2.0
> +##
> +{ 'type': 'EventInfo', 'data': {'name': 'str'} }
> +
> +##
> +# @query-events:
> +#
> +# Return a list of supported QMP events by this server
> +#
> +# Returns: A list of @EventInfo for all supported events
> +#
> +# Since: 1.2.0
> +##
> +{ 'command': 'query-events', 'returns': ['EventInfo'] }
> +
> +##
>  # @MigrationStats
>  #
>  # Detailed migration status.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index db980fa..9c15d3c 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1179,6 +1179,43 @@ EQMP
>      },
>  
>  SQMP
> +query-events
> +--------------
> +
> +List QMP available events.
> +
> +Each event is represented by a json-object, the returned value is a json-array
> +of all events.
> +
> +Each json-object contains:
> +
> +- "name": event's name (json-string)
> +
> +Example:
> +
> +-> { "execute": "query-events" }
> +<- {
> +      "return":[
> +         {
> +            "name":"SHUTDOWN"
> +         },
> +         {
> +            "name":"RESET"
> +         }
> +      ]
> +   }
> +
> +Note: This example has been shortened as the real response is too long.
> +
> +EQMP
> +
> +    {
> +        .name       = "query-events",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_input_query_events,
> +    },
> +
> +SQMP
>  query-chardev
>  -------------
>  

[Qemu-devel] [PATCH v2 2/3] Add event notification for guest balloon changes

[Daniel P. Berrange]

From: "Daniel P. Berrange" <berrange@redhat.com>

After setting a balloon target value, applications have to
continually poll 'query-balloon' to determine whether the
guest has reacted to this request. The virtio-balloon backend
knows exactly when the guest has reacted though, and thus it
is possible to emit a JSON event to tell the mgmt application
whenever the guest balloon changes.

This introduces a new 'qemu_balloon_change()' API which is
to be called by balloon driver backends, whenever they have
a change in balloon value. This takes the 'actual' balloon
value, as would be found in the BalloonInfo struct.

The qemu_balloon_change API emits a JSON monitor event which
looks like:

  {"timestamp": {"seconds": 1337162462, "microseconds": 814521},
   "event": "BALLOON_CHANGE", "data": {"actual": 944766976}}

* balloon.c, balloon.h: Introduce qemu_balloon_change() for
  emitting balloon change events on the monitor
* hw/virtio-balloon.c: Invoke qemu_balloon_change() whenever
  the guest changes the balloon actual value
* monitor.c, monitor.h: Define QEVENT_BALLOON_CHANGE

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 QMP/qmp-events.txt  |   18 ++++++++++++++++++
 balloon.c           |   14 ++++++++++++++
 balloon.h           |    2 ++
 hw/virtio-balloon.c |    5 +++++
 monitor.c           |    1 +
 monitor.h           |    1 +
 6 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
index 9286af5..9ba7079 100644
--- a/QMP/qmp-events.txt
+++ b/QMP/qmp-events.txt
@@ -335,3 +335,21 @@ Example:
                "len": 10737418240, "offset": 134217728,
                "speed": 0 },
      "timestamp": { "seconds": 1267061043, "microseconds": 959568 } }
+
+
+BALLOON_CHANGE
+----------
+
+Emitted when the guest changes the actual BALLOON level. This
+value is equivalent to the 'actual' field return by the
+'query-balloon' command
+
+Data:
+
+- "actual": actual level of the guest memory balloon in bytes (json-number)
+
+Example:
+
+{ "event": "BALLOON_CHANGE",
+    "data": { "actual": 944766976 },
+    "timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
diff --git a/balloon.c b/balloon.c
index aa354f7..913862b 100644
--- a/balloon.c
+++ b/balloon.c
@@ -30,6 +30,7 @@
 #include "balloon.h"
 #include "trace.h"
 #include "qmp-commands.h"
+#include "qjson.h"
 
 static QEMUBalloonEvent *balloon_event_fn;
 static QEMUBalloonStatus *balloon_stat_fn;
@@ -80,6 +81,19 @@ static int qemu_balloon_status(BalloonInfo *info)
     return 1;
 }
 
+void qemu_balloon_change(int64_t actual)
+{
+    QObject *data;
+
+    data = qobject_from_jsonf("{ 'actual': %" PRId64 " }",
+                              actual);
+
+    monitor_protocol_event(QEVENT_BALLOON_CHANGE, data);
+
+    qobject_decref(data);
+}
+
+
 BalloonInfo *qmp_query_balloon(Error **errp)
 {
     BalloonInfo *info;
diff --git a/balloon.h b/balloon.h
index b60fd5d..2ebac0d 100644
--- a/balloon.h
+++ b/balloon.h
@@ -24,4 +24,6 @@ int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
 			     QEMUBalloonStatus *stat_func, void *opaque);
 void qemu_remove_balloon_handler(void *opaque);
 
+void qemu_balloon_change(int64_t actual);
+
 #endif
diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c
index ce9d2c9..9137573 100644
--- a/hw/virtio-balloon.c
+++ b/hw/virtio-balloon.c
@@ -146,8 +146,13 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
 {
     VirtIOBalloon *dev = to_virtio_balloon(vdev);
     struct virtio_balloon_config config;
+    uint32_t oldactual = dev->actual;
     memcpy(&config, config_data, 8);
     dev->actual = le32_to_cpu(config.actual);
+    if (dev->actual != oldactual) {
+        qemu_balloon_change(ram_size -
+                            (dev->actual << VIRTIO_BALLOON_PFN_SHIFT));
+    }
 }
 
 static uint32_t virtio_balloon_get_features(VirtIODevice *vdev, uint32_t f)
diff --git a/monitor.c b/monitor.c
index a3bc2c7..75fd4cf 100644
--- a/monitor.c
+++ b/monitor.c
@@ -443,6 +443,7 @@ static const char *monitor_event_names[] = {
     [QEVENT_DEVICE_TRAY_MOVED] = "DEVICE_TRAY_MOVED",
     [QEVENT_SUSPEND] = "SUSPEND",
     [QEVENT_WAKEUP] = "WAKEUP",
+    [QEVENT_BALLOON_CHANGE] = "BALLOON_CHANGE",
 };
 QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX)
 
diff --git a/monitor.h b/monitor.h
index cd1d878..5f4de1b 100644
--- a/monitor.h
+++ b/monitor.h
@@ -41,6 +41,7 @@ typedef enum MonitorEvent {
     QEVENT_DEVICE_TRAY_MOVED,
     QEVENT_SUSPEND,
     QEVENT_WAKEUP,
+    QEVENT_BALLOON_CHANGE,
 
     /* Add to 'monitor_event_names' array in monitor.c when
      * defining new events here */
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH v2 2/3] Add event notification for guest balloon changes

[Amit Shah]

On (Mon) 21 May 2012 [17:59:52], Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@redhat.com>
> 
> After setting a balloon target value, applications have to
> continually poll 'query-balloon' to determine whether the
> guest has reacted to this request. The virtio-balloon backend
> knows exactly when the guest has reacted though, and thus it
> is possible to emit a JSON event to tell the mgmt application
> whenever the guest balloon changes.
> 
> This introduces a new 'qemu_balloon_change()' API which is

I prefer qemu_balloon_changed(), it is clearer that this is called
after a balloon value change.  qemu_balloon_change() can be taken to
mean the function is called as a response the the monitor 'balloon'
command.

> +BALLOON_CHANGE
> +----------

similarly, this can be BALLOON_CHANGED


		Amit

Re: [Qemu-devel] [PATCH v2 2/3] Add event notification for guest balloon changes

[Daniel P. Berrange]

On Tue, May 22, 2012 at 01:14:59AM +0530, Amit Shah wrote:
> On (Mon) 21 May 2012 [17:59:52], Daniel P. Berrange wrote:
> > From: "Daniel P. Berrange" <berrange@redhat.com>
> > 
> > After setting a balloon target value, applications have to
> > continually poll 'query-balloon' to determine whether the
> > guest has reacted to this request. The virtio-balloon backend
> > knows exactly when the guest has reacted though, and thus it
> > is possible to emit a JSON event to tell the mgmt application
> > whenever the guest balloon changes.
> > 
> > This introduces a new 'qemu_balloon_change()' API which is
> 
> I prefer qemu_balloon_changed(), it is clearer that this is called
> after a balloon value change.  qemu_balloon_change() can be taken to
> mean the function is called as a response the the monitor 'balloon'
> command.

Happy to change this.

> > +BALLOON_CHANGE
> > +----------
> 
> similarly, this can be BALLOON_CHANGED

For the sake of consistency with the existing RTC_CHANGE event, I prefer
the naming I already have.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v2 2/3] Add event notification for guest balloon changes

[Amit Shah]

On (Mon) 21 May 2012 [20:50:55], Daniel P. Berrange wrote:
> On Tue, May 22, 2012 at 01:14:59AM +0530, Amit Shah wrote:
> > On (Mon) 21 May 2012 [17:59:52], Daniel P. Berrange wrote:
> > > From: "Daniel P. Berrange" <berrange@redhat.com>
> > > 
> > > After setting a balloon target value, applications have to
> > > continually poll 'query-balloon' to determine whether the
> > > guest has reacted to this request. The virtio-balloon backend
> > > knows exactly when the guest has reacted though, and thus it
> > > is possible to emit a JSON event to tell the mgmt application
> > > whenever the guest balloon changes.
> > > 
> > > This introduces a new 'qemu_balloon_change()' API which is
> > 
> > I prefer qemu_balloon_changed(), it is clearer that this is called
> > after a balloon value change.  qemu_balloon_change() can be taken to
> > mean the function is called as a response the the monitor 'balloon'
> > command.
> 
> Happy to change this.
> 
> > > +BALLOON_CHANGE
> > > +----------
> > 
> > similarly, this can be BALLOON_CHANGED
> 
> For the sake of consistency with the existing RTC_CHANGE event, I prefer
> the naming I already have.

OK, thinking about it, it seems alright for this to remain
BALLOON_CHANGE, as it's an event that originates from qemu for mgmt
apps, so there isn't ambiguity about it.

Maybe others can chime in?

		Amit

[Qemu-devel] [PATCH v2 3/3] Add rate limiting of RTC_CHANGE, BALLOON_CHANGE & WATCHDOG events

[Daniel P. Berrange]

From: "Daniel P. Berrange" <berrange@redhat.com>

Allow certain event types to be rate limited to avoid flooding
monitor clients. The monitor_protocol_event() method is changed
such that instead of immediately emitting the event to Monitor
instances, it will call a new monitor_protocol_event_queue()
method.

This will check to see if the rate limit for the event has been
exceeded, and if so schedule a timer to wakeup at the end of the
rate limit period. If further events arrive before the timer fires,
the previously queued event will be discarded in favour of the new
event. The event will eventually be emitted when the timer fires.

This logic is applied to RTC_CHANGE, BALLOON_CHANGE & WATCHDOG
events, since the data associated with these events is stateless

 * monitor.c: Add support for rate limiting
 * monitor.h: Define monitor_global_init for one-time setup tasks
 * vl.c: Invoke monitor_global_init
 * trace-events: Add hooks for monitor event tracing

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 monitor.c    |  162 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 monitor.h    |    1 +
 trace-events |    5 ++
 vl.c         |    2 +
 4 files changed, 164 insertions(+), 6 deletions(-)

diff --git a/monitor.c b/monitor.c
index 75fd4cf..5135966 100644
--- a/monitor.c
+++ b/monitor.c
@@ -66,6 +66,7 @@
 #include "memory.h"
 #include "qmp-commands.h"
 #include "hmp.h"
+#include "qemu-thread.h"
 
 /* for pic/irq_info */
 #if defined(TARGET_SPARC)
@@ -145,6 +146,19 @@ typedef struct MonitorControl {
     int command_mode;
 } MonitorControl;
 
+/*
+ * To prevent flooding clients, events can be throttled. The
+ * throttling is calculating globally, rather than per-Monitor
+ * instance.
+ */
+typedef struct MonitorEventState {
+    MonitorEvent event; /* Event being tracked */
+    int64_t rate;       /* Period over which to throttle. 0 to disable */
+    int64_t last;       /* Time at which event was last emitted */
+    QEMUTimer *timer;   /* Timer for handling delayed events */
+    QObject *data;      /* Event pending delayed dispatch */
+} MonitorEventState;
+
 struct Monitor {
     CharDriverState *chr;
     int mux_out;
@@ -447,6 +461,141 @@ static const char *monitor_event_names[] = {
 };
 QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX)
 
+MonitorEventState monitor_event_state[QEVENT_MAX];
+QemuMutex monitor_event_state_lock;
+
+/*
+ * Emits the event to every monitor instance
+ */
+static void
+monitor_protocol_event_emit(MonitorEvent event,
+                            QObject *data)
+{
+    Monitor *mon;
+
+    trace_monitor_protocol_event_emit(event, data);
+    QLIST_FOREACH(mon, &mon_list, entry) {
+        if (monitor_ctrl_mode(mon) && qmp_cmd_mode(mon)) {
+            monitor_json_emitter(mon, data);
+        }
+    }
+}
+
+
+/*
+ * Queue a new event for emission to Monitor instances,
+ * applying any rate limiting if required.
+ */
+static void
+monitor_protocol_event_queue(MonitorEvent event,
+                             QObject *data)
+{
+    MonitorEventState *evstate;
+    int64_t now = qemu_get_clock_ns(rt_clock);
+    assert(event < QEVENT_MAX);
+
+    qemu_mutex_lock(&monitor_event_state_lock);
+    evstate = &(monitor_event_state[event]);
+    trace_monitor_protocol_event_queue(event,
+                                       data,
+                                       evstate->rate,
+                                       evstate->last,
+                                       now);
+
+    /* Rate limit of 0 indicates no throttling */
+    if (!evstate->rate) {
+        monitor_protocol_event_emit(event, data);
+        evstate->last = now;
+    } else {
+        int64_t delta = now - evstate->last;
+        if (evstate->data ||
+            delta < evstate->rate) {
+            /* If there's an existing event pending, replace
+             * it with the new event, otherwise schedule a
+             * timer for delayed emission
+             */
+            if (evstate->data) {
+                qobject_decref(evstate->data);
+            } else {
+                int64_t then = evstate->last + evstate->rate;
+                qemu_mod_timer_ns(evstate->timer, then);
+            }
+            evstate->data = data;
+            qobject_incref(evstate->data);
+        } else {
+            monitor_protocol_event_emit(event, data);
+            evstate->last = now;
+        }
+    }
+    qemu_mutex_unlock(&monitor_event_state_lock);
+}
+
+
+/*
+ * The callback invoked by QemuTimer when a delayed
+ * event is ready to be emitted
+ */
+static void monitor_protocol_event_handler(void *opaque)
+{
+    MonitorEventState *evstate = opaque;
+    int64_t now = qemu_get_clock_ns(rt_clock);
+
+    qemu_mutex_lock(&monitor_event_state_lock);
+
+    trace_monitor_protocol_event_handler(evstate->event,
+                                         evstate->data,
+                                         evstate->last,
+                                         now);
+    if (evstate->data) {
+        monitor_protocol_event_emit(evstate->event, evstate->data);
+        qobject_decref(evstate->data);
+        evstate->data = NULL;
+    }
+    evstate->last = now;
+    qemu_mutex_unlock(&monitor_event_state_lock);
+}
+
+
+/*
+ * @event: the event ID to be limited
+ * @rate: the rate limit in milliseconds
+ *
+ * Sets a rate limit on a particular event, so no
+ * more than 1 event will be emitted within @rate
+ * milliseconds
+ */
+static void
+monitor_protocol_event_throttle(MonitorEvent event,
+                                int64_t rate)
+{
+    MonitorEventState *evstate;
+    assert(event < QEVENT_MAX);
+
+    evstate = &(monitor_event_state[event]);
+
+    trace_monitor_protocol_event_throttle(event, rate);
+    evstate->event = event;
+    evstate->rate = rate * SCALE_MS;
+    evstate->timer = qemu_new_timer(rt_clock,
+                                    SCALE_MS,
+                                    monitor_protocol_event_handler,
+                                    evstate);
+    evstate->last = 0;
+    evstate->data = NULL;
+}
+
+
+/* Global, one-time initializer to configure the rate limiting
+ * and initialize state */
+static void monitor_protocol_event_init(void)
+{
+    qemu_mutex_init(&monitor_event_state_lock);
+    /* Limit RTC & BALLOON events to 1 per second */
+    monitor_protocol_event_throttle(QEVENT_RTC_CHANGE, 1000);
+    monitor_protocol_event_throttle(QEVENT_BALLOON_CHANGE, 1000);
+    monitor_protocol_event_throttle(QEVENT_WATCHDOG, 1000);
+}
+
 /**
  * monitor_protocol_event(): Generate a Monitor event
  *
@@ -456,7 +605,6 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
 {
     QDict *qmp;
     const char *event_name;
-    Monitor *mon;
 
     assert(event < QEVENT_MAX);
 
@@ -471,11 +619,8 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
         qdict_put_obj(qmp, "data", data);
     }
 
-    QLIST_FOREACH(mon, &mon_list, entry) {
-        if (monitor_ctrl_mode(mon) && qmp_cmd_mode(mon)) {
-            monitor_json_emitter(mon, QOBJECT(qmp));
-        }
-    }
+    trace_monitor_protocol_event(event, event_name, qmp);
+    monitor_protocol_event_queue(event, QOBJECT(qmp));
     QDECREF(qmp);
 }
 
@@ -4564,6 +4709,11 @@ static void sortcmdlist(void)
  * End:
  */
 
+void monitor_global_init(void)
+{
+    monitor_protocol_event_init();
+}
+
 void monitor_init(CharDriverState *chr, int flags)
 {
     static int is_first_init = 1;
diff --git a/monitor.h b/monitor.h
index 5f4de1b..3342cb4 100644
--- a/monitor.h
+++ b/monitor.h
@@ -51,6 +51,7 @@ typedef enum MonitorEvent {
 
 int monitor_cur_is_qmp(void);
 
+void monitor_global_init(void);
 void monitor_protocol_event(MonitorEvent event, QObject *data);
 void monitor_init(CharDriverState *chr, int flags);
 
diff --git a/trace-events b/trace-events
index 87cb96c..449b8ee 100644
--- a/trace-events
+++ b/trace-events
@@ -641,6 +641,11 @@ esp_mem_writeb_cmd_ensel(uint32_t val) "Enable selection (%2.2x)"
 # monitor.c
 handle_qmp_command(void *mon, const char *cmd_name) "mon %p cmd_name \"%s\""
 monitor_protocol_emitter(void *mon) "mon %p"
+monitor_protocol_event(uint32_t event, const char *evname, void *data) "event=%d name \"%s\" data %p"
+monitor_protocol_event_handler(uint32_t event, void *data, uint64_t last, uint64_t now) "event=%d data=%p last=%" PRId64 " now=%" PRId64
+monitor_protocol_event_emit(uint32_t event, void *data) "event=%d data=%p"
+monitor_protocol_event_queue(uint32_t event, void *data, uint64_t rate, uint64_t last, uint64_t now) "event=%d data=%p rate=%" PRId64 " last=%" PRId64 " now=%" PRId64
+monitor_protocol_event_throttle(uint32_t event, uint64_t rate) "event=%d rate=%" PRId64
 
 # hw/opencores_eth.c
 open_eth_mii_write(unsigned idx, uint16_t v) "MII[%02x] <- %04x"
diff --git a/vl.c b/vl.c
index 23ab3a3..70eedfa 100644
--- a/vl.c
+++ b/vl.c
@@ -3208,6 +3208,8 @@ int main(int argc, char **argv, char **envp)
     }
     loc_set_none();
 
+    monitor_global_init();
+
     /* Init CPU def lists, based on config
      * - Must be called after all the qemu_read_config_file() calls
      * - Must be called before list_cpus()
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH v2 3/3] Add rate limiting of RTC_CHANGE, BALLOON_CHANGE & WATCHDOG events

[Daniel P. Berrange]

On Wed, May 30, 2012 at 03:50:37PM -0300, Luiz Capitulino wrote:
> On Mon, 21 May 2012 17:59:53 +0100
> "Daniel P. Berrange" <berrange@redhat.com> wrote:

> > +/* Global, one-time initializer to configure the rate limiting
> > + * and initialize state */
> > +static void monitor_protocol_event_init(void)
> > +{
> > +    qemu_mutex_init(&monitor_event_state_lock);
> > +    /* Limit RTC & BALLOON events to 1 per second */
> > +    monitor_protocol_event_throttle(QEVENT_RTC_CHANGE, 1000);
> > +    monitor_protocol_event_throttle(QEVENT_BALLOON_CHANGE, 1000);
> > +    monitor_protocol_event_throttle(QEVENT_WATCHDOG, 1000);
> 
> What about SUSPENDED and BLOCK_IO_ERROR? Couldn't the former be also
> used by a malicious guest to cause a DoS? The former is already emitted
> several times for virtio.

This can't be used to filter BLOCK_IO_ERROR, since that event
contains per-device state information. Filtering this would
need to be done in the block layer, so it can done per device.

I don't think SUSPEND can be used to DoS, since once the VM
is in the suspend state, a monitor command is required to wake
it up again before the guest OS can trigger a new suspend.

> > @@ -4564,6 +4709,11 @@ static void sortcmdlist(void)
> >   * End:
> >   */
> >  
> > +void monitor_global_init(void)
> > +{
> 
> It's better to call it monitor_early_init() (or monitor_init_early()).

Hmm, I chose this name because wanted to make it clear
that this applied to all monitor instances, vs monitor_init
which is per-monitor.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v2 3/3] Add rate limiting of RTC_CHANGE, BALLOON_CHANGE & WATCHDOG events

[Luiz Capitulino]

On Fri, 8 Jun 2012 17:48:56 +0100
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Wed, May 30, 2012 at 03:50:37PM -0300, Luiz Capitulino wrote:
> > On Mon, 21 May 2012 17:59:53 +0100
> > "Daniel P. Berrange" <berrange@redhat.com> wrote:
> 
> > > +/* Global, one-time initializer to configure the rate limiting
> > > + * and initialize state */
> > > +static void monitor_protocol_event_init(void)
> > > +{
> > > +    qemu_mutex_init(&monitor_event_state_lock);
> > > +    /* Limit RTC & BALLOON events to 1 per second */
> > > +    monitor_protocol_event_throttle(QEVENT_RTC_CHANGE, 1000);
> > > +    monitor_protocol_event_throttle(QEVENT_BALLOON_CHANGE, 1000);
> > > +    monitor_protocol_event_throttle(QEVENT_WATCHDOG, 1000);
> > 
> > What about SUSPENDED and BLOCK_IO_ERROR? Couldn't the former be also
> > used by a malicious guest to cause a DoS? The former is already emitted
> > several times for virtio.
> 
> This can't be used to filter BLOCK_IO_ERROR, since that event
> contains per-device state information. Filtering this would
> need to be done in the block layer, so it can done per device.

That's right.

> I don't think SUSPEND can be used to DoS, since once the VM
> is in the suspend state, a monitor command is required to wake
> it up again before the guest OS can trigger a new suspend.

Can't the guest OS awake itself?

Re: [Qemu-devel] [PATCH v2 3/3] Add rate limiting of RTC_CHANGE, BALLOON_CHANGE & WATCHDOG events

[Daniel P. Berrange]

On Mon, Jun 11, 2012 at 02:22:05PM -0300, Luiz Capitulino wrote:
> On Fri, 8 Jun 2012 17:48:56 +0100
> "Daniel P. Berrange" <berrange@redhat.com> wrote:
> 
> > On Wed, May 30, 2012 at 03:50:37PM -0300, Luiz Capitulino wrote:
> > > On Mon, 21 May 2012 17:59:53 +0100
> > > "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > 
> > > > +/* Global, one-time initializer to configure the rate limiting
> > > > + * and initialize state */
> > > > +static void monitor_protocol_event_init(void)
> > > > +{
> > > > +    qemu_mutex_init(&monitor_event_state_lock);
> > > > +    /* Limit RTC & BALLOON events to 1 per second */
> > > > +    monitor_protocol_event_throttle(QEVENT_RTC_CHANGE, 1000);
> > > > +    monitor_protocol_event_throttle(QEVENT_BALLOON_CHANGE, 1000);
> > > > +    monitor_protocol_event_throttle(QEVENT_WATCHDOG, 1000);
> > > 
> > > What about SUSPENDED and BLOCK_IO_ERROR? Couldn't the former be also
> > > used by a malicious guest to cause a DoS? The former is already emitted
> > > several times for virtio.
> > 
> > This can't be used to filter BLOCK_IO_ERROR, since that event
> > contains per-device state information. Filtering this would
> > need to be done in the block layer, so it can done per device.
> 
> That's right.
> 
> > I don't think SUSPEND can be used to DoS, since once the VM
> > is in the suspend state, a monitor command is required to wake
> > it up again before the guest OS can trigger a new suspend.
> 
> Can't the guest OS awake itself?

I didn't think so. Even if it can, we can't rate limit SUSPEND
events in isolation, because they must be strictly ordered
wrt RESUME events.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v2 3/3] Add rate limiting of RTC_CHANGE, BALLOON_CHANGE & WATCHDOG events

[Paolo Bonzini]

Il 13/06/2012 16:53, Daniel P. Berrange ha scritto:
>>> > > I don't think SUSPEND can be used to DoS, since once the VM
>>> > > is in the suspend state, a monitor command is required to wake
>>> > > it up again before the guest OS can trigger a new suspend.
>> > 
>> > Can't the guest OS awake itself?
> I didn't think so. Even if it can, we can't rate limit SUSPEND
> events in isolation, because they must be strictly ordered
> wrt RESUME events.

It can program the RTC to awake the OS, but only at 1 wakeup/second.

Paolo

Re: [Qemu-devel] [PATCH v2 3/3] Add rate limiting of RTC_CHANGE, BALLOON_CHANGE & WATCHDOG events

[Daniel P. Berrange]

On Wed, Jun 13, 2012 at 04:57:28PM +0200, Paolo Bonzini wrote:
> Il 13/06/2012 16:53, Daniel P. Berrange ha scritto:
> >>> > > I don't think SUSPEND can be used to DoS, since once the VM
> >>> > > is in the suspend state, a monitor command is required to wake
> >>> > > it up again before the guest OS can trigger a new suspend.
> >> > 
> >> > Can't the guest OS awake itself?
> > I didn't think so. Even if it can, we can't rate limit SUSPEND
> > events in isolation, because they must be strictly ordered
> > wrt RESUME events.
> 
> It can program the RTC to awake the OS, but only at 1 wakeup/second.

Ah that hardware rate limit is good - we don't need todo separate
throttling with this event then.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v2 3/3] Add rate limiting of RTC_CHANGE, BALLOON_CHANGE & WATCHDOG events

[Paolo Bonzini]

Il 13/06/2012 17:06, Daniel P. Berrange ha scritto:
> On Wed, Jun 13, 2012 at 04:57:28PM +0200, Paolo Bonzini wrote:
>> Il 13/06/2012 16:53, Daniel P. Berrange ha scritto:
>>>>>>> I don't think SUSPEND can be used to DoS, since once the VM
>>>>>>> is in the suspend state, a monitor command is required to wake
>>>>>>> it up again before the guest OS can trigger a new suspend.
>>>>>
>>>>> Can't the guest OS awake itself?
>>> I didn't think so. Even if it can, we can't rate limit SUSPEND
>>> events in isolation, because they must be strictly ordered
>>> wrt RESUME events.
>>
>> It can program the RTC to awake the OS, but only at 1 wakeup/second.
> 
> Ah that hardware rate limit is good - we don't need todo separate
> throttling with this event then.

Hmm, the ACPI PMTimer could be worse though.  Perhaps we could write a
kvm-unit-test and time how many wakeups we can do per second.

Paolo

Re: [Qemu-devel] [PATCH v2 3/3] Add rate limiting of RTC_CHANGE, BALLOON_CHANGE & WATCHDOG events

[Daniel P. Berrange]

On Wed, May 30, 2012 at 03:50:37PM -0300, Luiz Capitulino wrote:
> On Mon, 21 May 2012 17:59:53 +0100
> "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > @@ -4564,6 +4709,11 @@ static void sortcmdlist(void)
> >   * End:
> >   */
> >  
> > +void monitor_global_init(void)
> > +{
> 
> It's better to call it monitor_early_init() (or monitor_init_early()).

This was a bit of stupidity on my part. The existing
'monitor_init' function (which is called per monitor
instance), already has a code block where one-time
global initialization is done. So I'm removing this
function in the next patch posting.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v2 0/3] Event notifications for balloon driver

[Luiz Capitulino]

On Mon, 21 May 2012 17:59:50 +0100
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> This series is a followup to 2 previously posted patches
> 
>  * BALLOON_CHANGE QMP event:
>    http://lists.nongnu.org/archive/html/qemu-devel/2012-05/msg02215.html
> 
>  * query-events QMP command: 
>    http://lists.nongnu.org/archive/html/qemu-devel/2012-05/msg02255.html
> 
> Changes since v1:
> 
>  - Use a static array of strings for QMP event ID -> string conversion
>  - Add BALLOON_CHANGE to qmp-events.txt
> 
> There is also a new patch in this series, which introduces the ability
> todo simple rate limiting of stateless monitor events. With the ballooning
> of a 1.8 GB guest, down to 0.9 GB this reduced the number of events
> emitted from ~50 down to just 4, spread across a 4 second time window.

How would that be with a 1TB guest?

One way of solving this would be to move the policy to the mngt app. that is,
we could have a qmp-event-set-rate-limit command that could be allowed to
be run while in negotiation mode (ie. before qmp_capabilities is executed).

But I'm honestly not sure if rate limit is the best solution for this problem...
How can several events spread in several seconds be useful to libvirt?

IMO, the best would be to have a way to know when the balloon driver is done
servicing a balloon request.

Amit, is this possible?

Re: [Qemu-devel] [PATCH v2 0/3] Event notifications for balloon driver

[Daniel P. Berrange]

On Tue, May 22, 2012 at 05:55:30PM -0300, Luiz Capitulino wrote:
> On Mon, 21 May 2012 17:59:50 +0100
> "Daniel P. Berrange" <berrange@redhat.com> wrote:
> 
> > This series is a followup to 2 previously posted patches
> > 
> >  * BALLOON_CHANGE QMP event:
> >    http://lists.nongnu.org/archive/html/qemu-devel/2012-05/msg02215.html
> > 
> >  * query-events QMP command: 
> >    http://lists.nongnu.org/archive/html/qemu-devel/2012-05/msg02255.html
> > 
> > Changes since v1:
> > 
> >  - Use a static array of strings for QMP event ID -> string conversion
> >  - Add BALLOON_CHANGE to qmp-events.txt
> > 
> > There is also a new patch in this series, which introduces the ability
> > todo simple rate limiting of stateless monitor events. With the ballooning
> > of a 1.8 GB guest, down to 0.9 GB this reduced the number of events
> > emitted from ~50 down to just 4, spread across a 4 second time window.
> 
> How would that be with a 1TB guest?

Sure it would take longer but I don't see that as a problem, provided
the updates are not too frequent, which rate limiting ensures.

> One way of solving this would be to move the policy to the mngt app. that is,
> we could have a qmp-event-set-rate-limit command that could be allowed to
> be run while in negotiation mode (ie. before qmp_capabilities is executed).

Yep, I considered doing this, but to be honest I don't think we need that
kind of granularity. Even if we had this command, we'd just unconditionally
set the rate limit to 1 second & be done with it.

> But I'm honestly not sure if rate limit is the best solution for this problem...
> How can several events spread in several seconds be useful to libvirt?

Basically at any point in time, libvirt wants to know what the current
balloon value is. We don't care whether it has "completed" or not, because
given a malicious guest it is entirely likely that completion may never
come, or indeed it may never balloon at all. Thus all we care about is
what the current value is, mgmt apps can then decide how/whether to enforce
this by setting cgroup limits.  Even without event notifications, all
libvirt cares about is the current level, as queried by 'query-balloon',
not any kind of completion. So the fact that events are progressively
spread over several seconds is in fact entirely desirable to us. THis
simply means we can avoid running 'query-balloon' and always have the
current balloon value accurate to approx 1 second.

> IMO, the best would be to have a way to know when the balloon driver is done
> servicing a balloon request.

As inferred above, IMHO, focusing on completion of ballooning is a bad
thing todo. Apps should only ever care about what the current balloon
level is.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v2 0/3] Event notifications for balloon driver

[Luiz Capitulino]

On Wed, 23 May 2012 11:35:42 +0100
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> > One way of solving this would be to move the policy to the mngt app. that is,
> > we could have a qmp-event-set-rate-limit command that could be allowed to
> > be run while in negotiation mode (ie. before qmp_capabilities is executed).
> 
> Yep, I considered doing this, but to be honest I don't think we need that
> kind of granularity. Even if we had this command, we'd just unconditionally
> set the rate limit to 1 second & be done with it.

Ok, and thinking about it again that's something we could add later on if
needed.

> > But I'm honestly not sure if rate limit is the best solution for this problem...
> > How can several events spread in several seconds be useful to libvirt?
> 
> Basically at any point in time, libvirt wants to know what the current
> balloon value is. We don't care whether it has "completed" or not, because
> given a malicious guest it is entirely likely that completion may never
> come, or indeed it may never balloon at all. Thus all we care about is
> what the current value is, mgmt apps can then decide how/whether to enforce
> this by setting cgroup limits.  Even without event notifications, all
> libvirt cares about is the current level, as queried by 'query-balloon',
> not any kind of completion. So the fact that events are progressively
> spread over several seconds is in fact entirely desirable to us. THis
> simply means we can avoid running 'query-balloon' and always have the
> current balloon value accurate to approx 1 second.

Okay, that's reasonable to me. The balloon event is just a hint then (as events
are supposed to be) and it does reflect the balloon state in the VM at the time
it's emitted.

I'd have to review the series in more detail, but in principle seems good.

Anthony, do you have objections?