[Qemu-devel] [PATCH v2 0/5] monitor: throttle VSERPORT_CHANGED by "id"

[Qemu-devel] [PATCH v2 0/5] monitor: throttle VSERPORT_CHANGED by "id"

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

QAPI_EVENT_VSERPORT_CHANGE reports changes of a virtio serial port
state. However, the events may be for different ports, but the throttle
mechanism may replace the event for a different port, since it only
checks the event type.

The following series implements throttling of events based on the "id"
field. Hopefully this hash table approach can be later extended if
other fields or combination of fields have to be used.

v1->v2:
- split first patch in 2 to ease review
- remove some extra space
- add some comments above delay handler function, and struct fields
- rename the delay handler data "delay_data"
- add a trace in monitor_protocol_event_delay()
- improve some commit messages
- simplify monitor_qapi_event_delay()
- add some comment assert code in monitor_qapi_event_id_delay() to
  ensure the given pending struct is valid
- fixed hashtable key leak
- rename qdict "data" argument to "qdict"
- removed superfluous parenthesis
- use a single timer handler for doing "id" filtering cleanup

Marc-André Lureau (5):
  monitor: split MonitorQAPIEventState
  monitor: introduce MonitorQAPIEventDelay callback
  monitor: rename QDict *data->qdict
  monitor: throttle QAPI_EVENT_VSERPORT_CHANGE by "id"
  monitor: remove old entries from event hash table

 monitor.c    | 256 ++++++++++++++++++++++++++++++++++++++++++++++-------------
 trace-events |   3 +-
 2 files changed, 203 insertions(+), 56 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v2 1/5] monitor: split MonitorQAPIEventState

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Create a separate structure MonitorQAPIEventPending for holding the data
related to pending event.

The next commits are going to reuse this structure for different
throttling implementations.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 monitor.c | 85 +++++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 50 insertions(+), 35 deletions(-)

diff --git a/monitor.c b/monitor.c
index 1f43263..2d2e030 100644
--- a/monitor.c
+++ b/monitor.c
@@ -174,18 +174,24 @@ typedef struct {
     bool in_command_mode;       /* are we in command mode? */
 } MonitorQMP;
 
+typedef struct {
+    QAPIEvent event;    /* Event being tracked */
+    int64_t last;       /* QEMU_CLOCK_REALTIME value at last emission */
+    QEMUTimer *timer;   /* Timer for handling delayed events */
+    QObject *data;      /* Event pending delayed dispatch */
+} MonitorQAPIEventPending;
+
+typedef struct MonitorQAPIEventState MonitorQAPIEventState;
+
 /*
  * To prevent flooding clients, events can be throttled. The
  * throttling is calculated globally, rather than per-Monitor
  * instance.
  */
