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

[Qemu-devel] [PATCH 0/3] 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.

rfc->v1:
- fix uppercase FALSE/TRUE and gpointer (glib-ism)
- spelling fix

Marc-André Lureau (3):
  monitor: split MonitorQAPIEventState
  monitor: throttle QAPI_EVENT_VSERPORT_CHANGE by "id"
  monitor: remove old entries from event hash table

 monitor.c    | 245 +++++++++++++++++++++++++++++++++++++++++++++--------------
 trace-events |   2 +-
 2 files changed, 188 insertions(+), 59 deletions(-)

-- 
2.4.3

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

[marcandre.lureau]

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

Create a separate pending event structure MonitorQAPIEventPending.
Use a MonitorQAPIEventDelay callback to handle the delaying. This
allows other implementations of throttling.

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

diff --git a/monitor.c b/monitor.c
index fc32f12..0a6a16a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -170,18 +170,27 @@ 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;
+
+typedef bool (*MonitorQAPIEventDelay) (MonitorQAPIEventState *evstate,
+                                       QDict *data);
 /*
  * 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;
+    MonitorQAPIEventDelay delay;
+    void *data;
+};
 
 struct Monitor {
     CharDriverState *chr;
@@ -452,6 +461,39 @@ static void monitor_qapi_event_emit(QAPIEvent event, QObject *data)
     }
 }
 
+static bool
+monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data)
+{
+    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
+    MonitorQAPIEventPending *p = evstate->data;
+    int64_t delta = now - p->last;
+
+    /* Rate limit of 0 indicates no throttling */
+    if (!evstate->rate) {
+        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 {
+            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.
@@ -467,35 +509,15 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *data, Error **errp)
     trace_monitor_protocol_event_queue(event,
                                        data,
                                        evstate->rate,
-                                       evstate->last,
                                        now);
 
-    /* Rate limit of 0 indicates no throttling */
     qemu_mutex_lock(&monitor_lock);
-    if (!evstate->rate) {
+
+    if (!evstate->delay ||
+        !evstate->delay(evstate, data)) {
         monitor_qapi_event_emit(event, QOBJECT(data));
-        evstate->last = now;
-    } else {
-        int64_t delta = now - evstate->last;
-        if (evstate->data ||
-            delta < evstate->rate) {
-            /* If there's an existing event pending, replace
-             * it with the new event, otherwise schedule a
-             * timer for delayed emission
-             */
-            if (evstate->data) {
-                qobject_decref(evstate->data);
-            } else {
-                int64_t then = evstate->last + evstate->rate;
-                timer_mod_ns(evstate->timer, then);
-            }
-            evstate->data = QOBJECT(data);
-            qobject_incref(evstate->data);
-        } else {
-            monitor_qapi_event_emit(event, QOBJECT(data));
-            evstate->last = now;
-        }
     }
+
     qemu_mutex_unlock(&monitor_lock);
 }
 
@@ -505,23 +527,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
@@ -539,15 +575,11 @@ monitor_qapi_event_throttle(QAPIEvent event, int64_t rate)
     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 = monitor_qapi_event_delay;
+    evstate->data = monitor_qapi_event_pending_new(event);
 }
 
 static void monitor_qapi_event_init(void)
diff --git a/trace-events b/trace-events
index 8f9614a..b1ea596 100644
--- a/trace-events
+++ b/trace-events
@@ -1028,7 +1028,7 @@ 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_queue(uint32_t event, void *data, uint64_t rate, uint64_t now) "event=%d data=%p rate=%" PRId64 " now=%" PRId64
 monitor_protocol_event_throttle(uint32_t event, uint64_t rate) "event=%d rate=%" PRId64
 
 # hw/net/opencores_eth.c
-- 
2.4.3

[Qemu-devel] [PATCH 2/3] 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, 78 insertions(+), 26 deletions(-)

diff --git a/monitor.c b/monitor.c
index 0a6a16a..7f5c80f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -462,10 +462,10 @@ static void monitor_qapi_event_emit(QAPIEvent event, QObject *data)
 }
 
 static bool
-monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data)
+monitor_qapi_event_pending_update(MonitorQAPIEventState *evstate,
+                                  MonitorQAPIEventPending *p, QDict *data)
 {
     int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
-    MonitorQAPIEventPending *p = evstate->data;
     int64_t delta = now - p->last;
 
     /* Rate limit of 0 indicates no throttling */
@@ -494,31 +494,13 @@ monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data)
     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);
-    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
-
-    evstate = &(monitor_qapi_event_state[event]);
-    trace_monitor_protocol_event_queue(event,
-                                       data,
-                                       evstate->rate,
-                                       now);
 
