From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:54165) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RoDEo-0004tS-BF for qemu-devel@nongnu.org; Fri, 20 Jan 2012 07:06:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RoDEm-0000o0-D4 for qemu-devel@nongnu.org; Fri, 20 Jan 2012 07:06:34 -0500 Received: from mx1.redhat.com ([209.132.183.28]:39802) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RoDEm-0000ns-2O for qemu-devel@nongnu.org; Fri, 20 Jan 2012 07:06:32 -0500 Date: Fri, 20 Jan 2012 10:06:24 -0200 From: Luiz Capitulino Message-ID: <20120120100624.7195e19a@doriath> In-Reply-To: <20120119215006.GC6799@us.ibm.com> References: <1326988591-27241-1-git-send-email-lcapitulino@redhat.com> <20120119215006.GC6799@us.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC 0/5]: QMP: add balloon-get-memory-stats command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Adam Litke Cc: aliguori@us.ibm.com, mdroth@linux.vnet.ibm.com, armbru@redhat.com, qemu-devel@nongnu.org, amit.shah@redhat.com, eblake@redhat.com On Thu, 19 Jan 2012 15:50:06 -0600 Adam Litke wrote: > On Thu, Jan 19, 2012 at 01:56:26PM -0200, Luiz Capitulino wrote: > > Long ago, commit 625a5be added the guest provided memory statistics to > > the query-balloon command. Unfortunately, it also introduced a severe > > bug: query-balloon would hang if the guest didn't respond. This, in turn, > > would also cause a hang in libvirt. > > > > Because of that, we decided to disable the guest memory stats feature > > (commit 11724ff). > > > > As we decided to let commands implement ad-hoc async mechanisms until we > > get a proper way to do it, I decided to try to re-enable that feature. > > > > My idea is to have a command and an event. The command gets the process > > started by sending a request to guest and returns. Later, when the guest > > makes the memory stats info available, it's sent to the client by means > > of an QMP event (please, take a look at patch 05/05 for full details). > > > > I'm not sure if that approach is good for libvirt though, so it would be > > very helpful to get their input (Eric, I'm CC'ing you here, but feel free > > to route this to someone else). > > > > Another interesting point is that, there's another way of doing this and > > it's using qemu-ga instead. That's, qemu-ga could read that information > > from proc and return it. This is easier & simpler, as it doesn't involve > > guest communication. We also could return a lot more information if needed. > > The only disadvantage I can see is the dependency on qemu-ga... > > > > QMP/qmp-events.txt | 28 ++++++++++++++++++++++++++++ > > balloon.c | 47 +++++++++++++++++++++++++++++++++++++---------- > > balloon.h | 7 ++++--- > > hmp.c | 25 +------------------------ > > hw/virtio-balloon.c | 39 +++++++++++++++++++++++++++------------ > > monitor.c | 3 +++ > > monitor.h | 1 + > > qapi-schema.json | 42 ++++++++++++++++++++++-------------------- > > qmp-commands.hx | 6 ++++++ > > 9 files changed, 129 insertions(+), 69 deletions(-) > > > > The patch series looks good. Thanks for making the improvements. Once it is > upstream I can help out with the libvirt pieces. I tested it with a Fedora-15 > VM and it worked out of the box :) I just have one small nit. Due to the way > that the virtio stats queue works, the guest emits a stats event with bogus > values when the balloon driver initializes (which gives the host control of the > channel). Your patches emit this initial event (which contains undefined > values). In my old code, we kept a boolean flag in the ballon device to record > if stats have been requested and only if that was set would we raise the event. > Without this, the guest can spam the host with an unlimited number of bogus > events. Yeah, I've seen this too. I'll fix it. We also need migration and HMP support. Although the latter seems difficult as we don't have a way for qemu subsystems to wait for events yet. > Tested-by: Adam Litke Thanks! >