From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37681) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZPEAx-0005oL-RD for qemu-devel@nongnu.org; Tue, 11 Aug 2015 14:21:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZPEAs-0002yd-NI for qemu-devel@nongnu.org; Tue, 11 Aug 2015 14:21:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56488) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZPEAs-0002yS-HN for qemu-devel@nongnu.org; Tue, 11 Aug 2015 14:21:22 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 824C9157742 for ; Tue, 11 Aug 2015 18:21:21 +0000 (UTC) References: <1439312647-22245-1-git-send-email-marcandre.lureau@redhat.com> From: Laszlo Ersek Message-ID: <55CA3D1E.80705@redhat.com> Date: Tue, 11 Aug 2015 20:21:18 +0200 MIME-Version: 1.0 In-Reply-To: <1439312647-22245-1-git-send-email-marcandre.lureau@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] monitor: remove QAPI_EVENT_VSERPORT_CHANGE throttle List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: marcandre.lureau@redhat.com, qemu-devel@nongnu.org Cc: amit.shah@redhat.com On 08/11/15 19:04, marcandre.lureau@redhat.com wrote: > From: Marc-Andr=C3=A9 Lureau >=20 > QAPI_EVENT_VSERPORT_CHANGE reports changes of a virtio serial port > state. However, the events may be for different ports, but the throttle > mechanism may replace the event for a different port, since it only > checks the event type. >=20 > libvirt relies on a correct state to be reported for all channels: the > qemu-ga commands may no longer work if the state is reported > disconnected. This can be triggered easily by having more than 1 > virtio-serial (qemu-ga + spice agent for example), and restarting > quickly daemons or more realistically going quickly in and out of > suspend. >=20 > In a future patch, we may want to throttle events based on their > arguments, but this will likely require dynamic allocations and more > complicated code to insert/lookup pending events based on various > arguments ("id" in QAPI_EVENT_VSERPORT_CHANGE case). >=20 > Fixes: > https://bugzilla.redhat.com/show_bug.cgi?id=3D1244064 >=20 > Signed-off-by: Marc-Andr=C3=A9 Lureau > --- > monitor.c | 1 - > 1 file changed, 1 deletion(-) >=20 > diff --git a/monitor.c b/monitor.c > index aeea2b5..e4d56f7 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -558,7 +558,6 @@ static void monitor_qapi_event_init(void) > monitor_qapi_event_throttle(QAPI_EVENT_BALLOON_CHANGE, 1000); > monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_REPORT_BAD, 1000); > monitor_qapi_event_throttle(QAPI_EVENT_QUORUM_FAILURE, 1000); > - monitor_qapi_event_throttle(QAPI_EVENT_VSERPORT_CHANGE, 1000); > =20 > qmp_event_set_func_emit(monitor_qapi_event_queue); > } >=20 I don't mind the change (and the point of argument sensitivity is not lost on me), but note that this undoes the protection that is spelled out in the leading comment of the function, not visible in the context he= re: /* Limit guest-triggerable events to 1 per second */ That was probably put in place in order to prevent a "malicious" guest from spamming the log files (and the CPU usage) of the management apps. One solution to that would be arg sensitivity. Another would be a burst-capable (ie. a slowly re-filling, limited size token bucket) throttle, maintained per event type. Until one of those gets written, I guess this patch is acceptable -- as long as mgmt people are okay with it. Daniel seems to be, so I don't mind= . Thanks Laszlo