[Qemu-devel] [PATCH 0/7] Throttle event VSERPORT_CHANGE separately by "id"

[Qemu-devel] [PATCH 0/7] Throttle event VSERPORT_CHANGE separately by "id"

[Markus Armbruster]

VSERPORT_CHANGE is emitted when the guest opens or closes a
virtio-serial port.  The event's member "id" identifies the port.

When several events arrive quickly, throttling drops all but the last
of them.  Because of that, a QMP client must assume that *any* port
may have changed state when it receives a VSERPORT_CHANGE event and
throttling may have happened.

Make the event more useful by throttling it for each port separately.

Marc-André recently posted 'monitor: throttle VSERPORT_CHANGED by
"id"' to do the same thing.  Why am I redoing his work?  Let me
explain.

In master, event throttling works per QAPIEvent.  Throttling state is
kept in MonitorQAPIEventState monitor_qapi_event_state[].  Going from
event to throttling state is a simple array subscript.

Marc-André's series makes the code governing throttling pluggable per
event.  MonitorQAPIEventState gets two new members, the governor
function and an opaque.  It loses the actual throttling state.  That
one's now in new type MonitorQAPIEventPending.

The standard governor has the opaque point to a single
MonitorQAPIEventPending.

The governor for VSERPORT_CHANGED has the opaque point to a hash table
mapping "id" to MonitorQAPIEventPending.

In my review, I challenged the complexity of this solution, but
Marc-André thinks it's justified.  That leaves me two choices: concede
the point, or come up with something that's actually simpler.

My solution doesn't make anything pluggable.  Instead, I simply
generalize the map from event to throttling state.  Instead of using
just the QAPIEvent as key for looking up state, I permit additional
use of event-specific data.  For VSERPORT_CHANGED, I additionally use
"id".  monitor_qapi_event_state[] becomes a hash table.

No callbacks, no type-punning, less code, and fewer code paths to
test.

PATCH 1-3 clean up the code some before I start.  PATCH 2 may fix
bugs, but to be sure I'd have to think some more about the old code.

PATCH 4-6 do the actual work.

PATCH 7 fixes docs to mention throttling.

Diffstat for monitor.c
    Marc-André's series: 201 insertions(+), 55 deletions(-)
    mine:                115 insertions(+), 78 deletions(-)

Here's an easy way to test this: run QEMU with four virtserial ports:

    -device virtio-serial,id=vser \
    -device virtserialport,id=vser1,nr=1 \
    -device virtserialport,id=vser2,nr=2 \
    -device virtserialport,id=vser3,nr=3 \
    -device virtserialport,id=vser4,nr=4

Connect to QMP so you can observe events.  Boot a Linux guest.  Run
something like

    # for ((i=0; i<100; i++)); do cat /dev/vport0p$((i % 4 + 1)); done

You'll get four VSERPORT_CHANGED events (one for each of the four
ports) immediately, and another four after a second.

Before this series, you'll get only two.

Markus Armbruster (7):
  monitor: Reduce casting of QAPI event QDict
  monitor: Simplify event throttling
  monitor: Switch from timer_new() to timer_new_ns()
  monitor: Split MonitorQAPIEventConf off MonitorQAPIEventState
  monitor: Turn monitor_qapi_event_state[] into a hash table
  monitor: Throttle event VSERPORT_CHANGE separately by "id"
  docs: Document QMP event rate limiting

 docs/qmp-events.txt |  12 ++++
 docs/qmp-spec.txt   |   5 ++
 monitor.c           | 192 ++++++++++++++++++++++++++++++----------------------
 trace-events        |   4 +-
 4 files changed, 131 insertions(+), 82 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/7] monitor: Reduce casting of QAPI event QDict

[Markus Armbruster]

