From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:46142) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S0DlE-000317-6l for qemu-devel@nongnu.org; Wed, 22 Feb 2012 10:05:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1S0Dl4-0008S2-1F for qemu-devel@nongnu.org; Wed, 22 Feb 2012 10:05:40 -0500 Received: from mail-pz0-f45.google.com ([209.85.210.45]:34368) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1S0Dl3-0008Rv-Ka for qemu-devel@nongnu.org; Wed, 22 Feb 2012 10:05:29 -0500 Received: by dadp14 with SMTP id p14so138918dad.4 for ; Wed, 22 Feb 2012 07:05:28 -0800 (PST) Sender: fluxion Date: Wed, 22 Feb 2012 09:05:22 -0600 From: Michael Roth Message-ID: <20120222150522.GA2824@illuin> References: <1328733040-16697-1-git-send-email-lcapitulino@redhat.com> <1328733040-16697-7-git-send-email-lcapitulino@redhat.com> <4F3E868D.8020107@us.ibm.com> <20120217151622.682d307b@doriath.home> <20120217215133.GE20920@illuin> <20120222104813.650f7779@doriath.home> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20120222104813.650f7779@doriath.home> Subject: Re: [Qemu-devel] [PATCH 6/6] qmp: add balloon-get-memory-stats & event List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: armbru@redhat.com, Anthony Liguori , eblake@redhat.com, qemu-devel@nongnu.org, agl@us.ibm.com On Wed, Feb 22, 2012 at 10:48:13AM -0200, Luiz Capitulino wrote: > On Fri, 17 Feb 2012 15:51:33 -0600 > Michael Roth wrote: > > > On Fri, Feb 17, 2012 at 03:16:22PM -0200, Luiz Capitulino wrote: > > > On Fri, 17 Feb 2012 10:55:41 -0600 > > > Anthony Liguori wrote: > > > > > > > On 02/08/2012 02:30 PM, Luiz Capitulino wrote: > > > > > This commit adds a QMP API for the guest provided memory statistics > > > > > (long disabled by commit 07b0403dfc2b2ac179ae5b48105096cc2d03375a). > > > > > > > > > > The approach taken by the original commit > > > > > (625a5befc2e3200b396594f002218d235e375da5) was to extend the > > > > > query-balloon command. It introduced a severe bug though: query-balloon > > > > > would hang if the guest didn't respond. > > > > > > > > > > The approach taken by this commit is asynchronous and thus avoids > > > > > any QMP hangs. > > > > > > > > > > First, a client has to issue the balloon-get-memory-stats command. > > > > > That command gets the process started by only sending a request to > > > > > the guest, it doesn't block. When the memory stats are made available > > > > > by the guest, they are returned to the client as an QMP event. > > > > > > > > > > Signed-off-by: Luiz Capitulino > > > > > > > > Do we need this to be stable in 1.1? > > > > > > Well, this is disabled for a long time already and libvirt needs it, so I'd > > > say asap, but isn't it possible to implement this with current QOM? > > > > > > > We can do this pretty nicely through QOM. We can have a polling property in the > > > > virtio-balloon driver, that when set, will enable the virtio-balloon device to > > > > poll the guest for statistics. > > > > > > > > > > > > We can also have properties for each of the memory statistics and a timestamp > > > > for when the last update was. > > > > > > > > I think this is a friendlier approach for clients, and a cleaner approach from a > > > > QEMU perspective. > > > > > > I agree it's friendlier, but is it a good idea to keep polling the guest for > > > something that may never be needed by a mngt app (real question)? > > > > Probably not, but then again you'd only need like 1-second granularity. > > I've talked with Anthony by irc about the implementation details of this and > it will be possible to enable/disable the polling, so this is not an issue > anymore. > > > Also, I think we can do away with the polling once async QMP is in > > place, so we wouldn't be stuck with it necessarilly. > > This is what this series does :) I don't think it's necessary to wait for > async support, we're accepting ad-hoc async mechanisms for other commands and > could use one here too if needed. I don't mind the ad-hoc implementation details, I just think in this case it's leaking out into our APIs. With proper async QMP in place we just re-enable query-balloon and existing synchronous QMP clients and libvirt interfaces Just Work (albeit with potential for a "timed-out" response). There's no need for a specialized balloon-get-memory-stats command at all. And if they want stats asynchronously, proper async QMP does that as well: they just need to make their QMP client async-aware (basically, don't wait around for a response, and tag the query-balloon request so you can match the response to the query). So balloon-get-memory-stats is already on the path to being deprecated. We should instead focus on just re-enabling query-balloon, and if that can be achieved with this approach, then I'm all for it, but I think we all seem to agree that a timer-based mechanism would be needed instead. > > > > We could allow the mngt app to do the polling by adding a query-balloon-stats > > > command (instead of balloon-get-memory-stats & event). This command could > > > return the latest available stats if any (with a timestamp) and query the > > > guest for new stats. > > > > The downside there is you could read some really stale data that way, > > to the point where any app that really cared would likely throw out the first > > result. > > Having stale data will be possible with any timer based polling... > Sorry, thought you were suggesting we do "lazy" polling where we only queue up a balloon request after a query-balloon has been issued, in which case the age of the response is unbounded... well, from a QMP standpoint, I guess that *is* what we'd be doing, and we'd just be telling management tools to add a wrapper around the interface as a work-around, which seems suggests it's not the right interface.