[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/3] monitor: split MonitorQAPIEventState
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 1/3] monitor: split MonitorQAPIEventState |
Date: |
Wed, 16 Sep 2015 15:57:51 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
address@hidden writes:
> From: Marc-André Lureau <address@hidden>
>
> 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 <address@hidden>
> ---
> 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