[Qemu-devel] [RFC 0/6] Throttle event VSERPORT_CHANGE separately by "id"

[Qemu-devel] [RFC 0/6] 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.

RFC because it's compile-tested only.  Compile-tested only because I
want to give my feedback to Marc-André's work as soon as I can
(although "as soon as I can" still means "frustratingly late", I'm
afraid).

PATCH 1+2 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 3-5 do the actual work.

PATCH 6 fixes docs to mention throttling.

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

Markus Armbruster (6):
  monitor: Reduce casting of QAPI event QDict
  monitor: Simplify event throttling
  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/qmp-events.txt |  12 ++++
 docs/qmp/qmp-spec.txt   |   5 ++
 monitor.c               | 186 ++++++++++++++++++++++++++++--------------------
 3 files changed, 126 insertions(+), 77 deletions(-)

-- 
2.4.3

[Qemu-devel] [RFC 1/6] 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>
---
 monitor.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/monitor.c b/monitor.c
index d0edb3b..2cbb2d9 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;       /* Event pending delayed dispatch */
 } MonitorQAPIEventState;
 
 struct Monitor {
@@ -445,14 +445,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));
         }
     }
 }
@@ -462,7 +462,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);
@@ -470,7 +470,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);
@@ -478,26 +478,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;
         }
     }
@@ -514,14 +514,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);
@@ -548,7 +548,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] [RFC 2/6] 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

FIXME update trace-event (and recompile the world)

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

diff --git a/monitor.c b/monitor.c
index 2cbb2d9..56c9dec 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;       /* Event pending delayed dispatch */
 } MonitorQAPIEventState;
@@ -465,65 +464,69 @@ 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]);
+    assert(event < QAPI_EVENT_MAX);
+    evstate = &monitor_qapi_event_state[event];
     trace_monitor_protocol_event_queue(event,
                                        qdict,
                                        evstate->rate,
-                                       evstate->last,
-                                       now);
+                                       0, 0); /* FIXME drop */
 
-    /* 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);
+                                         0, 0); /* FIXME drop */
     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);
 }
 
@@ -539,20 +542,17 @@ static void
 monitor_qapi_event_throttle(QAPIEvent event, int64_t rate)
 {
     MonitorQAPIEventState *evstate;
+
     assert(event < QAPI_EVENT_MAX);
-
-    evstate = &(monitor_qapi_event_state[event]);
-
+    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->last = 0;
     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

[Qemu-devel] [RFC 3/6] 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>
---
 monitor.c | 79 ++++++++++++++++++++++++++++++---------------------------------
 1 file changed, 38 insertions(+), 41 deletions(-)

diff --git a/monitor.c b/monitor.c
index 56c9dec..75c9580 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;       /* Event pending delayed dispatch */
 } MonitorQAPIEventState;
 
+typedef struct {
+    int64_t rate;       /* Minimum time (in ns) between two events */
+} MonitorQAPIEventConf;
+
 struct Monitor {
     CharDriverState *chr;
     int reset_seen;
@@ -438,6 +441,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];
 
 /*
@@ -463,24 +476,26 @@ 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,
+                                       evconf->rate,
                                        0, 0); /* FIXME drop */
 
     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.
              */
@@ -489,15 +504,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);
         }
     }
 
@@ -505,13 +520,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,
@@ -524,46 +540,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] [RFC 4/6] 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>
---
 monitor.c | 55 ++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 17 deletions(-)

diff --git a/monitor.c b/monitor.c
index 75c9580..9807a5b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -451,7 +451,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
@@ -469,6 +469,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.
@@ -481,7 +483,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,
@@ -493,7 +494,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, .qdict = qdict };
+
+        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,
@@ -512,6 +518,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);
         }
     }
@@ -541,27 +555,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] [RFC 5/6] 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>
---
 monitor.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/monitor.c b/monitor.c
index 9807a5b..7e21407 100644
--- a/monitor.c
+++ b/monitor.c
@@ -567,8 +567,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->qdict, "id"));
+    }
+
+    return hash;
 }
 
 static gboolean qapi_event_throttle_equal(const void *a, const void *b)
@@ -576,7 +581,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->qdict, "id"),
+                       qdict_get_str(evb->qdict, "id"));
+    }
+
+    return TRUE;
 }
 
 static void monitor_qapi_event_init(void)
