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

[Markus Armbruster]

"Michael S. Tsirkin" <address@hidden> writes:

> 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" <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?).
>
> 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.

No, QEMU should aggressively reuse the number part.  Heck, it's free to
randomly mess with it without notice.  Makes the x-stat-N effectively
useless for anything but experimenting.  Which is exactly the point of
naming them x-.

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

x-things always look like they'd invite misuse.  That look goes away
fast when your misuse breaks on every other QEMU upgrade.