-    qemu_mutex_lock(&monitor_lock);
-
-    if (!evstate->delay ||
-        !evstate->delay(evstate, data)) {
-        monitor_qapi_event_emit(event, QOBJECT(data));
-    }
+static bool
+monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data)
+{
+    MonitorQAPIEventPending *p = evstate->data;
 
-    qemu_mutex_unlock(&monitor_lock);
+    return monitor_qapi_event_pending_update(evstate, p, data);
 }
 
 /*
@@ -558,6 +540,58 @@ monitor_qapi_event_pending_new(QAPIEvent event)
     return p;
 }
 
+static bool
+monitor_qapi_event_id_delay(MonitorQAPIEventState *evstate, QDict *data)
+{
+    GHashTable *ht = evstate->data;
+    QDict *s = qdict_get_qdict(data, "data");
+    const char *id = qdict_get_str(s, "id");
+    MonitorQAPIEventPending *p = g_hash_table_lookup(ht, id);
+    QAPIEvent event = evstate - monitor_qapi_event_state;
+
+    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, data);
+}
+
+/*
+ * 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);
+    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
+
+    evstate = &(monitor_qapi_event_state[event]);
+    trace_monitor_protocol_event_queue(event,
+                                       data,
+                                       evstate->rate,
+                                       now);
+
+    qemu_mutex_lock(&monitor_lock);
+
+    if (!evstate->delay ||
+        !evstate->delay(evstate, data)) {
+        monitor_qapi_event_emit(event, QOBJECT(data));
+    }
+
+    qemu_mutex_unlock(&monitor_lock);
+}
+
+static void
+monitor_qapi_event_pending_free(MonitorQAPIEventPending *p)
+{
+    timer_del(p->timer);
+    timer_free(p->timer);
+    g_free(p);
+}
+
 /*
  * @event: the event ID to be limited
  * @rate: the rate limit in milliseconds
@@ -582,6 +616,24 @@ monitor_qapi_event_throttle(QAPIEvent event, int64_t rate)
     evstate->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->data =
+        g_hash_table_new_full(g_str_hash, g_str_equal, NULL,
+            (GDestroyNotify)monitor_qapi_event_pending_free);
+}
+
 static void monitor_qapi_event_init(void)
 {
     /* Limit guest-triggerable events to 1 per second */
@@ -590,7 +642,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

[Qemu-devel] [PATCH 3/3] 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 | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 46 insertions(+), 1 deletion(-)

diff --git a/monitor.c b/monitor.c
index 7f5c80f..8d0c61b 100644
--- a/monitor.c
+++ b/monitor.c
@@ -540,6 +540,45 @@ monitor_qapi_event_pending_new(QAPIEvent event)
     return p;
 }
 
+static void monitor_qapi_event_id_remove(void *opaque)
+{
+    MonitorQAPIEventPending *p = opaque;
+    MonitorQAPIEventState *s = &monitor_qapi_event_state[p->event];
+    GHashTable *ht = s->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;
+
+    p->timer->cb = monitor_qapi_event_id_remove;
+    timer_mod_ns(p->timer, then);
+}
+
+static void monitor_qapi_event_id_handler(void *opaque)
+{
+    MonitorQAPIEventPending *p = opaque;
+
+    monitor_qapi_event_handler(p);
+    monitor_qapi_event_id_schedule_remove(p);
+}
+
 static bool
 monitor_qapi_event_id_delay(MonitorQAPIEventState *evstate, QDict *data)
 {
@@ -554,7 +593,13 @@ monitor_qapi_event_id_delay(MonitorQAPIEventState *evstate, QDict *data)
         g_hash_table_insert(ht, g_strdup(id), p);
     }
 
