| Message ID | 1439408763-12785-2-git-send-email-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/12/15 21:46, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > Create a seperate 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(-) Assume there has been a long period of silence (no attempts to emit an event). Now some client code makes a call to emit the event. Will that event be emitted immediately, or will it be delayed to see if more are coming? I'd like to understand this aspect first. I think the first instance of the event, after the grace period, should be emitted immediately, and further instances that quickly follow should be suppressed. It would also be possible to delay (and update) a "current instance" in-place until the calls quiesce, and emit the (most recent) update then. (I'm getting the impression that the patch does this, but I could be wrong.) I think emitting the first immediately (and suppressing followers) is better; that's eg. what the kernel log does. Nagle's algorithm (TCP_CORK <-> TCP_NODELAY) is a (strained) example for the other behavior (ie. coalesce leaders). I think that approach would not be a natural fit for QEMU's events. Of course, if your code already implements what I think would be more applicable, then I shouldn't have spoken! :) Can you clarify it though please? Thanks! Laszlo > diff --git a/monitor.c b/monitor.c > index aeea2b5..9c51ffa 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 MonitorQAPIEventPending { > + 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; > + gpointer 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 94bf3bb..e39ff33 100644 > --- a/trace-events > +++ b/trace-events > @@ -1027,7 +1027,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 >
Hi ----- Original Message ----- > On 08/12/15 21:46, marcandre.lureau@redhat.com wrote: > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > Create a seperate 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(-) > > Assume there has been a long period of silence (no attempts to emit an > event). Now some client code makes a call to emit the event. > > Will that event be emitted immediately, or will it be delayed to see if > more are coming? I'd like to understand this aspect first. > > I think the first instance of the event, after the grace period, should > be emitted immediately, and further instances that quickly follow should > be suppressed. This is what qemu already does. The first event is sent immediately, the later ones may be delayed (but there will be at least one event every period, if the event is flooded). This patch 1/3 doesn't change the logic, only it split things to make them a bit more modular. So the rest of the patches do not change the qemu delay logic, it adds a way to delay based on (event + "id") instead of just (event). It does that by adding an additional "id" hashtable for the event type. My hope is that this apporach could be reused if other field or combinations of fields are necessary for other events, but for now, it's hardcoded for "id". cheers
On 08/12/2015 02:00 PM, Laszlo Ersek wrote: > Assume there has been a long period of silence (no attempts to emit an > event). Now some client code makes a call to emit the event. > > Will that event be emitted immediately, or will it be delayed to see if > more are coming? I'd like to understand this aspect first. > > I think the first instance of the event, after the grace period, should > be emitted immediately, and further instances that quickly follow should > be suppressed. That has always been the goal of event throttling: when a new event arrives and the timer is not running, emit it right away and start the timer. If another event arrives while the timer has not expired, save it. If multiple events arrive during the timer, the last one received overwrites any others. Then, when the timer expires, either there is a saved event (one or more events arrived during the throttling window), so we send it and restart another throttling window, or there was no event pending so the line is quiet and the next event can send right away. Thus, management will get notification as soon as possible that events are starting to happen after a period of quiet, but will then not be spammed any faster than once per throttle period (once per second) with additional events. When additional events are sent, they are always the most accurate state of the system at the time the event was queued, but intermediate events may have been lost. Furthermore, unless the events have not been happening, the throttling of the event means the management might not see the event until nearly a second late, although the timestamp associated with the event should still be accurate to the time it was queued up, not finally sent.
On 08/12/2015 01:46 PM, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > Create a seperate pending event structure MonitorQAPIEventPending. s/seperate/separate/ > 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(-) > > > +typedef struct MonitorQAPIEventPending { > + 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; Some places combine the typedef with the struct definition; I'm not sure there's any hard and fast rule, though. (HACKING mentions that we want the typedef, but doesn't give guidelines on how it must be provided). > +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; > + gpointer data; Do we really need 'gpointer', or is 'void *' sufficient? > > +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; s/FALSE/false/ (we want to directly use the C99 'bool' type here, not the glib macros that expand to who-knows-what-type followed by implicit conversion back to bool). > + p->data = QOBJECT(data); > + qobject_incref(p->data); > + return TRUE; > + } > + > + p->last = now; > + return FALSE; two more ALL_CAPS to convert to the lower bool counterpart. Otherwise looks like a sane split.
diff --git a/monitor.c b/monitor.c index aeea2b5..9c51ffa 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 MonitorQAPIEventPending { + 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; + gpointer 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 94bf3bb..e39ff33 100644 --- a/trace-events +++ b/trace-events @@ -1027,7 +1027,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