From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49420) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZfW56-0004aj-04 for qemu-devel@nongnu.org; Fri, 25 Sep 2015 12:42:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZfW51-0002sc-1c for qemu-devel@nongnu.org; Fri, 25 Sep 2015 12:42:43 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60050) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZfW50-0002sQ-NH for qemu-devel@nongnu.org; Fri, 25 Sep 2015 12:42:38 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id 4D31A2F9119 for ; Fri, 25 Sep 2015 16:42:36 +0000 (UTC) References: <1443189647-11417-1-git-send-email-armbru@redhat.com> <1443189647-11417-3-git-send-email-armbru@redhat.com> From: Eric Blake Message-ID: <56057976.9020604@redhat.com> Date: Fri, 25 Sep 2015 10:42:30 -0600 MIME-Version: 1.0 In-Reply-To: <1443189647-11417-3-git-send-email-armbru@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="6Cecn6eqgAgAG1K2U4piAQu4whT2TNvlH" Subject: Re: [Qemu-devel] [RFC 2/6] monitor: Simplify event throttling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , qemu-devel@nongnu.org Cc: amit.shah@redhat.com, marcandre.lureau@redhat.com, lersek@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --6Cecn6eqgAgAG1K2U4piAQu4whT2TNvlH Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 09/25/2015 08:00 AM, Markus Armbruster wrote: > The event throttling state machine is hard to understand. I'm not > sure it's entirely correct. Rewrite it in a more straightforward > manner: >=20 > State 1: No event sent recently (less than evconf->rate ns ago) >=20 > Invariant: evstate->timer is not pending, evstate->qdict is null >=20 > On event: send event, arm timer, goto state 2 >=20 > State 2: Event sent recently, no additional event being delayed >=20 > Invariant: evstate->timer is pending, evstate->qdict is null >=20 > On event: store it in evstate->qdict, goto state 3 >=20 > On timer: goto state 1 >=20 > State 3: Event sent recently, additional event being delayed >=20 > Invariant: evstate->timer is pending, evstate->qdict is non-null >=20 > On event: store it in evstate->qdict, goto state 3 >=20 > On timer: send evstate->qdict, clear evstate->qdict, > arm timer, goto state 2 >=20 Makes sense for what throttling is supposed to be. > FIXME update trace-event (and recompile the world) Obviously something you'd fold into a v2, so I'll defer R-b until seeing it. But I like the approach of this patch. >=20 > Signed-off-by: Markus Armbruster > --- > monitor.c | 70 +++++++++++++++++++++++++++++++------------------------= -------- > 1 file changed, 35 insertions(+), 35 deletions(-) >=20 > if (!evstate->rate) { > + /* Unthrottled event */ > monitor_qapi_event_emit(event, qdict); > - evstate->last =3D now; > } else { > - int64_t delta =3D now - evstate->last; > - if (evstate->qdict || > - delta < evstate->rate) { > - /* If there's an existing event pending, replace > - * it with the new event, otherwise schedule a > - * timer for delayed emission > + if (timer_pending(evstate->timer)) { Possible in states 2 and 3... > + /* > + * Timer is pending for (at least) evstate->rate ns after > + * last send. Store event for sending when timer fires, > + * replacing a prior stored event if any. > */ > - if (evstate->qdict) { > - QDECREF(evstate->qdict); > - } else { > - int64_t then =3D evstate->last + evstate->rate; > - timer_mod_ns(evstate->timer, then); > - } > + QDECREF(evstate->qdict); no-op in state 2, otherwise replaces the old pending event in state 3 > evstate->qdict =3D qdict; > QINCREF(evstate->qdict); either way, we are now in state 3 awaiting the next event or timer. > } else { we are in state 1... > + /* > + * Last send was (at least) evstate->rate ns ago. > + * Send immediately, and arm the timer to call > + * monitor_qapi_event_handler() in evstate->rate ns. Any > + * events arriving before then will be delayed until then.= > + */ > + int64_t now =3D qemu_clock_get_ns(QEMU_CLOCK_REALTIME); > + > monitor_qapi_event_emit(event, qdict); > - evstate->last =3D now; > + timer_mod_ns(evstate->timer, now + evstate->rate); and now in state 2. > } > } > + > qemu_mutex_unlock(&monitor_lock); > } > =20 > /* > - * The callback invoked by QemuTimer when a delayed > - * event is ready to be emitted > + * This function runs evstate->rate ns after sending a throttled > + * event. > + * If another event has since been stored, send it. > */ > static void monitor_qapi_event_handler(void *opaque) > { We get here in either state 2 or 3... > MonitorQAPIEventState *evstate =3D opaque; > - int64_t now =3D qemu_clock_get_ns(QEMU_CLOCK_REALTIME); > =20 > trace_monitor_protocol_event_handler(evstate->event, > evstate->qdict, > - evstate->last, > - now); > + 0, 0); /* FIXME drop */ > qemu_mutex_lock(&monitor_lock); > + > if (evstate->qdict) { state 3, so send it... > + int64_t now =3D qemu_clock_get_ns(QEMU_CLOCK_REALTIME); > + > monitor_qapi_event_emit(evstate->event, evstate->qdict); > QDECREF(evstate->qdict); > evstate->qdict =3D NULL; > + timer_mod_ns(evstate->timer, now + evstate->rate); =2E..and rearm to go back to state 2 > } > - evstate->last =3D now; > + =2E..otherwise, we didn't rearm, so we go back to state 1 > qemu_mutex_unlock(&monitor_lock); > } > =20 Matches the state logic called out in the commit message. > @@ -539,20 +542,17 @@ static void > monitor_qapi_event_throttle(QAPIEvent event, int64_t rate) > { > MonitorQAPIEventState *evstate; > + > assert(event < QAPI_EVENT_MAX); > - > - evstate =3D &(monitor_qapi_event_state[event]); > - > + evstate =3D &monitor_qapi_event_state[event]; > 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->qdict =3D NULL; > - evstate->timer =3D timer_new(QEMU_CLOCK_REALTIME, > - SCALE_MS, > - monitor_qapi_event_handler, > - evstate); > + evstate->timer =3D timer_new_ns(QEMU_CLOCK_REALTIME, > + monitor_qapi_event_handler, > + evstate); Is this a separate cleanup? It looks like you're fixing code style, and then switching from timer_new() to timer_new_ns(), but it's not obvious from just this context whether you still have the right scale (and I didn't go chase documentation). It wasn't mentioned in the commit message, and seems unrelated to the new state machine. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --6Cecn6eqgAgAG1K2U4piAQu4whT2TNvlH Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWBXl2AAoJEKeha0olJ0NqvjMH/R/BxkS9Vm2wEw1dly2jhByz VeES1eKAQaq41OnrxJxPduO25vK6R6bGhHrFuQ90c7HoHRV+NUF0/j12K6CmA8TK H6FvejHQVQYdbSkj2WGlQDfZJN6hgQ1GAWnJDNvTjzhgCp8XAD7qX3ikM5zqQPgJ ogHF1bRUmL9KlSnWD+jrNjPUhebuvUOXqiAan4bfpHJPVHrvo8Ckk1kprI45cDVD 6KrJUc2K/z55LI/5EnCmJbsllhvIiijIo7hAg3d5H8a/U5I0ecenvoF7vW7H93Kt NvP32caWr4envQe03qbJg1QJ5nCjd30/3cUknIvN4QCmKyRKjW3JfgVWrvMyae8= =8X7b -----END PGP SIGNATURE----- --6Cecn6eqgAgAG1K2U4piAQu4whT2TNvlH--