Make the variables holding the event QDict instead of QObject.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 monitor.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/monitor.c b/monitor.c
index 4f1ba2f..2fa90ca 100644
--- a/monitor.c
+++ b/monitor.c
@@ -185,7 +185,7 @@ typedef struct MonitorQAPIEventState {
     int64_t rate;       /* Minimum time (in ns) between two events */
     int64_t last;       /* QEMU_CLOCK_REALTIME value at last emission */
     QEMUTimer *timer;   /* Timer for handling delayed events */
-    QObject *data;      /* Event pending delayed dispatch */
+    QDict *qdict;       /* Delayed event (if any) */
 } MonitorQAPIEventState;
 
 struct Monitor {
@@ -444,14 +444,14 @@ static MonitorQAPIEventState monitor_qapi_event_state[QAPI_EVENT_MAX];
  * Emits the event to every monitor instance, @event is only used for trace
  * Called with monitor_lock held.
  */
-static void monitor_qapi_event_emit(QAPIEvent event, QObject *data)
+static void monitor_qapi_event_emit(QAPIEvent event, QDict *qdict)
 {
     Monitor *mon;
 
-    trace_monitor_protocol_event_emit(event, data);
+    trace_monitor_protocol_event_emit(event, qdict);
     QLIST_FOREACH(mon, &mon_list, entry) {
         if (monitor_is_qmp(mon) && mon->qmp.in_command_mode) {
-            monitor_json_emitter(mon, data);
+            monitor_json_emitter(mon, QOBJECT(qdict));
         }
     }
 }
@@ -461,7 +461,7 @@ static void monitor_qapi_event_emit(QAPIEvent event, QObject *data)
  * applying any rate limiting if required.
  */
 static void
-monitor_qapi_event_queue(QAPIEvent event, QDict *data, Error **errp)
+monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp)
 {
     MonitorQAPIEventState *evstate;
     assert(event < QAPI_EVENT_MAX);
@@ -469,7 +469,7 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *data, Error **errp)
 
     evstate = &(monitor_qapi_event_state[event]);
     trace_monitor_protocol_event_queue(event,
-                                       data,
+                                       qdict,
                                        evstate->rate,
                                        evstate->last,
                                        now);
@@ -477,26 +477,26 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *data, Error **errp)
     /* Rate limit of 0 indicates no throttling */
     qemu_mutex_lock(&monitor_lock);
     if (!evstate->rate) {
-        monitor_qapi_event_emit(event, QOBJECT(data));
+        monitor_qapi_event_emit(event, qdict);
         evstate->last = now;
     } else {
         int64_t delta = now - evstate->last;
-        if (evstate->data ||
+        if (evstate->qdict ||
             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);
+            if (evstate->qdict) {
+                QDECREF(evstate->qdict);
             } else {
                 int64_t then = evstate->last + evstate->rate;
                 timer_mod_ns(evstate->timer, then);
             }
-            evstate->data = QOBJECT(data);
-            qobject_incref(evstate->data);
+            evstate->qdict = qdict;
+            QINCREF(evstate->qdict);
         } else {
-            monitor_qapi_event_emit(event, QOBJECT(data));
+            monitor_qapi_event_emit(event, qdict);
             evstate->last = now;
         }
     }
@@ -513,14 +513,14 @@ static void monitor_qapi_event_handler(void *opaque)
     int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
 
     trace_monitor_protocol_event_handler(evstate->event,
-                                         evstate->data,
+                                         evstate->qdict,
                                          evstate->last,
                                          now);
     qemu_mutex_lock(&monitor_lock);