-typedef struct MonitorQAPIEventState {
-    QAPIEvent event;    /* Event being tracked */
+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 */
-} MonitorQAPIEventState;
+    void *delay_data;   /* data for the throttle handler */
+};
 
 struct Monitor {
     CharDriverState *chr;
@@ -464,40 +470,41 @@ static void
 monitor_qapi_event_queue(QAPIEvent event, QDict *data, Error **errp)
 {
     MonitorQAPIEventState *evstate;
+    MonitorQAPIEventPending *p;
     assert(event < QAPI_EVENT_MAX);
     int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
 
-    evstate = &(monitor_qapi_event_state[event]);
+    evstate = &monitor_qapi_event_state[event];
+    p = evstate->delay_data;
     trace_monitor_protocol_event_queue(event,
                                        data,
                                        evstate->rate,
-                                       evstate->last,
+                                       p->last,
                                        now);
 
     /* Rate limit of 0 indicates no throttling */
     qemu_mutex_lock(&monitor_lock);
     if (!evstate->rate) {
         monitor_qapi_event_emit(event, QOBJECT(data));
-        evstate->last = now;
     } else {
-        int64_t delta = now - evstate->last;
-        if (evstate->data ||
+        int64_t delta = now - p->last;
+        if (evstate->delay_data ||
             delta < evstate->rate) {
             /* If there's an existing event pending, replace
              * it with the new event, otherwise schedule a
              * timer for delayed emission
              */
-            if (evstate->data) {
-                qobject_decref(evstate->data);
+            if (evstate->delay_data) {
+                qobject_decref(evstate->delay_data);
             } else {
-                int64_t then = evstate->last + evstate->rate;
-                timer_mod_ns(evstate->timer, then);
+                int64_t then = p->last + evstate->rate;
+                timer_mod_ns(p->timer, then);
             }
-            evstate->data = QOBJECT(data);
-            qobject_incref(evstate->data);
+            evstate->delay_data = QOBJECT(data);
+            qobject_incref(evstate->delay_data);
         } else {
             monitor_qapi_event_emit(event, QOBJECT(data));
-            evstate->last = now;
+            p->last = now;
         }
     }
     qemu_mutex_unlock(&monitor_lock);
@@ -509,23 +516,37 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *data, Error **errp)
  */
 static void monitor_qapi_event_handler(void *opaque)
 {
-    MonitorQAPIEventState *evstate = opaque;
+    MonitorQAPIEventPending *p = opaque;
     int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
 
-    trace_monitor_protocol_event_handler(evstate->event,
-                                         evstate->data,
-                                         evstate->last,
+    trace_monitor_protocol_event_handler(p->event,
+                                         p->data,
+                                         p->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 (p->data) {
+        monitor_qapi_event_emit(p->event, p->data);
+        qobject_decref(p->data);
+        p->data = NULL;
     }
-    evstate->last = now;
+    p->last = now;
     qemu_mutex_unlock(&monitor_lock);
 }
 
+static MonitorQAPIEventPending *
+monitor_qapi_event_pending_new(QAPIEvent event)
+{
+    MonitorQAPIEventPending *p;
+
+    p = g_new0(MonitorQAPIEventPending, 1);
+    p->event = event;
+    p->timer = timer_new(QEMU_CLOCK_REALTIME,
+                         SCALE_MS,
+                         monitor_qapi_event_handler,
+                         p);
+    return p;
+}
+
 /*
  * @event: the event ID to be limited
  * @rate: the rate limit in milliseconds
@@ -540,18 +561,12 @@ 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->data = NULL;
-    evstate->timer = timer_new(QEMU_CLOCK_REALTIME,
-                               SCALE_MS,
-                               monitor_qapi_event_handler,
-                               evstate);
+    evstate->delay_data = monitor_qapi_event_pending_new(event);
 }
 
 static void monitor_qapi_event_init(void)
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 1/5] monitor: split MonitorQAPIEventState

[Daniel P. Berrange]

On Thu, Sep 17, 2015 at 06:08:46PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Create a separate structure MonitorQAPIEventPending for holding the data
> related to pending event.
> 
> The next commits are going to reuse this structure for different
> throttling implementations.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c | 85 +++++++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 50 insertions(+), 35 deletions(-)

Reviewed-By: Daniel P. Berrange <berrange@redhat.com>

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

[Qemu-devel] [PATCH v2 2/5] monitor: introduce MonitorQAPIEventDelay callback

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Move the delay handling in a seperate function that can be changed for
different throttling implementations, as will be done in following commits.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 monitor.c    | 85 +++++++++++++++++++++++++++++++++++++++---------------------
 trace-events |  3 ++-
 2 files changed, 57 insertions(+), 31 deletions(-)

diff --git a/monitor.c b/monitor.c
index 2d2e030..e78ecc2 100644
--- a/monitor.c
+++ b/monitor.c
@@ -183,6 +183,8 @@ typedef struct {
 
 typedef struct MonitorQAPIEventState MonitorQAPIEventState;
 
+typedef bool (*MonitorQAPIEventDelay)(MonitorQAPIEventState *evstate,
+                                      QDict *data);
 /*
  * To prevent flooding clients, events can be throttled. The
  * throttling is calculated globally, rather than per-Monitor
@@ -190,6 +192,7 @@ typedef struct MonitorQAPIEventState MonitorQAPIEventState;
  */
 struct MonitorQAPIEventState {
     int64_t rate;       /* Minimum time (in ns) between two events */
+    MonitorQAPIEventDelay delay;
     void *delay_data;   /* data for the throttle handler */
 };
 
@@ -463,50 +466,70 @@ static void monitor_qapi_event_emit(QAPIEvent event, QObject *data)
 }
 
 /*
- * Queue a new event for emission to Monitor instances,
- * applying any rate limiting if required.
+ * A simple delay handler, that will delay all events according to the
+ * evstate->rate limit.
+ *
+ * Return 'false' if the event is not delayed and can be emitted now.
  */
-static void
-monitor_qapi_event_queue(QAPIEvent event, QDict *data, Error **errp)
+static bool
+monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data)
 {
-    MonitorQAPIEventState *evstate;
-    MonitorQAPIEventPending *p;
-    assert(event < QAPI_EVENT_MAX);
     int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
+    MonitorQAPIEventPending *p = evstate->delay_data;
+    int64_t delta = now - p->last;
 
-    evstate = &monitor_qapi_event_state[event];
-    p = evstate->delay_data;
-    trace_monitor_protocol_event_queue(event,
+    trace_monitor_protocol_event_delay(p->event,
                                        data,
                                        evstate->rate,
                                        p->last,
                                        now);
 
     /* Rate limit of 0 indicates no throttling */
-    qemu_mutex_lock(&monitor_lock);
     if (!evstate->rate) {
-        monitor_qapi_event_emit(event, QOBJECT(data));
-    } else {
-        int64_t delta = now - p->last;
-        if (evstate->delay_data ||
-            delta < evstate->rate) {
-            /* If there's an existing event pending, replace
-             * it with the new event, otherwise schedule a
-             * timer for delayed emission
-             */
-            if (evstate->delay_data) {
-                qobject_decref(evstate->delay_data);
-            } else {
-                int64_t then = p->last + evstate->rate;
-                timer_mod_ns(p->timer, then);
-            }
-            evstate->delay_data = QOBJECT(data);
-            qobject_incref(evstate->delay_data);
+        p->last = now;
+        return false;
+    }
+
+    if (p->data || delta < evstate->rate) {
+        /* If there's an existing event pending, replace
+         * it with the new event, otherwise schedule a
+         * timer for delayed emission
+         */
+        if (p->data) {
+            qobject_decref(p->data);
         } else {
-            monitor_qapi_event_emit(event, QOBJECT(data));
-            p->last = now;
+            int64_t then = p->last + evstate->rate;
+            timer_mod_ns(p->timer, then);
         }
+        p->data = QOBJECT(data);
+        qobject_incref(p->data);
+        return true;
     }
+
+    p->last = now;
+    return false;
+}
+
+/*
+ * Queue a new event for emission to Monitor instances,
+ * applying any rate limiting if required.
+ */
+static void
+monitor_qapi_event_queue(QAPIEvent event, QDict *data, Error **errp)
+{
+    MonitorQAPIEventState *evstate;
+    assert(event < QAPI_EVENT_MAX);
+
+    evstate = &monitor_qapi_event_state[event];
+    trace_monitor_protocol_event_queue(event, data);
+
+    qemu_mutex_lock(&monitor_lock);
+
+    if (!evstate->delay ||
+        !evstate->delay(evstate, data)) {
+        monitor_qapi_event_emit(event, QOBJECT(data));
+    }
+
     qemu_mutex_unlock(&monitor_lock);
 }
 
@@ -566,6 +589,8 @@ monitor_qapi_event_throttle(QAPIEvent event, int64_t rate)
     trace_monitor_protocol_event_throttle(event, rate);
     assert(rate * SCALE_MS <= INT64_MAX);
     evstate->rate = rate * SCALE_MS;
+
+    evstate->delay = monitor_qapi_event_delay;
     evstate->delay_data = monitor_qapi_event_pending_new(event);
 }
 
diff --git a/trace-events b/trace-events
index 88a2f14..20eb8bf 100644
--- a/trace-events
+++ b/trace-events
@@ -1035,7 +1035,8 @@ 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_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_delay(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 *data) "event=%d data=%p"
 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 v2 2/5] monitor: introduce MonitorQAPIEventDelay callback

[Daniel P. Berrange]

On Thu, Sep 17, 2015 at 06:08:47PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Move the delay handling in a seperate function that can be changed for
> different throttling implementations, as will be done in following commits.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c    | 85 +++++++++++++++++++++++++++++++++++++++---------------------
>  trace-events |  3 ++-
>  2 files changed, 57 insertions(+), 31 deletions(-)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>

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

[Qemu-devel] [PATCH v2 3/5] monitor: rename QDict *data->qdict

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

As suggested by Markus Armbruster, this is a bit more specific for the
event qdict than 'data'.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 monitor.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/monitor.c b/monitor.c
index e78ecc2..2f8af5b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -178,13 +178,13 @@ typedef struct {
     QAPIEvent event;    /* Event being tracked */
     int64_t last;       /* QEMU_CLOCK_REALTIME value at last emission */
     QEMUTimer *timer;   /* Timer for handling delayed events */
-    QObject *data;      /* Event pending delayed dispatch */
+    QObject *qdict;     /* Event pending delayed dispatch */
 } MonitorQAPIEventPending;
 
 typedef struct MonitorQAPIEventState MonitorQAPIEventState;
 
 typedef bool (*MonitorQAPIEventDelay)(MonitorQAPIEventState *evstate,
-                                      QDict *data);
+                                      QDict *qdict);
 /*
  * To prevent flooding clients, events can be throttled. The
  * throttling is calculated globally, rather than per-Monitor
@@ -472,14 +472,14 @@ static void monitor_qapi_event_emit(QAPIEvent event, QObject *data)
  * Return 'false' if the event is not delayed and can be emitted now.
  */
 static bool
-monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data)
+monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *qdict)
 {
     int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
     MonitorQAPIEventPending *p = evstate->delay_data;
     int64_t delta = now - p->last;
 
     trace_monitor_protocol_event_delay(p->event,
-                                       data,
+                                       qdict,
                                        evstate->rate,
                                        p->last,
                                        now);
@@ -490,19 +490,19 @@ monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data)
         return false;
     }
 