-    return monitor_qapi_event_pending_update(evstate, p, data);
+    if (monitor_qapi_event_pending_update(evstate, p, data)) {
+        p->timer->cb = monitor_qapi_event_id_handler;
+        return true;
+    } else {
+        monitor_qapi_event_id_schedule_remove(p);
+        return false;
+    }
 }
 
 /*
-- 
2.4.3

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

[Marc-André Lureau]

ping

On Wed, Sep 2, 2015 at 1:09 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.
>
> rfc->v1:
> - fix uppercase FALSE/TRUE and gpointer (glib-ism)
> - spelling fix
>
> Marc-André Lureau (3):
>   monitor: split MonitorQAPIEventState
>   monitor: throttle QAPI_EVENT_VSERPORT_CHANGE by "id"
>   monitor: remove old entries from event hash table
>
>  monitor.c    | 245 +++++++++++++++++++++++++++++++++++++++++++++--------------
>  trace-events |   2 +-
>  2 files changed, 188 insertions(+), 59 deletions(-)
>
> --
> 2.4.3
>



-- 
Marc-André Lureau

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

[Amit Shah]

On (Tue) 08 Sep 2015 [12:18:53], Marc-André Lureau wrote:
> ping

I'd rather the qmp / qapi maintainers chimed in - the changes touch
that part of code rather than the virtio-serial related code.

> 
> On Wed, Sep 2, 2015 at 1:09 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.
> >
> > rfc->v1:
> > - fix uppercase FALSE/TRUE and gpointer (glib-ism)
> > - spelling fix
> >
> > Marc-André Lureau (3):
> >   monitor: split MonitorQAPIEventState
> >   monitor: throttle QAPI_EVENT_VSERPORT_CHANGE by "id"
> >   monitor: remove old entries from event hash table
> >
> >  monitor.c    | 245 +++++++++++++++++++++++++++++++++++++++++++++--------------
> >  trace-events |   2 +-
> >  2 files changed, 188 insertions(+), 59 deletions(-)
> >
> > --
> > 2.4.3
> >
> 
> 
> 
> -- 
> Marc-André Lureau

		Amit

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

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> On (Tue) 08 Sep 2015 [12:18:53], Marc-André Lureau wrote:
>> ping
>
> I'd rather the qmp / qapi maintainers chimed in - the changes touch
> that part of code rather than the virtio-serial related code.

It's in my (overlong) review queue.  I apologize for the delay.

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

[Daniel P. Berrange]

On Wed, Sep 02, 2015 at 01:09:41PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Create a separate pending event structure MonitorQAPIEventPending.
> Use a MonitorQAPIEventDelay callback to handle the delaying. This
> allows other implementations of throttling.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c    | 124 +++++++++++++++++++++++++++++++++++++----------------------
>  trace-events |   2 +-
>  2 files changed, 79 insertions(+), 47 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 2/3] monitor: throttle QAPI_EVENT_VSERPORT_CHANGE by "id"

[Daniel P. Berrange]

On Wed, Sep 02, 2015 at 01:09:42PM +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, 78 insertions(+), 26 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 3/3] monitor: remove old entries from event hash table

[Daniel P. Berrange]

On Wed, Sep 02, 2015 at 01:09:43PM +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 | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 46 insertions(+), 1 deletion(-)

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 1/3] monitor: split MonitorQAPIEventState

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Create a separate pending event structure MonitorQAPIEventPending.
> Use a MonitorQAPIEventDelay callback to handle the delaying. This
> allows other implementations of throttling.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c    | 124 +++++++++++++++++++++++++++++++++++++----------------------
>  trace-events |   2 +-
>  2 files changed, 79 insertions(+), 47 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index fc32f12..0a6a16a 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -170,18 +170,27 @@ 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;

"MonitorQAPIEventPending" sounds like the name of a predicate "is an
event pending?"  MonitorQAPIPendingEvent?

> +
> +typedef struct MonitorQAPIEventState MonitorQAPIEventState;
> +
> +typedef bool (*MonitorQAPIEventDelay) (MonitorQAPIEventState *evstate,
> +                                       QDict *data);

Style nit: we don't normally have space before an argument list.

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

Since your commit message is so sparse, I have to actually think to
figure out how this patch works.  To think, I have to write.  And when I
write, I can just as well send it out, so you can correct my thinking.

If you want less verbose reviews from me, try writing more verbose
commit messages :)

Obvious: event, last, timer and data move into MonitorQAPIEventDelay.

Not obvious (yet): how to reach them.  Read on and see, I guess.

> +    void *data;

What does data point to?

Why does it have to be void *?

> +};
>  
>  struct Monitor {
>      CharDriverState *chr;
> @@ -452,6 +461,39 @@ static void monitor_qapi_event_emit(QAPIEvent event, QObject *data)
>      }
>  }
>  
> +static bool
> +monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data)
> +{
> +    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> +    MonitorQAPIEventPending *p = evstate->data;
> +    int64_t delta = now - p->last;
> +
> +    /* Rate limit of 0 indicates no throttling */
> +    if (!evstate->rate) {
> +        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 {
> +            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;
> +}
> +

Okay, this is the part of monitor_qapi_event_queue() protected by the
lock, less the monitor_qapi_event_emit(), with the move of last, time
and data from evstate to p squashed in, and the computation of delta
moved up some.  Would be easier to review if you did the transformations
in separate patches.

Could use a function comment, because the return value isn't obvious.

Also: too many things are named "data" for my taste.

>  /*
>   * Queue a new event for emission to Monitor instances,
>   * applying any rate limiting if required.
> @@ -467,35 +509,15 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *data, Error **errp)
>      trace_monitor_protocol_event_queue(event,
>                                         data,
>                                         evstate->rate,
> -                                       evstate->last,
>                                         now);

Should monitor_qapi_event_delay() trace evstate->last to compensate?

>  
> -    /* Rate limit of 0 indicates no throttling */
>      qemu_mutex_lock(&monitor_lock);
> -    if (!evstate->rate) {
> +
> +    if (!evstate->delay ||
> +        !evstate->delay(evstate, data)) {

Sure evstate->delay can be null?

>          monitor_qapi_event_emit(event, QOBJECT(data));
> -        evstate->last = now;
> -    } else {
> -        int64_t delta = now - evstate->last;
> -        if (evstate->data ||
> -            delta < evstate->rate) {
> -            /* If there's an existing event pending, replace
> -             * it with the new event, otherwise schedule a
> -             * timer for delayed emission
> -             */
> -            if (evstate->data) {
> -                qobject_decref(evstate->data);
> -            } else {
> -                int64_t then = evstate->last + evstate->rate;
> -                timer_mod_ns(evstate->timer, then);
> -            }
> -            evstate->data = QOBJECT(data);
> -            qobject_incref(evstate->data);
> -        } else {
> -            monitor_qapi_event_emit(event, QOBJECT(data));
> -            evstate->last = now;
> -        }
>      }
> +
>      qemu_mutex_unlock(&monitor_lock);
>  }
>  
> @@ -505,23 +527,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
> @@ -539,15 +575,11 @@ monitor_qapi_event_throttle(QAPIEvent event, int64_t rate)
>      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 = monitor_qapi_event_delay;
> +    evstate->data = monitor_qapi_event_pending_new(event);
>  }
>  
>  static void monitor_qapi_event_init(void)