-    if (evstate->data) {
-        monitor_qapi_event_emit(evstate->event, evstate->data);
-        qobject_decref(evstate->data);
-        evstate->data = NULL;
+    if (evstate->qdict) {
+        monitor_qapi_event_emit(evstate->event, evstate->qdict);
+        QDECREF(evstate->qdict);
+        evstate->qdict = NULL;
     }
     evstate->last = now;
     qemu_mutex_unlock(&monitor_lock);
@@ -547,7 +547,7 @@ monitor_qapi_event_throttle(QAPIEvent event, int64_t rate)
     assert(rate * SCALE_MS <= INT64_MAX);
     evstate->rate = rate * SCALE_MS;
     evstate->last = 0;
-    evstate->data = NULL;
+    evstate->qdict = NULL;
     evstate->timer = timer_new(QEMU_CLOCK_REALTIME,
                                SCALE_MS,
                                monitor_qapi_event_handler,
-- 
2.4.3

[Qemu-devel] [PATCH 2/7] monitor: Simplify event throttling

[Markus Armbruster]

The event throttling state machine is hard to understand.  I'm not
sure it's entirely correct.  Rewrite it in a more straightforward
manner:

State 1: No event sent recently (less than evconf->rate ns ago)

    Invariant: evstate->timer is not pending, evstate->qdict is null

    On event: send event, arm timer, goto state 2

State 2: Event sent recently, no additional event being delayed

    Invariant: evstate->timer is pending, evstate->qdict is null

    On event: store it in evstate->qdict, goto state 3

    On timer: goto state 1

State 3: Event sent recently, additional event being delayed

    Invariant: evstate->timer is pending, evstate->qdict is non-null

    On event: store it in evstate->qdict, goto state 3

    On timer: send evstate->qdict, clear evstate->qdict,
              arm timer, goto state 2

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c    | 63 +++++++++++++++++++++++++++++-------------------------------
 trace-events |  4 ++--
 2 files changed, 32 insertions(+), 35 deletions(-)

diff --git a/monitor.c b/monitor.c
index 2fa90ca..ca201fd 100644
--- a/monitor.c
+++ b/monitor.c
@@ -183,7 +183,6 @@ typedef struct {
 typedef struct MonitorQAPIEventState {
     QAPIEvent event;    /* Event being tracked */
     int64_t rate;       /* Minimum time (in ns) between two events */
-    int64_t last;       /* QEMU_CLOCK_REALTIME value at last emission */
     QEMUTimer *timer;   /* Timer for handling delayed events */
     QDict *qdict;       /* Delayed event (if any) */
 } MonitorQAPIEventState;
@@ -464,65 +463,64 @@ static void
 monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp)
 {
     MonitorQAPIEventState *evstate;
+
     assert(event < QAPI_EVENT_MAX);
-    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
+    evstate = &monitor_qapi_event_state[event];
+    trace_monitor_protocol_event_queue(event, qdict, evstate->rate);
 
-    evstate = &(monitor_qapi_event_state[event]);
-    trace_monitor_protocol_event_queue(event,
-                                       qdict,
-                                       evstate->rate,
-                                       evstate->last,
-                                       now);
-
-    /* Rate limit of 0 indicates no throttling */
     qemu_mutex_lock(&monitor_lock);
+
     if (!evstate->rate) {
+        /* Unthrottled event */
         monitor_qapi_event_emit(event, qdict);
-        evstate->last = now;
     } else {
-        int64_t delta = now - evstate->last;
-        if (evstate->qdict ||
-            delta < evstate->rate) {
-            /* If there's an existing event pending, replace
-             * it with the new event, otherwise schedule a
-             * timer for delayed emission
+        if (timer_pending(evstate->timer)) {
+            /*
+             * Timer is pending for (at least) evstate->rate ns after
+             * last send.  Store event for sending when timer fires,
+             * replacing a prior stored event if any.
              */
-            if (evstate->qdict) {
-                QDECREF(evstate->qdict);
-            } else {
-                int64_t then = evstate->last + evstate->rate;
-                timer_mod_ns(evstate->timer, then);
-            }
+            QDECREF(evstate->qdict);
             evstate->qdict = qdict;
             QINCREF(evstate->qdict);
         } else {
+            /*
+             * Last send was (at least) evstate->rate ns ago.
+             * Send immediately, and arm the timer to call
+             * monitor_qapi_event_handler() in evstate->rate ns.  Any
+             * events arriving before then will be delayed until then.
+             */
+            int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
+
             monitor_qapi_event_emit(event, qdict);
-            evstate->last = now;
+            timer_mod_ns(evstate->timer, now + evstate->rate);
         }
     }
+
     qemu_mutex_unlock(&monitor_lock);
 }
 
 /*
- * The callback invoked by QemuTimer when a delayed
- * event is ready to be emitted
+ * This function runs evstate->rate ns after sending a throttled
+ * event.
+ * If another event has since been stored, send it.
  */
 static void monitor_qapi_event_handler(void *opaque)
 {
     MonitorQAPIEventState *evstate = opaque;
-    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
 
-    trace_monitor_protocol_event_handler(evstate->event,
-                                         evstate->qdict,
-                                         evstate->last,
-                                         now);
+    trace_monitor_protocol_event_handler(evstate->event, evstate->qdict);
     qemu_mutex_lock(&monitor_lock);
+
     if (evstate->qdict) {
+        int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
+
         monitor_qapi_event_emit(evstate->event, evstate->qdict);
         QDECREF(evstate->qdict);
         evstate->qdict = NULL;
+        timer_mod_ns(evstate->timer, now + evstate->rate);
     }
-    evstate->last = now;
+
     qemu_mutex_unlock(&monitor_lock);
 }
 
@@ -546,7 +544,6 @@ monitor_qapi_event_throttle(QAPIEvent event, int64_t rate)
     evstate->event = event;
     assert(rate * SCALE_MS <= INT64_MAX);
     evstate->rate = rate * SCALE_MS;
-    evstate->last = 0;
     evstate->qdict = NULL;
     evstate->timer = timer_new(QEMU_CLOCK_REALTIME,
                                SCALE_MS,
diff --git a/trace-events b/trace-events
index a0ddc6b..7fddc77 100644
--- a/trace-events
+++ b/trace-events
@@ -1031,9 +1031,9 @@ esp_pci_sbac_write(uint32_t reg, uint32_t val) "sbac: 0x%8.8x -> 0x%8.8x"
 # 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_handler(uint32_t event, void *data, uint64_t last, uint64_t now) "event=%d data=%p last=%" PRId64 " now=%" PRId64
+monitor_protocol_event_handler(uint32_t event, void *qdict) "event=%d data=%p"
 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_queue(uint32_t event, void *qdict, uint64_t rate) "event=%d data=%p rate=%" PRId64
 monitor_protocol_event_throttle(uint32_t event, uint64_t rate) "event=%d rate=%" PRId64
 
 # hw/net/opencores_eth.c
-- 
2.4.3

Re: [Qemu-devel] [PATCH 2/7] monitor: Simplify event throttling

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1348 bytes --]

On 10/15/2015 09:08 AM, Markus Armbruster wrote:
> The event throttling state machine is hard to understand.  I'm not
> sure it's entirely correct.  Rewrite it in a more straightforward
> manner:
> 
> State 1: No event sent recently (less than evconf->rate ns ago)
> 
>     Invariant: evstate->timer is not pending, evstate->qdict is null
> 
>     On event: send event, arm timer, goto state 2
> 
> State 2: Event sent recently, no additional event being delayed
> 
>     Invariant: evstate->timer is pending, evstate->qdict is null
> 
>     On event: store it in evstate->qdict, goto state 3
> 
>     On timer: goto state 1
> 
> State 3: Event sent recently, additional event being delayed
> 
>     Invariant: evstate->timer is pending, evstate->qdict is non-null
> 
>     On event: store it in evstate->qdict, goto state 3
> 
>     On timer: send evstate->qdict, clear evstate->qdict,
>               arm timer, goto state 2
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c    | 63 +++++++++++++++++++++++++++++-------------------------------
>  trace-events |  4 ++--
>  2 files changed, 32 insertions(+), 35 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 3/7] monitor: Switch from timer_new() to timer_new_ns()

[Markus Armbruster]

We don't actually care for the scale, so we can just as well use the
simpler interface.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index ca201fd..baffd76 100644
--- a/monitor.c
+++ b/monitor.c
@@ -545,10 +545,9 @@ monitor_qapi_event_throttle(QAPIEvent event, int64_t rate)
     assert(rate * SCALE_MS <= INT64_MAX);
     evstate->rate = rate * SCALE_MS;
     evstate->qdict = NULL;
-    evstate->timer = timer_new(QEMU_CLOCK_REALTIME,
-                               SCALE_MS,
-                               monitor_qapi_event_handler,
-                               evstate);
+    evstate->timer = timer_new_ns(QEMU_CLOCK_REALTIME,
+                                  monitor_qapi_event_handler,
+                                  evstate);
 }
 
 static void monitor_qapi_event_init(void)