-    if (p->data || delta < evstate->rate) {
+    if (p->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 (p->data) {
-            qobject_decref(p->data);
+        if (p->qdict) {
+            qobject_decref(p->qdict);
         } else {
             int64_t then = p->last + evstate->rate;
             timer_mod_ns(p->timer, then);
         }
-        p->data = QOBJECT(data);
-        qobject_incref(p->data);
+        p->qdict = QOBJECT(qdict);
+        qobject_incref(p->qdict);
         return true;
     }
 
@@ -515,19 +515,19 @@ monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *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);
 
     evstate = &monitor_qapi_event_state[event];
-    trace_monitor_protocol_event_queue(event, data);
+    trace_monitor_protocol_event_queue(event, qdict);
 
     qemu_mutex_lock(&monitor_lock);
 
     if (!evstate->delay ||
-        !evstate->delay(evstate, data)) {
-        monitor_qapi_event_emit(event, QOBJECT(data));
+        !evstate->delay(evstate, qdict)) {
+        monitor_qapi_event_emit(event, QOBJECT(qdict));
     }
 
     qemu_mutex_unlock(&monitor_lock);
@@ -543,14 +543,14 @@ static void monitor_qapi_event_handler(void *opaque)
     int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
 
     trace_monitor_protocol_event_handler(p->event,
-                                         p->data,
+                                         p->qdict,
                                          p->last,
                                          now);
     qemu_mutex_lock(&monitor_lock);
-    if (p->data) {
-        monitor_qapi_event_emit(p->event, p->data);
-        qobject_decref(p->data);
-        p->data = NULL;
+    if (p->qdict) {
+        monitor_qapi_event_emit(p->event, p->qdict);
+        qobject_decref(p->qdict);
+        p->qdict = NULL;
     }
     p->last = now;
     qemu_mutex_unlock(&monitor_lock);
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 3/5] monitor: rename QDict *data->qdict

[Daniel P. Berrange]

On Thu, Sep 17, 2015 at 06:08:48PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> As suggested by Markus Armbruster, this is a bit more specific for the
> event qdict than 'data'.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>


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

[Qemu-devel] [PATCH v2 4/5] monitor: throttle QAPI_EVENT_VSERPORT_CHANGE by "id"

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Use a hash table to lookup the pending event corresponding to the "id"
field. The hash table may grow without limit here, the following patch
will add some cleaning.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 monitor.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 81 insertions(+), 23 deletions(-)

diff --git a/monitor.c b/monitor.c
index 2f8af5b..90f06ce 100644
--- a/monitor.c
+++ b/monitor.c
@@ -472,10 +472,10 @@ static void monitor_qapi_event_emit(QAPIEvent event, QObject *data)
  * Return 'false' if the event is not delayed and can be emitted now.
  */
 static bool
-monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *qdict)
+monitor_qapi_event_pending_update(MonitorQAPIEventState *evstate,
+                                  MonitorQAPIEventPending *p, QDict *qdict)
 {
     int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
-    MonitorQAPIEventPending *p = evstate->delay_data;
     int64_t delta = now - p->last;
 
     trace_monitor_protocol_event_delay(p->event,
@@ -510,27 +510,11 @@ monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *qdict)
     return false;
 }
 
