From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54020) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZPl3w-0004IA-IS for qemu-devel@nongnu.org; Thu, 13 Aug 2015 01:28:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZPl3r-000407-He for qemu-devel@nongnu.org; Thu, 13 Aug 2015 01:28:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50069) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZPl3r-000400-Ca for qemu-devel@nongnu.org; Thu, 13 Aug 2015 01:28:19 -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 C57994C65E for ; Thu, 13 Aug 2015 05:28:17 +0000 (UTC) Date: Thu, 13 Aug 2015 10:58:15 +0530 From: Amit Shah Message-ID: <20150813052815.GA23342@grmbl.mre> References: <1439312647-22245-1-git-send-email-marcandre.lureau@redhat.com> <55CA3D1E.80705@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <55CA3D1E.80705@redhat.com> 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: Laszlo Ersek Cc: marcandre.lureau@redhat.com, qemu-devel@nongnu.org On (Tue) 11 Aug 2015 [20:21:18], Laszlo Ersek wrote: > On 08/11/15 19:04, marcandre.lureau@redhat.com wrote: > > From: Marc-Andr=E9 Lureau > >=20 > > QAPI_EVENT_VSERPORT_CHANGE reports changes of a virtio serial port > > state. However, the events may be for different ports, but the thrott= le > > 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: th= e > > 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=E9 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 >=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 = here: >=20 > /* Limit guest-triggerable events to 1 per second */ >=20 > 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. >=20 > 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. >=20 > 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 mi= nd. OK - so I'll queue it. Thanks, Amit