-- 
2.4.3

Re: [Qemu-devel] [PATCH 3/7] monitor: Switch from timer_new() to timer_new_ns()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1256 bytes --]

On 10/15/2015 09:08 AM, Markus Armbruster wrote:
> We don't actually care for the scale, so we can just as well use the
> simpler interface.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index ca201fd..baffd76 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -545,10 +545,9 @@ monitor_qapi_event_throttle(QAPIEvent event, int64_t rate)
>      assert(rate * SCALE_MS <= INT64_MAX);

A bit unrelated:

Isn't this assertion risky, compared to:

assert(rate <= INT64_MAX / SCALE_MS);

>      evstate->rate = rate * SCALE_MS;
>      evstate->qdict = NULL;
> -    evstate->timer = timer_new(QEMU_CLOCK_REALTIME,
> -                               SCALE_MS,
> -                               monitor_qapi_event_handler,
> -                               evstate);
> +    evstate->timer = timer_new_ns(QEMU_CLOCK_REALTIME,
> +                                  monitor_qapi_event_handler,
> +                                  evstate);
>  }

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH 4/7] monitor: Split MonitorQAPIEventConf off MonitorQAPIEventState

[Markus Armbruster]

In preparation of turning monitor_qapi_event_state[] into a hash table
for finer grained throttling.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 monitor.c | 80 ++++++++++++++++++++++++++++++---------------------------------
 1 file changed, 38 insertions(+), 42 deletions(-)