-/*
- * Queue a new event for emission to Monitor instances,
- * applying any rate limiting if required.
- */
-static void
-monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp)
+static bool
+monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *qdict)
 {
-    MonitorQAPIEventState *evstate;
-    assert(event < QAPI_EVENT_MAX);
-
-    evstate = &monitor_qapi_event_state[event];
-    trace_monitor_protocol_event_queue(event, qdict);
-
-    qemu_mutex_lock(&monitor_lock);
-
-    if (!evstate->delay ||
-        !evstate->delay(evstate, qdict)) {
-        monitor_qapi_event_emit(event, QOBJECT(qdict));
-    }
-
-    qemu_mutex_unlock(&monitor_lock);
+    return monitor_qapi_event_pending_update(evstate,
+                                             evstate->delay_data, qdict);
 }
 
 /*
@@ -571,6 +555,62 @@ monitor_qapi_event_pending_new(QAPIEvent event)
 }
 
 /*
+ * A delay handler that will filter events by the "id" event field.
+ * evstate must be an element of the monitor_qapi_event_state array.
+ *
+ * Return 'false' if the event is not delayed and can be emitted now.
+ */
+static bool
+monitor_qapi_event_id_delay(MonitorQAPIEventState *evstate, QDict *qdict)
+{
+    GHashTable *ht = evstate->delay_data;
+    QDict *data = qdict_get_qdict(qdict, "data");
+    const char *id = qdict_get_str(data, "id");
+    MonitorQAPIEventPending *p = g_hash_table_lookup(ht, id);
+    QAPIEvent event = evstate - monitor_qapi_event_state;
+    assert(event >= 0 || event < QAPI_EVENT_MAX);
+
+    if (!p) {
+        p = monitor_qapi_event_pending_new(event);
+        g_hash_table_insert(ht, g_strdup(id), p);
+    }
+
+    return monitor_qapi_event_pending_update(evstate, p, qdict);
+}
+
+/*
+ * Queue a new event for emission to Monitor instances,
+ * applying any rate limiting if required.
+ */
+static void
+monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp)
+{
+    MonitorQAPIEventState *evstate;
+    assert(event < QAPI_EVENT_MAX);
+
+    evstate = &(monitor_qapi_event_state[event]);
+    trace_monitor_protocol_event_queue(event, qdict);
+
+    qemu_mutex_lock(&monitor_lock);
+
+    if (!evstate->delay ||
+        !evstate->delay(evstate, qdict)) {
+        monitor_qapi_event_emit(event, QOBJECT(qdict));
+    }
+
+    qemu_mutex_unlock(&monitor_lock);
+}
+
+static void
+monitor_qapi_event_pending_free(MonitorQAPIEventPending *p)
+{
+    qobject_decref(p->qdict);
+    timer_del(p->timer);
+    timer_free(p->timer);
+    g_free(p);
+}
+
+/*
  * @event: the event ID to be limited
  * @rate: the rate limit in milliseconds
  *
@@ -594,6 +634,24 @@ monitor_qapi_event_throttle(QAPIEvent event, int64_t rate)
     evstate->delay_data = monitor_qapi_event_pending_new(event);
 }
 
+static void
+monitor_qapi_event_id_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);
+    assert(rate * SCALE_MS <= INT64_MAX);
+    evstate->rate = rate * SCALE_MS;
+
+    evstate->delay = monitor_qapi_event_id_delay;
+    evstate->delay_data =
+        g_hash_table_new_full(g_str_hash, g_str_equal, g_free,
+            (GDestroyNotify)monitor_qapi_event_pending_free);
+}
+
 static void monitor_qapi_event_init(void)
 {
     /* Limit guest-triggerable events to 1 per second */
