Re: [Qemu-devel] [PATCH 1/2] virtio-balloon: export all balloon statistics

[Roman Kagan]

On Thu, Feb 25, 2016 at 10:54:17AM +0200, Michael S. Tsirkin wrote:
> On Thu, Feb 25, 2016 at 09:44:06AM +0100, Markus Armbruster wrote:
> > "Denis V. Lunev" <address@hidden> writes:
> > 
> > > On 02/24/2016 06:43 PM, Eric Blake wrote:
> > >> On 02/24/2016 07:31 AM, Michael S. Tsirkin wrote:
> > >>> Roman Kagan <address@hidden> writes:
> > >>>> On Tue, Feb 23, 2016 at 05:49:21PM +0200, Michael S. Tsirkin wrote:
> > >>>>> On Tue, Feb 23, 2016 at 06:29:33PM +0300, Denis V. Lunev wrote:
> > >>>>> > On 02/23/2016 06:24 PM, Michael S. Tsirkin wrote:
> > >>>>> > >On Tue, Feb 23, 2016 at 05:59:44PM +0300, Denis V. Lunev wrote:
> > >>>>> > >>From: Igor Redko <address@hidden>
> > >>>>> > >>
> > >>>>> > >>We are making experiments with different autoballooning strategies
> > >>>>> > >>based on the guest behavior. Thus we need to experiment with 
> > >>>>> > >>different
> > >>>>> > >>guest statistics. For now every counter change requires QEMU 
> > >>>>> > >>recompilation
> > >>>>> > >>and dances with Libvirt.
> > >>>>> > >>
> > >>>>> > >>This patch introduces transport for unrecognized counters in 
> > >>>>> > >>virtio-balloon.
> > >>>>> > >>This transport can be used for measuring benefits from using new
> > >>>>> > >>balloon counters, before submitting any patches. Current 
> > >>>>> > >>alternative
> > >>>>> > >>is 'guest-exec' transport which isn't made for such delicate 
> > >>>>> > >>matters
> > >>>>> > >>and can influence test results.
> > >>>>> > >>
> > >>>>> > >>Originally all counters with tag >= VIRTIO_BALLOON_S_NR were 
> > >>>>> > >>ignored.
> > >>>>> > >>Instead of this we keep first (VIRTIO_BALLOON_S_NR + 32) counters 
> > >>>>> > >>from the
> > >>>>> > >>queue and pass unrecognized ones with the following names: 
> > >>>>> > >>'x-stat-XXXX',
> > >>>>> > >>where XXXX is a tag number in hex. Defined counters are reported 
> > >>>>> > >>with their
> > >>>>> > >>regular names.
> > >>>>> > >>
> > >>>>> > >>Signed-off-by: Igor Redko <address@hidden>
> > >>>>> > >>Signed-off-by: Denis V. Lunev <address@hidden>
> > >>>>> > >>CC: Michael S. Tsirkin <address@hidden>
> > >>>>> > >This seems to open the ABI to abuse.
> > >>>>> > >Seems like a reasonable way to experiment though.
> > >>>>> > >How about adding this within #if 0 statements?
> > >>>>> > >You can uncomment them for debugging ...
> > >>>>> > I'd prefer to have this enabled.
> > 
> > Yes, conditional compilation should be used sparingly.  I don't have an
> > opinion on whether using it here is appropriate.
> > 
> > >>>>> > Why do you think that it opens "abuse" way?
> > >>>>> 
> > >>>>> Because people will use this to hack drivers and management tools
> > >>>>> bypassing qemu.
> > 
> > Easy to avoid: shuffle the N in x-stat-N around from time to time, to
> > reinforce the lesson that you must not rely on their presence or
> > semantics.  I doubt it'll be necessary beyond the renumbering that
> > happens naturally when we add supported counters, or the reshuffling
> > that happens when somebody messes with the unsupported counters.
> > 
> > >>>> I'm curious why you think it's a problem?  Even the existing stats are
> > >>>> simply propagated to the management level by qemu with no processing
> > >>>> other than assigning text labels.  The proposed naming scheme for
> > >>>> unrecognized counters includes "x-" prefix which explicitly marks them
> > >>>> as unstable so people using them take their risk.
> > >>>>
> > >>>> One of the benefits is forward compatibility, so that counters that 
> > >>>> have
> > >>>> graduated into supported ones and have got their own number and name,
> > >>>> can be made to work with qemu that doesn't yet recognize them.
> > >>> Then management does start relying on the x- prefixed things,
> > >>> and once it's used to that it's a slippery slope.
> > >> Any management tool that relies on an x- prefix name is broken.
> > 
> > Or at least assumes the full risk of breaking without notice whenever
> > QEMU changes.  Abbreviating that to just "broken" seems fair enough :)
> > 
> > >>                                                                  We've
> > >> explicitly documented that the x- prefix is unstable and liable to go
> > >> away with a future release. Any management app that wants to use a
> > >> feature beginning with x- should FIRST push hard to get the x- removed
> > >> and stabilize the interface (and libvirt, at least, does just that).
> > >>
> > > this was exactly an original idea. Names started with 'x-' are
> > > _officially_ unstable and for debug purpose. That is why I'd
> > > prefer if v2 of the patchset will be taken.
> > 
> > Looks like fair use of x- to me.
> 
> 
> Well I already heard:
> 
>       One of the benefits is forward compatibility, so that counters that have
>       graduated into supported ones and have got their own number and name,
>       can be made to work with qemu that doesn't yet recognize them.
> 
> in this thread, which seems to mean exactly that people start planning to 
> abuse it
> even before it's merged.

That quote (from yours truly) states the opposite.

The whole point is that there are several participants in the process,
with independent release cycles and policies, but with a common
"registry" of supported stats (with the master copy being in the kernel,
right?).  Once a counter is accepted there, you can start shipping the
guest driver providing it, and you don't have to wait until qemu catches
up: your management level can read "x-stat-NEW_NUMBER" *or* "new_name",
as both NEW_NUMBER and new_name are now allocated for that new counter.

So yes, people are planning to use it, in particluar, before it's merged
into qemu proper, but I don't see how that creates any extra maintenance
burden on qemu side.  Anybody using x- is on their own; the scheme I
sketched seems reasonably safe but is the headache of that management
software anyway.

Roman.