diff --git a/monitor.c b/monitor.c
index baffd76..5e3a7f2 100644
--- a/monitor.c
+++ b/monitor.c
@@ -182,11 +182,14 @@ typedef struct {
  */
 typedef struct MonitorQAPIEventState {
     QAPIEvent event;    /* Event being tracked */
-    int64_t rate;       /* Minimum time (in ns) between two events */
     QEMUTimer *timer;   /* Timer for handling delayed events */
     QDict *qdict;       /* Delayed event (if any) */
 } MonitorQAPIEventState;
 
+typedef struct {
+    int64_t rate;       /* Minimum time (in ns) between two events */
+} MonitorQAPIEventConf;
+
 struct Monitor {
     CharDriverState *chr;
     int reset_seen;
@@ -437,6 +440,16 @@ static void monitor_protocol_emitter(Monitor *mon, QObject *data,
 }
 
 
+static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT_MAX] = {
+    /* Limit guest-triggerable events to 1 per second */
+    [QAPI_EVENT_RTC_CHANGE]        = { 1000 * SCALE_MS },
+    [QAPI_EVENT_WATCHDOG]          = { 1000 * SCALE_MS },
+    [QAPI_EVENT_BALLOON_CHANGE]    = { 1000 * SCALE_MS },
+    [QAPI_EVENT_QUORUM_REPORT_BAD] = { 1000 * SCALE_MS },
+    [QAPI_EVENT_QUORUM_FAILURE]    = { 1000 * SCALE_MS },
+    [QAPI_EVENT_VSERPORT_CHANGE]   = { 1000 * SCALE_MS },
+};
+
 static MonitorQAPIEventState monitor_qapi_event_state[QAPI_EVENT_MAX];
 
 /*
@@ -462,21 +475,23 @@ static void monitor_qapi_event_emit(QAPIEvent event, QDict *qdict)
 static void
 monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp)
 {
+    MonitorQAPIEventConf *evconf;
     MonitorQAPIEventState *evstate;
 
     assert(event < QAPI_EVENT_MAX);
+    evconf = &monitor_qapi_event_conf[event];
     evstate = &monitor_qapi_event_state[event];
-    trace_monitor_protocol_event_queue(event, qdict, evstate->rate);
+    trace_monitor_protocol_event_queue(event, qdict, evconf->rate);
 
     qemu_mutex_lock(&monitor_lock);
 
-    if (!evstate->rate) {
+    if (!evconf->rate) {
         /* Unthrottled event */
         monitor_qapi_event_emit(event, qdict);
     } else {
         if (timer_pending(evstate->timer)) {
             /*
-             * Timer is pending for (at least) evstate->rate ns after
+             * Timer is pending for (at least) evconf->rate ns after
              * last send.  Store event for sending when timer fires,
              * replacing a prior stored event if any.
              */
@@ -485,15 +500,15 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp)
             QINCREF(evstate->qdict);
         } else {
             /*
-             * Last send was (at least) evstate->rate ns ago.
+             * Last send was (at least) evconf->rate ns ago.
              * Send immediately, and arm the timer to call
-             * monitor_qapi_event_handler() in evstate->rate ns.  Any
+             * monitor_qapi_event_handler() in evconf->rate ns.  Any
              * events arriving before then will be delayed until then.
              */
             int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
 
             monitor_qapi_event_emit(event, qdict);
-            timer_mod_ns(evstate->timer, now + evstate->rate);
+            timer_mod_ns(evstate->timer, now + evconf->rate);
         }
     }
 
@@ -501,13 +516,14 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp)
 }
 
 /*
- * This function runs evstate->rate ns after sending a throttled
+ * This function runs evconf->rate ns after sending a throttled
  * event.
  * If another event has since been stored, send it.
  */
 static void monitor_qapi_event_handler(void *opaque)
 {
     MonitorQAPIEventState *evstate = opaque;
+    MonitorQAPIEventConf *evconf = &monitor_qapi_event_conf[evstate->event];
 
     trace_monitor_protocol_event_handler(evstate->event, evstate->qdict);
     qemu_mutex_lock(&monitor_lock);
@@ -518,47 +534,27 @@ static void monitor_qapi_event_handler(void *opaque)
         monitor_qapi_event_emit(evstate->event, evstate->qdict);
         QDECREF(evstate->qdict);
         evstate->qdict = NULL;
-        timer_mod_ns(evstate->timer, now + evstate->rate);
+        timer_mod_ns(evstate->timer, now + evconf->rate);
     }
 
     qemu_mutex_unlock(&monitor_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_qapi_event_throttle(QAPIEvent event, int64_t rate)
-{
-    MonitorQAPIEventState *evstate;
-    assert(event < QAPI_EVENT_MAX);
-
-    evstate = &(monitor_qapi_event_state[event]);
-
-    trace_monitor_protocol_event_throttle(event, rate);
-    evstate->event = event;
-    assert(rate * SCALE_MS <= INT64_MAX);
-    evstate->rate = rate * SCALE_MS;
-    evstate->qdict = NULL;
-    evstate->timer = timer_new_ns(QEMU_CLOCK_REALTIME,
-                                  monitor_qapi_event_handler,
-                                  evstate);
-}
-
 static void monitor_qapi_event_init(void)
 {
-    /* Limit guest-triggerable events to 1 per second */
-    monitor_qapi_event_throttle(QAPI_EVENT_RTC_CHANGE, 1000);
-    monitor_qapi_event_throttle(QAPI_EVENT_WATCHDOG, 1000);
-    monitor_qapi_event_throttle(QAPI_EVENT_BALLOON_CHANGE, 1000);
-    monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_REPORT_BAD, 1000);
-    monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_FAILURE, 1000);
-    monitor_qapi_event_throttle(QAPI_EVENT_VSERPORT_CHANGE, 1000);
+    int i;
+    MonitorQAPIEventState *evstate;
+
+    for (i = 0; i < QAPI_EVENT_MAX; i++) {
+        if (monitor_qapi_event_conf[i].rate) {
+            evstate = &monitor_qapi_event_state[i];
+            evstate->event = i;
+            evstate->qdict = NULL;
+            evstate->timer = timer_new_ns(QEMU_CLOCK_REALTIME,
+                                          monitor_qapi_event_handler,
+                                          evstate);
+        }
+    }
 
     qmp_event_set_func_emit(monitor_qapi_event_queue);
 }
