diff mbox

[1/3] monitor: split MonitorQAPIEventState

Message ID 1441192183-4812-2-git-send-email-marcandre.lureau@redhat.com
State New
Headers show

Commit Message

Marc-André Lureau Sept. 2, 2015, 11:09 a.m. UTC
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(-)

Comments

Daniel P. Berrangé Sept. 10, 2015, 8:34 a.m. UTC | #1
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
Markus Armbruster Sept. 16, 2015, 1:57 p.m. UTC | #2
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
Marc-André Lureau Sept. 17, 2015, 1:17 p.m. UTC | #3
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
>
diff mbox

Patch

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