All right, the moved members end up in evstate->data, and we now
indirect the interesting part of monitor_qapi_event_queue() through
evstate->delay.

Why this is useful isn't obvious, yet.  The only clue I have is the
commit message's "This allows other implementations of throttling."  I'd
add something like "The next few commits will add one."

> diff --git a/trace-events b/trace-events
> index 8f9614a..b1ea596 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -1028,7 +1028,7 @@ 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_queue(uint32_t event, void *data, uint64_t rate, uint64_t now) "event=%d data=%p rate=%" PRId64 " now=%" PRId64
>  monitor_protocol_event_throttle(uint32_t event, uint64_t rate) "event=%d rate=%" PRId64
>  
>  # hw/net/opencores_eth.c

Re: [Qemu-devel] [PATCH 2/3] monitor: throttle QAPI_EVENT_VSERPORT_CHANGE by "id"

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> 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, 78 insertions(+), 26 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 0a6a16a..7f5c80f 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -462,10 +462,10 @@ static void monitor_qapi_event_emit(QAPIEvent event, QObject *data)
>  }
>  
>  static bool
> -monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data)
> +monitor_qapi_event_pending_update(MonitorQAPIEventState *evstate,
> +                                  MonitorQAPIEventPending *p, QDict *data)

Function name gets even longer, yet I miss a function comment as much as
before :)

Since this is the common part of monitor_qapi_event_delay() and
monitor_qapi_event_id_delay(), what about calling it
monitor_qapi_event_do_delay()?

>  {
>      int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> -    MonitorQAPIEventPending *p = evstate->data;
>      int64_t delta = now - p->last;
>  
>      /* Rate limit of 0 indicates no throttling */
> @@ -494,31 +494,13 @@ monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data)
>      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);
> -    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> -
> -    evstate = &(monitor_qapi_event_state[event]);
> -    trace_monitor_protocol_event_queue(event,
> -                                       data,
> -                                       evstate->rate,
> -                                       now);
>  
> -    qemu_mutex_lock(&monitor_lock);
> -
> -    if (!evstate->delay ||
> -        !evstate->delay(evstate, data)) {
> -        monitor_qapi_event_emit(event, QOBJECT(data));
> -    }

No change, just diff being insufficiently smart.

> +static bool
> +monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data)
> +{
> +    MonitorQAPIEventPending *p = evstate->data;
>  
> -    qemu_mutex_unlock(&monitor_lock);
> +    return monitor_qapi_event_pending_update(evstate, p, data);
>  }

Why not simply

    return monitor_qapi_event_pending_update(evstate, evstate->data, data);

