diff mbox

[RFC,4/6] monitor: Turn monitor_qapi_event_state[] into a hash table

Message ID 1443189647-11417-5-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

Markus Armbruster Sept. 25, 2015, 2 p.m. UTC 
In preparation of finer grained throttling.

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

Comments

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

> @@ -512,6 +518,14 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp)
>              int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>  
>              monitor_qapi_event_emit(event, qdict);
> +
> +            evstate = g_new(MonitorQAPIEventState, 1);
> +            evstate->event = event;
> +            evstate->qdict = NULL;
> +            evstate->timer = timer_new_ns(QEMU_CLOCK_REALTIME,
> +                                          monitor_qapi_event_handler,
> +                                          evstate);

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

The conversion looks sane:
Reviewed-by: Eric Blake <eblake@redhat.com>
Markus Armbruster Sept. 28, 2015, 8:32 a.m. UTC | #2 
Eric Blake <eblake@redhat.com> writes:

> On 09/25/2015 08:00 AM, Markus Armbruster wrote:
>> In preparation of finer grained throttling.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  monitor.c | 55 ++++++++++++++++++++++++++++++++++++++-----------------
>>  1 file changed, 38 insertions(+), 17 deletions(-)
>> 
>
>> @@ -512,6 +518,14 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *qdict, Error **errp)
>>              int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>>  
>>              monitor_qapi_event_emit(event, qdict);
>> +
>> +            evstate = g_new(MonitorQAPIEventState, 1);
>> +            evstate->event = event;
>> +            evstate->qdict = NULL;
>> +            evstate->timer = timer_new_ns(QEMU_CLOCK_REALTIME,
>> +                                          monitor_qapi_event_handler,
>> +                                          evstate);
>
> Now timers are created and destroyed dynamically upon use rather than
> reused and just waiting to be rearmed; I hope there aren't any obvious
> inefficiencies from doing that.

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

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

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

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

Thanks!
diff mbox

Patch

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