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

[Michael Roth]

On Wed, Feb 22, 2012 at 02:09:35PM -0200, Luiz Capitulino wrote:
> 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).

But I'm not suggesting we make query-balloon asynchronous.

I'm suggesting be re-enable it as a synchronous interface by having it
immediately return the latest-available results from a timer-driven
query mechanism that tucks away the query results, such as the one Anthony
suggested.

In the future we can drop the polling backend and switch the backend over to
using proper async QMP that queries on demand. The change to current users
would be transparent (and it would also expose an *optional* async
front-end that would also handle the "send me an event when it's ready"
use-case, so balloon-get-memory-stats would never be needed).

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

Along with timer-based refresh of the properties? If so I don't understand why 
we
can't just have query-balloon simply return those properties when it's called? I
thought that's what Anthony was driving at with the timer-based
approach.

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