-- 
2.4.3

[Qemu-devel] [RFC 6/6] docs: Document QMP event rate limiting

[Markus Armbruster]

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

diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
index d92cc48..d2f1ce4 100644
--- a/docs/qmp/qmp-events.txt
+++ b/docs/qmp/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/qmp-spec.txt b/docs/qmp/qmp-spec.txt
index 4c28cd9..4cd6fdb 100644
--- a/docs/qmp/qmp-spec.txt
+++ b/docs/qmp/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 more
+events arrive within one second, all but the last one are dropped, and
+the last one is delayed.  Rate-limiting applies to each kind of event
+separately.
+
 2.5 QGA Synchronization
 -----------------------
 
-- 
2.4.3

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

[Marc-André Lureau]

Hi

On Fri, Sep 25, 2015 at 4:00 PM, Markus Armbruster <armbru@redhat.com> wrote:
> 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.

I dislike the approach to put all event filters in the same hashtable.
The simple case doesn't need it, and I kept it that way. But
certainly, it shorten a bit the code. Since it is probably not
performance critical, it doesn't matter, so you may prefer less code.
I would still argue that it's cleaner and faster the way I proposed.
ymmv

> RFC because it's compile-tested only.  Compile-tested only because I
> want to give my feedback to Marc-André's work as soon as I can
> (although "as soon as I can" still means "frustratingly late", I'm
> afraid).
>
> PATCH 1+2 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 3-5 do the actual work.
>
> PATCH 6 fixes docs to mention throttling.
>
> Diffstat for monitor.c
>     Marc-André's series: 201 insertions(+), 55 deletions(-)
>     mine:                109 insertions(+), 77 deletions(-)
>
> Markus Armbruster (6):
>   monitor: Reduce casting of QAPI event QDict
>   monitor: Simplify event throttling
>   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/qmp-events.txt |  12 ++++
>  docs/qmp/qmp-spec.txt   |   5 ++
>  monitor.c               | 186 ++++++++++++++++++++++++++++--------------------
>  3 files changed, 126 insertions(+), 77 deletions(-)
>
> --
> 2.4.3
>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [RFC 1/6] monitor: Reduce casting of QAPI event QDict

[Eric Blake]

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

On 09/25/2015 08:00 AM, Markus Armbruster wrote:
> Make the variables holding the event QDict instead of QObject.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c | 38 +++++++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index d0edb3b..2cbb2d9 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;       /* Event pending delayed dispatch */
>  } MonitorQAPIEventState;
>  

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] [RFC 2/6] monitor: Simplify event throttling

[Eric Blake]

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

On 09/25/2015 08:00 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
> 

Makes sense for what throttling is supposed to be.

> FIXME update trace-event (and recompile the world)

Obviously something you'd fold into a v2, so I'll defer R-b until seeing
it.  But I like the approach of this patch.

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

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

Possible in states 2 and 3...

> +            /*
> +             * 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);

no-op in state 2, otherwise replaces the old pending event in state 3

>              evstate->qdict = qdict;
>              QINCREF(evstate->qdict);

either way, we are now in state 3 awaiting the next event or timer.

>          } else {

we are in state 1...

> +            /*
> +             * 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);

and now in state 2.

>          }
>      }
> +
>      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)
>  {

We get here in either state 2 or 3...

>      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);
> +                                         0, 0); /* FIXME drop */
>      qemu_mutex_lock(&monitor_lock);
> +
>      if (evstate->qdict) {

state 3, so send it...

> +        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);

...and rearm to go back to state 2

>      }
> -    evstate->last = now;
> +

...otherwise, we didn't rearm, so we go back to state 1

>      qemu_mutex_unlock(&monitor_lock);
>  }
>  

Matches the state logic called out in the commit message.

> @@ -539,20 +542,17 @@ static void
>  monitor_qapi_event_throttle(QAPIEvent event, int64_t rate)
>  {
>      MonitorQAPIEventState *evstate;
> +
>      assert(event < QAPI_EVENT_MAX);
> -
> -    evstate = &(monitor_qapi_event_state[event]);
> -
> +    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->last = 0;
>      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);