-- 
2.4.3

[Qemu-devel] [PATCH 5/7] monitor: Turn monitor_qapi_event_state[] into a hash table

[Markus Armbruster]

In preparation of finer grained throttling.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 monitor.c | 55 ++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 17 deletions(-)

diff --git a/monitor.c b/monitor.c
index 5e3a7f2..ebec428 100644
--- a/monitor.c
+++ b/monitor.c
@@ -450,7 +450,7 @@ static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT_MAX] = {
     [QAPI_EVENT_VSERPORT_CHANGE]   = { 1000 * SCALE_MS },
 };
 
-static MonitorQAPIEventState monitor_qapi_event_state[QAPI_EVENT_MAX];
+GHashTable *monitor_qapi_event_state;
 
 /*
  * Emits the event to every monitor instance, @event is only used for trace
@@ -468,6 +468,8 @@ static void monitor_qapi_event_emit(QAPIEvent event, QDict *qdict)
     }
 }
 
+static void monitor_qapi_event_handler(void *opaque);
+
 /*
  * Queue a new event for emission to Monitor instances,
  * applying any rate limiting if required.
@@ -480,7 +482,6 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp)
 
     assert(event < QAPI_EVENT_MAX);
     evconf = &monitor_qapi_event_conf[event];
-    evstate = &monitor_qapi_event_state[event];
     trace_monitor_protocol_event_queue(event, qdict, evconf->rate);
 
     qemu_mutex_lock(&monitor_lock);
@@ -489,7 +490,12 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp)
         /* Unthrottled event */
         monitor_qapi_event_emit(event, qdict);
     } else {
-        if (timer_pending(evstate->timer)) {
+        MonitorQAPIEventState key = { .event = event };
+
+        evstate = g_hash_table_lookup(monitor_qapi_event_state, &key);
+        assert(!evstate || timer_pending(evstate->timer));
+
+        if (evstate) {
             /*
              * Timer is pending for (at least) evconf->rate ns after
              * last send.  Store event for sending when timer fires,
@@ -508,6 +514,14 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp)
             int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
 
             monitor_qapi_event_emit(event, qdict);
+
+            evstate = g_new(MonitorQAPIEventState, 1);
+            evstate->event = event;
+            evstate->qdict = NULL;
+            evstate->timer = timer_new_ns(QEMU_CLOCK_REALTIME,
+                                          monitor_qapi_event_handler,
+                                          evstate);
+            g_hash_table_add(monitor_qapi_event_state, evstate);
             timer_mod_ns(evstate->timer, now + evconf->rate);
         }
     }
@@ -535,27 +549,34 @@ static void monitor_qapi_event_handler(void *opaque)
         QDECREF(evstate->qdict);
         evstate->qdict = NULL;
         timer_mod_ns(evstate->timer, now + evconf->rate);
+    } else {
+        g_hash_table_remove(monitor_qapi_event_state, evstate);
+        timer_free(evstate->timer);
+        g_free(evstate);
     }
 
     qemu_mutex_unlock(&monitor_lock);
 }
 
+static unsigned int qapi_event_throttle_hash(const void *key)
+{
+    const MonitorQAPIEventState *evstate = key;
+
+    return evstate->event * 255;
+}
+
+static gboolean qapi_event_throttle_equal(const void *a, const void *b)
+{
+    const MonitorQAPIEventState *eva = a;
+    const MonitorQAPIEventState *evb = b;
+
+    return eva->event == evb->event;
+}
+
 static void monitor_qapi_event_init(void)
 {
-    int i;
-    MonitorQAPIEventState *evstate;
-
-    for (i = 0; i < QAPI_EVENT_MAX; i++) {
-        if (monitor_qapi_event_conf[i].rate) {
-            evstate = &monitor_qapi_event_state[i];
-            evstate->event = i;
-            evstate->qdict = NULL;
-            evstate->timer = timer_new_ns(QEMU_CLOCK_REALTIME,
-                                          monitor_qapi_event_handler,
-                                          evstate);
-        }
-    }
-
+    monitor_qapi_event_state = g_hash_table_new(qapi_event_throttle_hash,
+                                                qapi_event_throttle_equal);
     qmp_event_set_func_emit(monitor_qapi_event_queue);
 }
 
-- 
2.4.3

[Qemu-devel] [PATCH 6/7] monitor: Throttle event VSERPORT_CHANGE separately by "id"

[Markus Armbruster]

VSERPORT_CHANGE is emitted when the guest opens or closes a
virtio-serial port.  The event's member "id" identifies the port.

When several events arrive quickly, throttling drops all but the last
of them.  Because of that, a QMP client must assume that *any* port
may have changed state when it receives a VSERPORT_CHANGE event and
throttling may have happened.

Make the event more useful by throttling it for each port separately.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 monitor.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index ebec428..f248a68 100644
--- a/monitor.c
+++ b/monitor.c
@@ -181,7 +181,8 @@ typedef struct {
  * instance.
  */
 typedef struct MonitorQAPIEventState {
-    QAPIEvent event;    /* Event being tracked */
+    QAPIEvent event;    /* Throttling state for this event type and... */
+    QDict *data;        /* ... data, see qapi_event_throttle_equal() */
     QEMUTimer *timer;   /* Timer for handling delayed events */
     QDict *qdict;       /* Delayed event (if any) */
 } MonitorQAPIEventState;
@@ -490,7 +491,8 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp)
         /* Unthrottled event */
         monitor_qapi_event_emit(event, qdict);
     } else {
-        MonitorQAPIEventState key = { .event = event };
+        QDict *data = qobject_to_qdict(qdict_get(qdict, "data"));
+        MonitorQAPIEventState key = { .event = event, .data = data };
 
         evstate = g_hash_table_lookup(monitor_qapi_event_state, &key);
         assert(!evstate || timer_pending(evstate->timer));
@@ -517,6 +519,8 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp)
 
             evstate = g_new(MonitorQAPIEventState, 1);
             evstate->event = event;
+            evstate->data = data;
+            QINCREF(evstate->data);
             evstate->qdict = NULL;
             evstate->timer = timer_new_ns(QEMU_CLOCK_REALTIME,
                                           monitor_qapi_event_handler,
@@ -551,6 +555,7 @@ static void monitor_qapi_event_handler(void *opaque)
         timer_mod_ns(evstate->timer, now + evconf->rate);
     } else {
         g_hash_table_remove(monitor_qapi_event_state, evstate);
+        QDECREF(evstate->data);
         timer_free(evstate->timer);
         g_free(evstate);
     }
@@ -561,8 +566,13 @@ static void monitor_qapi_event_handler(void *opaque)
 static unsigned int qapi_event_throttle_hash(const void *key)
 {
     const MonitorQAPIEventState *evstate = key;
+    unsigned int hash = evstate->event * 255;
 
-    return evstate->event * 255;
+    if (evstate->event == QAPI_EVENT_VSERPORT_CHANGE) {
+        hash += g_str_hash(qdict_get_str(evstate->data, "id"));
+    }
+
+    return hash;
 }
 
 static gboolean qapi_event_throttle_equal(const void *a, const void *b)
@@ -570,7 +580,16 @@ static gboolean qapi_event_throttle_equal(const void *a, const void *b)
     const MonitorQAPIEventState *eva = a;
     const MonitorQAPIEventState *evb = b;
 
-    return eva->event == evb->event;
+    if (eva->event != evb->event) {
+        return FALSE;
+    }
+
+    if (eva->event == QAPI_EVENT_VSERPORT_CHANGE) {
+        return !strcmp(qdict_get_str(eva->data, "id"),
+                       qdict_get_str(evb->data, "id"));
+    }
+
+    return TRUE;
 }
 
 static void monitor_qapi_event_init(void)