>  
>  /*
> @@ -558,6 +540,58 @@ monitor_qapi_event_pending_new(QAPIEvent event)
>      return p;
>  }
>  
> +static bool
> +monitor_qapi_event_id_delay(MonitorQAPIEventState *evstate, QDict *data)
> +{
> +    GHashTable *ht = evstate->data;
> +    QDict *s = qdict_get_qdict(data, "data");

Why is this called s?

Here are less confusing names:

* The parameter should not be called data, because it's the complete QMP
  event object, and *not* its data member.  Either match
  QMPEventFuncEmit and call it qdict, or call it event.

* The variable s *is* the data member, and should therefore be called data.

> +    const char *id = qdict_get_str(s, "id");

This assumes s has a member "id" of type QString.  Therefore, the
function can only be used for events where that is always the case.
Unobvious requirements like that should be spelled out in a function
comment.

> +    MonitorQAPIEventPending *p = g_hash_table_lookup(ht, id);
> +    QAPIEvent event = evstate - monitor_qapi_event_state;

Another unobvious requirement: evstate must point into
monitor_qapi_event_state.

> +
> +    if (!p) {
> +        p = monitor_qapi_event_pending_new(event);
> +        g_hash_table_insert(ht, g_strdup(id), p);

Note for later: both key and value are dynamically allocated.

> +    }
> +
> +    return monitor_qapi_event_pending_update(evstate, p, data);
> +}
> +
> +/*
> + * 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);
> +    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> +
> +    evstate = &(monitor_qapi_event_state[event]);
> +    trace_monitor_protocol_event_queue(event,
> +                                       data,
> +                                       evstate->rate,
> +                                       now);
> +
> +    qemu_mutex_lock(&monitor_lock);
> +
> +    if (!evstate->delay ||
> +        !evstate->delay(evstate, data)) {
> +        monitor_qapi_event_emit(event, QOBJECT(data));
> +    }
> +
> +    qemu_mutex_unlock(&monitor_lock);
> +}
> +
> +static void
> +monitor_qapi_event_pending_free(MonitorQAPIEventPending *p)
> +{
> +    timer_del(p->timer);
> +    timer_free(p->timer);
> +    g_free(p);

Ignorant question: who owns p->data?

> +}
> +
>  /*
>   * @event: the event ID to be limited
>   * @rate: the rate limit in milliseconds
> @@ -582,6 +616,24 @@ monitor_qapi_event_throttle(QAPIEvent event, int64_t rate)
>      evstate->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]);

Superfluous parenthesis.

> +
> +    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->data =
> +        g_hash_table_new_full(g_str_hash, g_str_equal, NULL,
> +            (GDestroyNotify)monitor_qapi_event_pending_free);

Both key and value are dynamically allocated.  But you define only the
value's free callback.  Why?

> +}
> +
>  static void monitor_qapi_event_init(void)
>  {
>      /* Limit guest-triggerable events to 1 per second */
> @@ -590,7 +642,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);
>  }

I got a few more questions.

Do we expect more events in need of separate throttling by some ID-like
member of their data object?

Do we expect *different* callbacks?  Callbacks that throttle by some
member of their data object won't count as different.

Do we really need the callback and the type punning?  As long as all we
have is the current two throttles, I'd try to do without, as follows.

monitor_qapi_event_throttle() takes the name of a data member as
additional argument.  If null, throttle like monitor_qapi_event_delay().
Else, throttle like monitor_qapi_event_id_delay().  Except do it in a
single function, with a single MonitorQAPIEventState.  The natural
representation of "either a single MonitorQAPIEventPending or a hash
table of them" is a union.

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

[Markus Armbruster]

marcandre.lureau@redhat.com writes:

> 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 | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/monitor.c b/monitor.c
> index 7f5c80f..8d0c61b 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -540,6 +540,45 @@ monitor_qapi_event_pending_new(QAPIEvent event)
>      return p;
>  }
>  
> +static void monitor_qapi_event_id_remove(void *opaque)
> +{
> +    MonitorQAPIEventPending *p = opaque;
> +    MonitorQAPIEventState *s = &monitor_qapi_event_state[p->event];
> +    GHashTable *ht = s->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;
> +
> +    p->timer->cb = monitor_qapi_event_id_remove;

I doubt you're supposed to poke into p->timer like that.

> +    timer_mod_ns(p->timer, then);
> +}
> +
> +static void monitor_qapi_event_id_handler(void *opaque)
> +{
> +    MonitorQAPIEventPending *p = opaque;
> +
> +    monitor_qapi_event_handler(p);
> +    monitor_qapi_event_id_schedule_remove(p);
> +}
> +
>  static bool
>  monitor_qapi_event_id_delay(MonitorQAPIEventState *evstate, QDict *data)
>  {
> @@ -554,7 +593,13 @@ monitor_qapi_event_id_delay(MonitorQAPIEventState *evstate, QDict *data)
>          g_hash_table_insert(ht, g_strdup(id), p);
>      }
>  
> -    return monitor_qapi_event_pending_update(evstate, p, data);
> +    if (monitor_qapi_event_pending_update(evstate, p, data)) {
> +        p->timer->cb = monitor_qapi_event_id_handler;
> +        return true;
> +    } else {
> +        monitor_qapi_event_id_schedule_remove(p);
> +        return false;
> +    }
>  }
>  
>  /*

Possibly cleaner than messing with timer->cb: have a single callback
that does the right thing, i.e. when the timer goes off, check whether
we have an event.  If yes, send it and rearm the timer.  If no, remove
the hash entry.

Re: [Qemu-devel] [PATCH 2/3] monitor: throttle QAPI_EVENT_VSERPORT_CHANGE by "id"

[Marc-André Lureau]

Hi

On Wed, Sep 16, 2015 at 6:40 PM, Markus Armbruster <armbru@redhat.com> wrote:
>
> I got a few more questions.
>
> Do we expect more events in need of separate throttling by some ID-like
> member of their data object?
> Do we expect *different* callbacks?  Callbacks that throttle by some
> member of their data object won't count as different.
>
> Do we really need the callback and the type punning?  As long as all we
> have is the current two throttles, I'd try to do without, as follows.
>

That's not a question you can answer easily, but I can easily imagine
that we may throttle events based on different conditions. So I prefer
a modular code if it's easy to do and hopefully easy to understand.

> monitor_qapi_event_throttle() takes the name of a data member as
> additional argument.  If null, throttle like monitor_qapi_event_delay().
> Else, throttle like monitor_qapi_event_id_delay().  Except do it in a
> single function, with a single MonitorQAPIEventState.  The natural
> representation of "either a single MonitorQAPIEventPending or a hash
> table of them" is a union.

You need a dispatch in a single function then, and it's very much not
modular code that way. I'd prefer we keep the modularity this patch
brings: different functions and structure for different throttle
implementations.

-- 
Marc-André Lureau

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

[Marc-André Lureau]

Hi

On Wed, Sep 16, 2015 at 3:57 PM, Markus Armbruster <armbru@redhat.com> wrote:
> marcandre.lureau@redhat.com writes:
>
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> Create a separate pending event structure MonitorQAPIEventPending.
>> Use a MonitorQAPIEventDelay callback to handle the delaying. This
>> allows other implementations of throttling.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  monitor.c    | 124 +++++++++++++++++++++++++++++++++++++----------------------
>>  trace-events |   2 +-
>>  2 files changed, 79 insertions(+), 47 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index fc32f12..0a6a16a 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -170,18 +170,27 @@ 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;
>
> "MonitorQAPIEventPending" sounds like the name of a predicate "is an
> event pending?"  MonitorQAPIPendingEvent?

I prefer to keep the 'QAPIEvent' together like the rest of the types.
I don't mind changing it though if you think it's a bad idea

>
>> +
>> +typedef struct MonitorQAPIEventState MonitorQAPIEventState;
>> +
>> +typedef bool (*MonitorQAPIEventDelay) (MonitorQAPIEventState *evstate,
>> +                                       QDict *data);
>
> Style nit: we don't normally have space before an argument list.
>

fixed

>>  /*
>>   * 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;
>> +    MonitorQAPIEventDelay delay;
>
> Since your commit message is so sparse, I have to actually think to
> figure out how this patch works.  To think, I have to write.  And when I
> write, I can just as well send it out, so you can correct my thinking.
>
> If you want less verbose reviews from me, try writing more verbose
> commit messages :)

ok

>
> Obvious: event, last, timer and data move into MonitorQAPIEventDelay.
>
> Not obvious (yet): how to reach them.  Read on and see, I guess.
>
>> +    void *data;
>
> What does data point to?
>
> Why does it have to be void *?

to allow other kind of throttling implementation and their own data,
I'll rename it and add a comment.

>
>> +};
>>
>>  struct Monitor {
>>      CharDriverState *chr;
>> @@ -452,6 +461,39 @@ static void monitor_qapi_event_emit(QAPIEvent event, QObject *data)
>>      }
>>  }
>>
>> +static bool
>> +monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data)
>> +{
>> +    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>> +    MonitorQAPIEventPending *p = evstate->data;
>> +    int64_t delta = now - p->last;
>> +
>> +    /* Rate limit of 0 indicates no throttling */
>> +    if (!evstate->rate) {
>> +        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 {
>> +            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;
>> +}
>> +
>
> Okay, this is the part of monitor_qapi_event_queue() protected by the
> lock, less the monitor_qapi_event_emit(), with the move of last, time
> and data from evstate to p squashed in, and the computation of delta
> moved up some.  Would be easier to review if you did the transformations
> in separate patches.
>

ok

> Could use a function comment, because the return value isn't obvious.
>

ok

> Also: too many things are named "data" for my taste.

It's the same QDict *data, ie the actual event qdict. How would you rename it?

I will rename the "delay data".

>>  /*
>>   * Queue a new event for emission to Monitor instances,
>>   * applying any rate limiting if required.
>> @@ -467,35 +509,15 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *data, Error **errp)
>>      trace_monitor_protocol_event_queue(event,
>>                                         data,
>>                                         evstate->rate,
>> -                                       evstate->last,
>>                                         now);
>
> Should monitor_qapi_event_delay() trace evstate->last to compensate?

ok

>
>>
>> -    /* Rate limit of 0 indicates no throttling */
>>      qemu_mutex_lock(&monitor_lock);
>> -    if (!evstate->rate) {
>> +
>> +    if (!evstate->delay ||
>> +        !evstate->delay(evstate, data)) {
>
> Sure evstate->delay can be null?

if no delay

>
>>          monitor_qapi_event_emit(event, QOBJECT(data));
>> -        evstate->last = now;
>> -    } else {
>> -        int64_t delta = now - evstate->last;
>> -        if (evstate->data ||
>> -            delta < evstate->rate) {
>> -            /* If there's an existing event pending, replace
>> -             * it with the new event, otherwise schedule a
>> -             * timer for delayed emission
>> -             */
>> -            if (evstate->data) {
>> -                qobject_decref(evstate->data);
>> -            } else {
>> -                int64_t then = evstate->last + evstate->rate;
>> -                timer_mod_ns(evstate->timer, then);
>> -            }
>> -            evstate->data = QOBJECT(data);
>> -            qobject_incref(evstate->data);
>> -        } else {
>> -            monitor_qapi_event_emit(event, QOBJECT(data));
>> -            evstate->last = now;
>> -        }
>>      }
>> +
>>      qemu_mutex_unlock(&monitor_lock);
>>  }
>>
>> @@ -505,23 +527,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
>> @@ -539,15 +575,11 @@ monitor_qapi_event_throttle(QAPIEvent event, int64_t rate)
>>      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 = monitor_qapi_event_delay;
>> +    evstate->data = monitor_qapi_event_pending_new(event);
>>  }
>>
>>  static void monitor_qapi_event_init(void)
>
> All right, the moved members end up in evstate->data, and we now
> indirect the interesting part of monitor_qapi_event_queue() through
> evstate->delay.
>
> Why this is useful isn't obvious, yet.  The only clue I have is the
> commit message's "This allows other implementations of throttling."  I'd
> add something like "The next few commits will add one."