Is this a separate cleanup?  It looks like you're fixing code style, and
then switching from timer_new() to timer_new_ns(), but it's not obvious
from just this context whether you still have the right scale (and I
didn't go chase documentation).  It wasn't mentioned in the commit
message, and seems unrelated to the new state machine.

-- 
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] [RFC 3/6] monitor: Split MonitorQAPIEventConf off MonitorQAPIEventState

[Eric Blake]

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

On 09/25/2015 08:00 AM, Markus Armbruster wrote:
> In preparation of turning monitor_qapi_event_state[] into a hash table
> for finer grained throttling.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c | 79 ++++++++++++++++++++++++++++++---------------------------------
>  1 file changed, 38 insertions(+), 41 deletions(-)
> 

> -/*
> - * @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)
> -{

At one point, I think we were considering having the rate be
user-tunable through a qom property, in which case this function is
still nicer than a monitor_qapi_event_conf[] fixed-rate table.  But
since I don't think we ever used it, I'm fine with dropping the
complexity and living with a constant rate.

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] [RFC 4/6] monitor: Turn monitor_qapi_event_state[] into a hash table

[Eric Blake]

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

On 09/25/2015 08:00 AM, Markus Armbruster wrote:
> In preparation of finer grained throttling.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  monitor.c | 55 ++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 38 insertions(+), 17 deletions(-)
> 

> @@ -512,6 +518,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);

Now timers are created and destroyed dynamically upon use rather than
reused and just waiting to be rearmed; I hope there aren't any obvious
inefficiencies from doing that.

The conversion looks sane:
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] [RFC 5/6] monitor: Throttle event VSERPORT_CHANGE separately by "id"

[Eric Blake]

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

On 09/25/2015 08:00 AM, Markus Armbruster wrote:
> 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>
> ---
>  monitor.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 

All future differentiation would be added as additional special cases
within the hash functions, but I like this approach for keeping the rest
of the algorithm independent from what the hashing considers as equivalent.

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] [RFC 6/6] docs: Document QMP event rate limiting

[Eric Blake]

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

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

Obvious rebase implied if your other patch to s,docs/qmp/,docs/, goes
through.

> 
> diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
> index d92cc48..d2f1ce4 100644
> --- a/docs/qmp/qmp-events.txt
> +++ b/docs/qmp/qmp-events.txt
> @@ -28,6 +28,8 @@ Example:
>      "data": { "actual": 944766976 },
>      "timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
>  
> +Note: this event is rate-limited.

Marc-Andre's series moves all the documentation into the .json files;
maybe we could make it easier by just listing rate-limiting in the .json
files for now, rather than having to churn through which file(s)
document things.

Do we want to document that VSERPORT_CHANGE rate limiting is per-"id"?

> +++ b/docs/qmp/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 more
> +events arrive within one second, all but the last one are dropped, and
> +the last one is delayed.  Rate-limiting applies to each kind of event
> +separately.

Do we also want to document that limits might be further tuned according
to other elements of the event, with VSERPORT_CHANGE "id" as the example?

-- 
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] [RFC 2/6] monitor: Simplify event throttling

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 09/25/2015 08:00 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
>> 
>
> Makes sense for what throttling is supposed to be.
>
>> FIXME update trace-event (and recompile the world)
>
> Obviously something you'd fold into a v2, so I'll defer R-b until seeing
> it.  But I like the approach of this patch.
>
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  monitor.c | 70 +++++++++++++++++++++++++++++++--------------------------------
>>  1 file changed, 35 insertions(+), 35 deletions(-)
>> 
>
>>      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)) {
>
> Possible in states 2 and 3...
>
>> +            /*
>> +             * 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);
>
> no-op in state 2, otherwise replaces the old pending event in state 3
>
>>              evstate->qdict = qdict;
>>              QINCREF(evstate->qdict);
>
> either way, we are now in state 3 awaiting the next event or timer.
>
>>          } else {
>
> we are in state 1...
>
>> +            /*
>> +             * 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);
>
> and now in state 2.
>
>>          }
>>      }
>> +
>>      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)
>>  {
>
> We get here in either state 2 or 3...
>
>>      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);
>> +                                         0, 0); /* FIXME drop */
>>      qemu_mutex_lock(&monitor_lock);
>> +
>>      if (evstate->qdict) {
>
> state 3, so send it...
>
>> +        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);
>
> ...and rearm to go back to state 2
>
>>      }
>> -    evstate->last = now;
>> +
>
> ...otherwise, we didn't rearm, so we go back to state 1
>
>>      qemu_mutex_unlock(&monitor_lock);
>>  }
>>  
>
> Matches the state logic called out in the commit message.
>
>> @@ -539,20 +542,17 @@ static void
>>  monitor_qapi_event_throttle(QAPIEvent event, int64_t rate)
>>  {
>>      MonitorQAPIEventState *evstate;
>> +
>>      assert(event < QAPI_EVENT_MAX);
>> -
>> -    evstate = &(monitor_qapi_event_state[event]);
>> -
>> +    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->last = 0;
>>      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);
>
> Is this a separate cleanup?  It looks like you're fixing code style, and
> then switching from timer_new() to timer_new_ns(), but it's not obvious
> from just this context whether you still have the right scale (and I
> didn't go chase documentation).  It wasn't mentioned in the commit
> message, and seems unrelated to the new state machine.

I'll split it off and explain it in the commit message.

Thanks!

Re: [Qemu-devel] [RFC 3/6] monitor: Split MonitorQAPIEventConf off MonitorQAPIEventState

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 09/25/2015 08:00 AM, Markus Armbruster wrote:
>> In preparation of turning monitor_qapi_event_state[] into a hash table
>> for finer grained throttling.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  monitor.c | 79 ++++++++++++++++++++++++++++++---------------------------------
>>  1 file changed, 38 insertions(+), 41 deletions(-)
>> 
>
>> -/*
>> - * @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)
>> -{
>
> At one point, I think we were considering having the rate be
> user-tunable through a qom property, in which case this function is
> still nicer than a monitor_qapi_event_conf[] fixed-rate table.  But
> since I don't think we ever used it, I'm fine with dropping the
> complexity and living with a constant rate.

I'm no friend on complicating stuff now to hopefully facilitate things
we might want to do some day, but don't really understand now.

Say we provide a way for the user to configure the rate.  Say you
accidentally set the rate to a month, and now want to fix your blunder
by setting it to a minute.  If an event has already arrived, the timer
is currently set to expire next month, and your fixed rate will take
effect then.

My point is configuring the rate could be more complicated than storing
the new rate, and keeping monitor_qapi_event_throttle() around won't
make the job materially easier.

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

Thanks!

Re: [Qemu-devel] [RFC 4/6] monitor: Turn monitor_qapi_event_state[] into a hash table

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 09/25/2015 08:00 AM, Markus Armbruster wrote:
>> In preparation of finer grained throttling.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  monitor.c | 55 ++++++++++++++++++++++++++++++++++++++-----------------
>>  1 file changed, 38 insertions(+), 17 deletions(-)
>> 
>
>> @@ -512,6 +518,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);
>
> Now timers are created and destroyed dynamically upon use rather than
> reused and just waiting to be rearmed; I hope there aren't any obvious
> inefficiencies from doing that.

I hope there aren't any unobvious inefficienies!  :)

If I read qemu-timer.c correctly, creating and destroying timers is
cheap.

Hmm, looks like I'm missing a timer_del() before timer_free().

> The conversion looks sane:
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [Qemu-devel] [RFC 5/6] monitor: Throttle event VSERPORT_CHANGE separately by "id"

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 09/25/2015 08:00 AM, Markus Armbruster wrote:
>> 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>
>> ---
>>  monitor.c | 18 ++++++++++++++++--
>>  1 file changed, 16 insertions(+), 2 deletions(-)
>> 
>
> All future differentiation would be added as additional special cases
> within the hash functions, but I like this approach for keeping the rest
> of the algorithm independent from what the hashing considers as equivalent.

Should that ever become unwieldy, we can easily add indirections right
there.

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

Thanks!

Re: [Qemu-devel] [RFC 6/6] docs: Document QMP event rate limiting

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 09/25/2015 08:00 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  docs/qmp/qmp-events.txt | 12 ++++++++++++
>>  docs/qmp/qmp-spec.txt   |  5 +++++
>>  2 files changed, 17 insertions(+)
>
> Obvious rebase implied if your other patch to s,docs/qmp/,docs/, goes
> through.
>
>> 
>> diff --git a/docs/qmp/qmp-events.txt b/docs/qmp/qmp-events.txt
>> index d92cc48..d2f1ce4 100644
>> --- a/docs/qmp/qmp-events.txt
>> +++ b/docs/qmp/qmp-events.txt
>> @@ -28,6 +28,8 @@ Example:
>>      "data": { "actual": 944766976 },
>>      "timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
>>  
>> +Note: this event is rate-limited.
>
> Marc-Andre's series moves all the documentation into the .json files;
> maybe we could make it easier by just listing rate-limiting in the .json
> files for now, rather than having to churn through which file(s)
> document things.

As long as qmp-events.txt exists, shouldn't we document rate-limiting
there?

> Do we want to document that VSERPORT_CHANGE rate limiting is per-"id"?

I did:

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

>> +++ b/docs/qmp/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 more
>> +events arrive within one second, all but the last one are dropped, and
>> +the last one is delayed.  Rate-limiting applies to each kind of event
>> +separately.
>
> Do we also want to document that limits might be further tuned according
> to other elements of the event, with VSERPORT_CHANGE "id" as the example?

You need to interpret "each kind of event" in a sufficiently fuzzy
manner :)

Seriously, I'm open to suggestions for better language here.

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

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Fri, Sep 25, 2015 at 4:00 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> 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.
>
> I dislike the approach to put all event filters in the same hashtable.
> The simple case doesn't need it, and I kept it that way. But
> certainly, it shorten a bit the code. Since it is probably not
> performance critical, it doesn't matter, so you may prefer less code.

Yes, I do prefer simpler code.  Fewer bugs, cheaper to maintain.

The performance differences aren't entirely obvious.  Sure, going
through a hash table is slower than a simple array subscript.  But your
indirections aren't free, either.  Mispredicted indirect calls, in
particular.

> I would still argue that it's cleaner and faster the way I proposed.
> ymmv

[...]

Re: [Qemu-devel] [RFC 6/6] docs: Document QMP event rate limiting

[Eric Blake]

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

On 09/28/2015 02:38 AM, Markus Armbruster wrote:

>>> +++ b/docs/qmp/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 more
>>> +events arrive within one second, all but the last one are dropped, and
>>> +the last one is delayed.  Rate-limiting applies to each kind of event
>>> +separately.
>>
>> Do we also want to document that limits might be further tuned according
>> to other elements of the event, with VSERPORT_CHANGE "id" as the example?
> 
> You need to interpret "each kind of event" in a sufficiently fuzzy
> manner :)
> 
> Seriously, I'm open to suggestions for better language here.

Maybe:

Some events are rate-limited to at most one per second. If more events
of the same type (along with any other fields that an event documents as
being significant) arrive within one second, ...

or:

Some events are rate-limited to at most one per second, either for the
event type in general, or additionally considering other significant
fields as documented by a specific event.  If more equivalent events
arrive within one second, ...

But since you did point out that VSERPORT_CHANGE mentions the filtering
on "id", I guess I won't try too much harder to paint the bikeshed any
further.

-- 
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] [RFC 6/6] docs: Document QMP event rate limiting

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 09/28/2015 02:38 AM, Markus Armbruster wrote:
>
>>>> +++ b/docs/qmp/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 more
>>>> +events arrive within one second, all but the last one are dropped, and
>>>> +the last one is delayed.  Rate-limiting applies to each kind of event
>>>> +separately.
>>>
>>> Do we also want to document that limits might be further tuned according
>>> to other elements of the event, with VSERPORT_CHANGE "id" as the example?
>> 
>> You need to interpret "each kind of event" in a sufficiently fuzzy
>> manner :)
>> 
>> Seriously, I'm open to suggestions for better language here.
>
> Maybe:
>
> Some events are rate-limited to at most one per second. If more events
> of the same type (along with any other fields that an event documents as
> being significant) arrive within one second, ...
>
> or:
>
> Some events are rate-limited to at most one per second, either for the
> event type in general, or additionally considering other significant
> fields as documented by a specific event.  If more equivalent events
> arrive within one second, ...
>
> But since you did point out that VSERPORT_CHANGE mentions the filtering
> on "id", I guess I won't try too much harder to paint the bikeshed any
> further.

I'll use this for inspiration when I prep the non-RFC patch.  Thanks!