-- 
2.4.3

[Qemu-devel] [PATCH 7/7] docs: Document QMP event rate limiting

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/qmp-events.txt | 12 ++++++++++++
 docs/qmp-spec.txt   |  5 +++++
 2 files changed, 17 insertions(+)

diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
index d92cc48..d2f1ce4 100644
--- a/docs/qmp-events.txt
+++ b/docs/qmp-events.txt
@@ -28,6 +28,8 @@ Example:
     "data": { "actual": 944766976 },
     "timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
 
+Note: this event is rate-limited.
+
 BLOCK_IMAGE_CORRUPTED
 ---------------------
 
@@ -296,6 +298,8 @@ Example:
      "data": { "reference": "usr1", "sector-num": 345435, "sectors-count": 5 },
      "timestamp": { "seconds": 1344522075, "microseconds": 745528 } }
 
+Note: this event is rate-limited.
+
 QUORUM_REPORT_BAD
 -----------------
 
@@ -318,6 +322,8 @@ Example:
      "data": { "node-name": "1.raw", "sector-num": 345435, "sectors-count": 5 },
      "timestamp": { "seconds": 1344522075, "microseconds": 745528 } }
 
+Note: this event is rate-limited.
+
 RESET
 -----
 
@@ -358,6 +364,8 @@ Example:
     "data": { "offset": 78 },
     "timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
 
