Re: [Qemu-devel] [PATCH 6/6] qmp: add balloon-get-memory-stats & event

[Luiz Capitulino]

On Wed, 22 Feb 2012 09:05:22 -0600
Michael Roth <address@hidden> wrote:

> On Wed, Feb 22, 2012 at 10:48:13AM -0200, Luiz Capitulino wrote:
> > On Fri, 17 Feb 2012 15:51:33 -0600
> > Michael Roth <address@hidden> 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 <address@hidden> 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<address@hidden>
> > > > > 
> > > > > 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.

It's not possible to re-use query-balloon for this, making a synchronous
command asynchronous is an incompatible change. That was exactly the problem
we had that forced us to disable this feature (and that's why a new command
is required, be it balloon-get-memory-stats or a QOM device property).

> 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.

Any command implementing its async schema is going to be deprecated when we
get proper async support. That's not a problem per se. I mean, a much worse
problem is to delay features & functionality because we don't have the perfect
means to implement them.

But that's a dead discussion I guess, as I already agreed on implementing
this as a device property.

> 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.
>