@@ -602,7 +660,7 @@ static void monitor_qapi_event_init(void)
     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);
+    monitor_qapi_event_id_throttle(QAPI_EVENT_VSERPORT_CHANGE, 1000);
 
     qmp_event_set_func_emit(monitor_qapi_event_queue);
 }
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 4/5] monitor: throttle QAPI_EVENT_VSERPORT_CHANGE by "id"

[Daniel P. Berrange]

On Thu, Sep 17, 2015 at 06:08:49PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Use a hash table to lookup the pending event corresponding to the "id"
> field. The hash table may grow without limit here, the following patch
> will add some cleaning.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  monitor.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 81 insertions(+), 23 deletions(-)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>


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

[Qemu-devel] [PATCH v2 5/5] monitor: remove old entries from event hash table

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Do not let the hash table grow without limit, schedule a cleanup for
outdated event.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 monitor.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 53 insertions(+), 5 deletions(-)

diff --git a/monitor.c b/monitor.c
index 90f06ce..a81341f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -541,7 +541,7 @@ static void monitor_qapi_event_handler(void *opaque)
 }
 
 static MonitorQAPIEventPending *
-monitor_qapi_event_pending_new(QAPIEvent event)
+monitor_qapi_event_pending_new(QAPIEvent event, QEMUTimerCB *handler)
 {
     MonitorQAPIEventPending *p;
 
@@ -549,11 +549,52 @@ monitor_qapi_event_pending_new(QAPIEvent event)
     p->event = event;
     p->timer = timer_new(QEMU_CLOCK_REALTIME,
                          SCALE_MS,
-                         monitor_qapi_event_handler,
+                         handler,
                          p);
     return p;
 }
 