+Note: this event is rate-limited.
+
 SHUTDOWN
 --------
 
@@ -632,6 +640,8 @@ Example:
     "data": { "id": "channel0", "open": true },
     "timestamp": { "seconds": 1401385907, "microseconds": 422329 } }
 
+Note: this event is rate-limited separately for each "id".
+
 WAKEUP
 ------
 
@@ -662,3 +672,5 @@ Example:
 
 Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is
 followed respectively by the RESET, SHUTDOWN, or STOP events.
+
+Note: this event is rate-limited.
diff --git a/docs/qmp-spec.txt b/docs/qmp-spec.txt
index 4c28cd9..4fb10a5 100644
--- a/docs/qmp-spec.txt
+++ b/docs/qmp-spec.txt
@@ -175,6 +175,11 @@ The format of asynchronous events is:
 For a listing of supported asynchronous events, please, refer to the
 qmp-events.txt file.
 
+Some events are rate-limited to at most one per second.  If additional
+"similar" events arrive within one second, all but the last one are
+dropped, and the last one is delayed.  "Similar" normally means same
+event type.  See qmp-events.txt for details.
+
 2.5 QGA Synchronization
 -----------------------
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH 7/7] docs: Document QMP event rate limiting

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 390 bytes --]

On 10/15/2015 09:08 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  docs/qmp-events.txt | 12 ++++++++++++
>  docs/qmp-spec.txt   |  5 +++++
>  2 files changed, 17 insertions(+)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 0/7] Throttle event VSERPORT_CHANGE separately by "id"

[Markus Armbruster]

Forgot to mention: this is based on my "[PATCH 0/6] qobject: Make
conversion from QObject * accept null".

[Qemu-devel] [PATCH 4.5/7] glib: add compatibility interface for g_hash_table_add()

[Markus Armbruster]

The next commit will use it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/glib-compat.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/glib-compat.h b/include/glib-compat.h
index fb25f43..5185630 100644
--- a/include/glib-compat.h
+++ b/include/glib-compat.h
@@ -165,6 +165,13 @@ static inline GThread *g_thread_new(const char *name,
 #define CompatGCond GCond
 #endif /* glib 2.31 */
 
+#if !GLIB_CHECK_VERSION(2, 32, 0)
+static inline gboolean g_hash_table_add (GHashTable *hash_table, gpointer key)
+{
+    return g_hash_table_replace(hash_table, key, key)
+}
+#endif
+
 #ifndef g_assert_true
 #define g_assert_true(expr)                                                    \
     do {                                                                       \
-- 
2.4.3

Re: [Qemu-devel] [PATCH 4.5/7] glib: add compatibility interface for g_hash_table_add()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 844 bytes --]

On 10/27/2015 08:51 AM, Markus Armbruster wrote:
> The next commit will use it.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  include/glib-compat.h | 7 +++++++
>  1 file changed, 7 insertions(+)

One nit:

> 
> diff --git a/include/glib-compat.h b/include/glib-compat.h
> index fb25f43..5185630 100644
> --- a/include/glib-compat.h
> +++ b/include/glib-compat.h
> @@ -165,6 +165,13 @@ static inline GThread *g_thread_new(const char *name,
>  #define CompatGCond GCond
>  #endif /* glib 2.31 */
>  
> +#if !GLIB_CHECK_VERSION(2, 32, 0)
> +static inline gboolean g_hash_table_add (GHashTable *hash_table, gpointer key)

Drop the space before (

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 4.5/7] glib: add compatibility interface for g_hash_table_add()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 10/27/2015 08:51 AM, Markus Armbruster wrote:
>> The next commit will use it.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  include/glib-compat.h | 7 +++++++
>>  1 file changed, 7 insertions(+)
>
> One nit:
>
>> 
>> diff --git a/include/glib-compat.h b/include/glib-compat.h
>> index fb25f43..5185630 100644
>> --- a/include/glib-compat.h
>> +++ b/include/glib-compat.h
>> @@ -165,6 +165,13 @@ static inline GThread *g_thread_new(const char *name,
>>  #define CompatGCond GCond
>>  #endif /* glib 2.31 */
>>  
>> +#if !GLIB_CHECK_VERSION(2, 32, 0)
>> +static inline gboolean g_hash_table_add (GHashTable *hash_table,
>> gpointer key)
>
> Drop the space before (
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Done.  Thanks!