From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60065) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZPcKP-0000hK-Kv for qemu-devel@nongnu.org; Wed, 12 Aug 2015 16:08:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZPcKJ-0007xz-9J for qemu-devel@nongnu.org; Wed, 12 Aug 2015 16:08:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43536) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZPcKJ-0007xV-0C for qemu-devel@nongnu.org; Wed, 12 Aug 2015 16:08:43 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 3C1B53FE82 for ; Wed, 12 Aug 2015 20:00:07 +0000 (UTC) References: <1439408763-12785-1-git-send-email-marcandre.lureau@redhat.com> <1439408763-12785-2-git-send-email-marcandre.lureau@redhat.com> From: Laszlo Ersek Message-ID: <55CBA5C1.1010401@redhat.com> Date: Wed, 12 Aug 2015 22:00:01 +0200 MIME-Version: 1.0 In-Reply-To: <1439408763-12785-2-git-send-email-marcandre.lureau@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [RFC 1/3] monitor: split MonitorQAPIEventState List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: marcandre.lureau@redhat.com, qemu-devel@nongnu.org On 08/12/15 21:46, marcandre.lureau@redhat.com wrote: > From: Marc-Andr=C3=A9 Lureau >=20 > Create a seperate pending event structure MonitorQAPIEventPending. > Use a MonitorQAPIEventDelay callback to handle the delaying. This > allows other implementations of throttling. >=20 > Signed-off-by: Marc-Andr=C3=A9 Lureau > --- > 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; > =20 > +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; > +}; > =20 > struct Monitor { > CharDriverState *chr; > @@ -452,6 +461,39 @@ static void monitor_qapi_event_emit(QAPIEvent even= t, QObject *data) > } > } > =20 > +static bool > +monitor_qapi_event_delay(MonitorQAPIEventState *evstate, QDict *data) > +{ > + int64_t now =3D qemu_clock_get_ns(QEMU_CLOCK_REALTIME); > + MonitorQAPIEventPending *p =3D evstate->data; > + int64_t delta =3D now - p->last; > + > + /* Rate limit of 0 indicates no throttling */ > + if (!evstate->rate) { > + p->last =3D 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 =3D p->last + evstate->rate; > + timer_mod_ns(p->timer, then); > + } > + p->data =3D QOBJECT(data); > + qobject_incref(p->data); > + return TRUE; > + } > + > + p->last =3D 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); > =20 > - /* 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 =3D now; > - } else { > - int64_t delta =3D 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 =3D evstate->last + evstate->rate; > - timer_mod_ns(evstate->timer, then); > - } > - evstate->data =3D QOBJECT(data); > - qobject_incref(evstate->data); > - } else { > - monitor_qapi_event_emit(event, QOBJECT(data)); > - evstate->last =3D now; > - } > } > + > qemu_mutex_unlock(&monitor_lock); > } > =20 > @@ -505,23 +527,37 @@ monitor_qapi_event_queue(QAPIEvent event, QDict *= data, Error **errp) > */ > static void monitor_qapi_event_handler(void *opaque) > { > - MonitorQAPIEventState *evstate =3D opaque; > + MonitorQAPIEventPending *p =3D opaque; > int64_t now =3D qemu_clock_get_ns(QEMU_CLOCK_REALTIME); > =20 > - 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 =3D NULL; > + if (p->data) { > + monitor_qapi_event_emit(p->event, p->data); > + qobject_decref(p->data); > + p->data =3D NULL; > } > - evstate->last =3D now; > + p->last =3D now; > qemu_mutex_unlock(&monitor_lock); > } > =20 > +static MonitorQAPIEventPending * > +monitor_qapi_event_pending_new(QAPIEvent event) > +{ > + MonitorQAPIEventPending *p; > + > + p =3D g_new0(MonitorQAPIEventPending, 1); > + p->event =3D event; > + p->timer =3D 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, int6= 4_t rate) > evstate =3D &(monitor_qapi_event_state[event]); > =20 > trace_monitor_protocol_event_throttle(event, rate); > - evstate->event =3D event; > assert(rate * SCALE_MS <=3D INT64_MAX); > evstate->rate =3D rate * SCALE_MS; > - evstate->last =3D 0; > - evstate->data =3D NULL; > - evstate->timer =3D timer_new(QEMU_CLOCK_REALTIME, > - SCALE_MS, > - monitor_qapi_event_handler, > - evstate); > + > + evstate->delay =3D monitor_qapi_event_delay; > + evstate->data =3D monitor_qapi_event_pending_new(event); > } > =20 > 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_nam= e) "mon %p cmd_name \"%s\"" > monitor_protocol_emitter(void *mon) "mon %p" > monitor_protocol_event_handler(uint32_t event, void *data, uint64_t la= st, uint64_t now) "event=3D%d data=3D%p last=3D%" PRId64 " now=3D%" PRId6= 4 > monitor_protocol_event_emit(uint32_t event, void *data) "event=3D%d da= ta=3D%p" > -monitor_protocol_event_queue(uint32_t event, void *data, uint64_t rate= , uint64_t last, uint64_t now) "event=3D%d data=3D%p rate=3D%" PRId64 " l= ast=3D%" PRId64 " now=3D%" PRId64 > +monitor_protocol_event_queue(uint32_t event, void *data, uint64_t rate= , uint64_t now) "event=3D%d data=3D%p rate=3D%" PRId64 " now=3D%" PRId64 > monitor_protocol_event_throttle(uint32_t event, uint64_t rate) "event=3D= %d rate=3D%" PRId64 > =20 > # hw/net/opencores_eth.c >=20