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

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Feb 25, 2016 at 12:30:21PM +0300, Roman Kagan wrote:
> 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" <den@openvz.org> writes:
> > > 
> > > > On 02/24/2016 06:43 PM, Eric Blake wrote:
> > > >> On 02/24/2016 07:31 AM, Michael S. Tsirkin wrote:
> > > >>> Roman Kagan <rkagan@virtuozzo.com> 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 <redkoi@virtuozzo.com>
> > > >>>>> > >>
> > > >>>>> > >>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 <redkoi@virtuozzo.com>
> > > >>>>> > >>Signed-off-by: Denis V. Lunev <den@openvz.org>
> > > >>>>> > >>CC: Michael S. Tsirkin <mst@redhat.com>
> > > >>>>> > >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?).

For most devices it's the virtio spec.

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

Basically if you do this hack QEMU must not reuse the x-stat-NEW_NUMBER
ever, otherwise management handling it will intepret it
as legacy QEMU and will break.

And the fact we have to argue about it tells me this is
a dangerous place to put the debugging hooks since it
seems to be begging to be misused.

-- 
MST