Re: [Qemu-devel] [PATCH 0/3] re-enable balloon stats

[mdroth <mdroth@linux.vnet.ibm.com>]

On Tue, Dec 11, 2012 at 04:28:40PM -0200, Luiz Capitulino wrote:
> On Tue, 11 Dec 2012 11:38:08 -0600
> mdroth <mdroth@linux.vnet.ibm.com> wrote:
> 
> > On Tue, Dec 11, 2012 at 03:14:02PM +0000, Dietmar Maurer wrote:
> > > > > > I'm not sure I like this for two reasons. First, there will be cases
> > > > > > where the user doesn't want this to be enabled. Second, we'll be
> > > > > > forcing an interval on users.
> > > > >
> > > > > So when should we set the stats-polling-interval? I first thought I
> > > > > simply set that at VM start, but that triggers an error:
> > > > >
> > > > > "guest doesn't support balloon stats"
> > > > >
> > > > > because the balloon driver is not loaded.
> > > > 
> > > > Yes, it's required to have the balloon driver loaded. The stats are reported by
> > > > it.
> > > 
> > > That does not really answers my question. So you think the management
> > > framework should start the VM, and then wait until the balloon driver is
> > > loaded? That sound very clumsy to me.
> > > 
> > > I mean, I just want to set the polling interval. Why does that need the balloon drive loaded 
> > > inside the guest (polling is done by the host)?
> > 
> > I agree. Should lack of a balloon module disable the timer
> > completely, or just silently fail? Management can always reference
> > stats-last-update to check that stats are being reported properly. -1 would
> > mean driver was never loaded, longer update intervals might mean guest
> > was rebooted/suspended/etc, but in both cases it makes sense that the
> > timer still try to fetch stats, and the determination of what stats are
> > valid/invalid be left to management.
> 
> I could move the check for the stats feature bit from the function that
> enables polling to the timer callback. This would solve Dietmar's use-case,
> but it would silently fail if the feature is never negotiated (as you said
> above).
> 
> > Alternatively, we can hook into the feature negotiation path and
> > defer the actual timer start until the required feature is negotiated,
> 
> That's also possible, and helps not wasting resources.
> 
> > and disable it (temporarilly or permanently based on guest behavior)
> > somewhere in the reset path). I'm not sure this will keep it from
> > running during hibernate/sleep...maybe hook into runstate changes?
> 
> Then it starts to get somewhat complicated.
> 

Agreed...that's only if we want to go to extremes to avoid running the
timer unecessarily, but probably not worth it.

I like your idea of just having the timer callback fail silently if the
feature isn't enabled.

And personally, I think just having a stale value for stats-last-update
is sufficient for error checking, but we can squirrel away an error in
a stats-last-error field if we want something more descriptive for
clients, and just wipe it out on the next successful update. Not sure
whether we should clear the stats upon error or not, I think it might
actually be useful to leave them. If the guest crashed because we
ballooned away too much memory or something for instance we'd be able
to see the last value we logged.