ok

>> diff --git a/trace-events b/trace-events
>> index 8f9614a..b1ea596 100644
>> --- a/trace-events
>> +++ b/trace-events
>> @@ -1028,7 +1028,7 @@ 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_queue(uint32_t event, void *data, uint64_t rate, uint64_t now) "event=%d data=%p rate=%" PRId64 " now=%" PRId64
>>  monitor_protocol_event_throttle(uint32_t event, uint64_t rate) "event=%d rate=%" PRId64
>>
>>  # hw/net/opencores_eth.c
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 2/3] monitor: throttle QAPI_EVENT_VSERPORT_CHANGE by "id"

[Marc-André Lureau]

Hi

On Wed, Sep 16, 2015 at 6:40 PM, Markus Armbruster <armbru@redhat.com> wrote:
> marcandre.lureau@redhat.com writes:
>
>> 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, 78 insertions(+), 26 deletions(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index 0a6a16a..7f5c80f 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -462,10 +462,10 @@ static void monitor_qapi_event_emit(QAPIEvent event, QObject *data)
>>  }
>>
>>  static bool
>> -monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data)
>> +monitor_qapi_event_pending_update(MonitorQAPIEventState *evstate,
>> +                                  MonitorQAPIEventPending *p, QDict *data)
>
> Function name gets even longer, yet I miss a function comment as much as
> before :)

It's longer simply because the struct name is longer.
The function "update" MonitorQAPIEventPending.

>
> Since this is the common part of monitor_qapi_event_delay() and
> monitor_qapi_event_id_delay(), what about calling it
> monitor_qapi_event_do_delay()?
>

That's confusing, not only it doesn't follow the struct prefix, but
it's also very close to monitor_qapi_event_delay() (the simple
throttle/delay handler)

>>  {
>>      int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>> -    MonitorQAPIEventPending *p = evstate->data;
>>      int64_t delta = now - p->last;
>>
>>      /* Rate limit of 0 indicates no throttling */
>> @@ -494,31 +494,13 @@ monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data)
>>      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);
>> -    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>> -
>> -    evstate = &(monitor_qapi_event_state[event]);
>> -    trace_monitor_protocol_event_queue(event,
>> -                                       data,
>> -                                       evstate->rate,
>> -                                       now);
>>
>> -    qemu_mutex_lock(&monitor_lock);
>> -
>> -    if (!evstate->delay ||
>> -        !evstate->delay(evstate, data)) {
>> -        monitor_qapi_event_emit(event, QOBJECT(data));
>> -    }
>
> No change, just diff being insufficiently smart.
>
>> +static bool
>> +monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data)
>> +{
>> +    MonitorQAPIEventPending *p = evstate->data;
>>
>> -    qemu_mutex_unlock(&monitor_lock);
>> +    return monitor_qapi_event_pending_update(evstate, p, data);
>>  }
>
> Why not simply
>
>     return monitor_qapi_event_pending_update(evstate, evstate->data, data);

ok