+static void monitor_qapi_event_id_remove(MonitorQAPIEventPending *p)
+{
+    MonitorQAPIEventState *s = &monitor_qapi_event_state[p->event];
+    GHashTable *ht = s->delay_data;
+    GHashTableIter iter;
+    gpointer value;
+
+    g_hash_table_iter_init(&iter, ht);
+    while (g_hash_table_iter_next(&iter, NULL, &value)) {
+        if (value == p) {
+            g_hash_table_iter_remove(&iter);
+            return;
+        }
+    }
+}
+
+/*
+ * do not let the hash table grow, if no later pending event
+ * scheduled, remove the old entry after rate timeout.
+ */
+static void monitor_qapi_event_id_schedule_remove(MonitorQAPIEventPending *p)
+{
+    MonitorQAPIEventState *s = &monitor_qapi_event_state[p->event];
+    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
+    int64_t then = now + s->rate;
+
+    timer_mod_ns(p->timer, then);
+}
+
+static void monitor_qapi_event_id_handler(void *opaque)
+{
+    MonitorQAPIEventPending *p = opaque;
+
+    if (p->qdict) {
+        monitor_qapi_event_handler(p);
+        monitor_qapi_event_id_schedule_remove(p);
+    } else {
+        monitor_qapi_event_id_remove(p);
+    }
+}
+
 /*
  * A delay handler that will filter events by the "id" event field.
  * evstate must be an element of the monitor_qapi_event_state array.
@@ -571,11 +612,17 @@ monitor_qapi_event_id_delay(MonitorQAPIEventState *evstate, QDict *qdict)
     assert(event >= 0 || event < QAPI_EVENT_MAX);
 
     if (!p) {
-        p = monitor_qapi_event_pending_new(event);
+        p = monitor_qapi_event_pending_new(event,
+                                           monitor_qapi_event_id_handler);
         g_hash_table_insert(ht, g_strdup(id), p);
     }
 
-    return monitor_qapi_event_pending_update(evstate, p, qdict);
+    if (monitor_qapi_event_pending_update(evstate, p, qdict)) {
+        return true;
+    } else {
+        monitor_qapi_event_id_schedule_remove(p);
+        return false;
+    }
 }
 
 /*
@@ -631,7 +678,8 @@ monitor_qapi_event_throttle(QAPIEvent event, int64_t rate)
     evstate->rate = rate * SCALE_MS;
 
     evstate->delay = monitor_qapi_event_delay;
-    evstate->delay_data = monitor_qapi_event_pending_new(event);
+    evstate->delay_data =
+        monitor_qapi_event_pending_new(event, monitor_qapi_event_handler);
 }
 
 static void
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 5/5] monitor: remove old entries from event hash table

[Daniel P. Berrange]

On Thu, Sep 17, 2015 at 06:08:50PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Do not let the hash table grow without limit, schedule a cleanup for
> outdated event.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 53 insertions(+), 5 deletions(-)

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>


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

Re: [Qemu-devel] [PATCH v2 0/5] monitor: throttle VSERPORT_CHANGED by "id"

[Marc-André Lureau]

ping

On Thu, Sep 17, 2015 at 6:08 PM,  <marcandre.lureau@redhat.com> wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> QAPI_EVENT_VSERPORT_CHANGE reports changes of a virtio serial port
> state. However, the events may be for different ports, but the throttle
> mechanism may replace the event for a different port, since it only
> checks the event type.
>
> The following series implements throttling of events based on the "id"
> field. Hopefully this hash table approach can be later extended if
> other fields or combination of fields have to be used.
>
> v1->v2:
> - split first patch in 2 to ease review
> - remove some extra space
> - add some comments above delay handler function, and struct fields
> - rename the delay handler data "delay_data"
> - add a trace in monitor_protocol_event_delay()
> - improve some commit messages
> - simplify monitor_qapi_event_delay()
> - add some comment assert code in monitor_qapi_event_id_delay() to
>   ensure the given pending struct is valid
> - fixed hashtable key leak
> - rename qdict "data" argument to "qdict"
> - removed superfluous parenthesis
> - use a single timer handler for doing "id" filtering cleanup
>
> Marc-André Lureau (5):
>   monitor: split MonitorQAPIEventState
>   monitor: introduce MonitorQAPIEventDelay callback
>   monitor: rename QDict *data->qdict
>   monitor: throttle QAPI_EVENT_VSERPORT_CHANGE by "id"
>   monitor: remove old entries from event hash table
>
>  monitor.c    | 256 ++++++++++++++++++++++++++++++++++++++++++++++-------------
>  trace-events |   3 +-
>  2 files changed, 203 insertions(+), 56 deletions(-)
>
> --
> 2.4.3
>



-- 
Marc-André Lureau