>
>>
>>  /*
>> @@ -558,6 +540,58 @@ monitor_qapi_event_pending_new(QAPIEvent event)
>>      return p;
>>  }
>>
>> +static bool
>> +monitor_qapi_event_id_delay(MonitorQAPIEventState *evstate, QDict *data)
>> +{
>> +    GHashTable *ht = evstate->data;
>> +    QDict *s = qdict_get_qdict(data, "data");
>
> Why is this called s?
>
> Here are less confusing names:
>
> * The parameter should not be called data, because it's the complete QMP
>   event object, and *not* its data member.  Either match
>   QMPEventFuncEmit and call it qdict, or call it event.
>

It was called "data" before, I think because "event" is for QAPIEvent
in general, and the "data" of QDict is the actual event data.

So I guess qdict is a better name, although it's also very generic to me.

> * The variable s *is* the data member, and should therefore be called data.

ok

>
>> +    const char *id = qdict_get_str(s, "id");
>
> This assumes s has a member "id" of type QString.  Therefore, the
> function can only be used for events where that is always the case.
> Unobvious requirements like that should be spelled out in a function
> comment.

It spelled in the function name, but I added a comment on top.

>> +    MonitorQAPIEventPending *p = g_hash_table_lookup(ht, id);
>> +    QAPIEvent event = evstate - monitor_qapi_event_state;
>
> Another unobvious requirement: evstate must point into
> monitor_qapi_event_state.

It's an element in the array. I'll add a comment and an assert.

>
>> +
>> +    if (!p) {
>> +        p = monitor_qapi_event_pending_new(event);
>> +        g_hash_table_insert(ht, g_strdup(id), p);
>
> Note for later: both key and value are dynamically allocated.

value has a destroy handler being called when removing the value

The key should also be destroyed, let's fix that leak. thanks

>
>> +    }
>> +
>> +    return monitor_qapi_event_pending_update(evstate, p, data);
>> +}
>> +
>> +/*
>> + * 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);
>> +    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>> +
>> +    evstate = &(monitor_qapi_event_state[event]);
>> +    trace_monitor_protocol_event_queue(event,
>> +                                       data,
>> +                                       evstate->rate,
>> +                                       now);
>> +
>> +    qemu_mutex_lock(&monitor_lock);
>> +
>> +    if (!evstate->delay ||
>> +        !evstate->delay(evstate, data)) {
>> +        monitor_qapi_event_emit(event, QOBJECT(data));
>> +    }
>> +
>> +    qemu_mutex_unlock(&monitor_lock);
>> +}
>> +
>> +static void
>> +monitor_qapi_event_pending_free(MonitorQAPIEventPending *p)
>> +{
>> +    timer_del(p->timer);
>> +    timer_free(p->timer);
>> +    g_free(p);
>
> Ignorant question: who owns p->data?
>

MonitorQAPIEventPending, but the event is freed when emitted. Let's
add a cleanup here too.

>> +}
>> +
>>  /*
>>   * @event: the event ID to be limited
>>   * @rate: the rate limit in milliseconds
>> @@ -582,6 +616,24 @@ monitor_qapi_event_throttle(QAPIEvent event, int64_t rate)
>>      evstate->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]);
>
> Superfluous parenthesis.
>

was there before, copy-pasta. removing

>> +
>> +    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->data =
>> +        g_hash_table_new_full(g_str_hash, g_str_equal, NULL,
>> +            (GDestroyNotify)monitor_qapi_event_pending_free);
>
> Both key and value are dynamically allocated.  But you define only the
> value's free callback.  Why?

At first I thought the key would remain in the hashtable, it's a leftover. Fixed

>
>> +}
>> +
>>  static void monitor_qapi_event_init(void)
>>  {
>>      /* Limit guest-triggerable events to 1 per second */
>> @@ -590,7 +642,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);
>>  }
>



-- 
Marc-André Lureau

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

[Marc-André Lureau]

On Wed, Sep 16, 2015 at 6:50 PM, Markus Armbruster <armbru@redhat.com> wrote:
> I doubt you're supposed to poke into p->timer like that.
>

I don't think that matters, much (it could have a helper perhaps), anyway:

>> +    timer_mod_ns(p->timer, then);
>> +}
>> +
>> +static void monitor_qapi_event_id_handler(void *opaque)
>> +{
>> +    MonitorQAPIEventPending *p = opaque;
>> +
>> +    monitor_qapi_event_handler(p);
>> +    monitor_qapi_event_id_schedule_remove(p);
>> +}
>> +
>>  static bool
>>  monitor_qapi_event_id_delay(MonitorQAPIEventState *evstate, QDict *data)
>>  {
>> @@ -554,7 +593,13 @@ monitor_qapi_event_id_delay(MonitorQAPIEventState *evstate, QDict *data)
>>          g_hash_table_insert(ht, g_strdup(id), p);
>>      }
>>
>> -    return monitor_qapi_event_pending_update(evstate, p, data);
>> +    if (monitor_qapi_event_pending_update(evstate, p, data)) {
>> +        p->timer->cb = monitor_qapi_event_id_handler;
>> +        return true;
>> +    } else {
>> +        monitor_qapi_event_id_schedule_remove(p);
>> +        return false;
>> +    }
>>  }
>>
>>  /*
>
> Possibly cleaner than messing with timer->cb: have a single callback
> that does the right thing, i.e. when the timer goes off, check whether
> we have an event.  If yes, send it and rearm the timer.  If no, remove
> the hash entry.

ok, I'll have a single handler instead

-- 
Marc-André Lureau