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

[Markus Armbruster]

Roman Kagan <address@hidden> writes:

> On Thu, Feb 25, 2016 at 11:46:59AM +0100, Markus Armbruster wrote:
>> "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-.
>
> I must be missing something...  QEMU has no business with the number at
> all unless it recognizes it; in that case it only replaces the dumb
> x-stat-N label with the one it knows.  How can it "randomly mess with
> it"?
>
> Let's get this straight: the only thing QEMU does with balloon stats is
> marshalling them into json.  (For that matter, libvirt only unmarshalls
> them back into an array of (int tag, long value) pairs so is similar.)
> Basically QEMU only plays the role of transport for memory stats between
> the guest driver and the management layer; I'm not even sure why it has
> to know what the individual fields mean.  What this patch proposes is
> essentially to make QEMU not stand in the way of the data it transports;
> it's only the endpoints' responsibility to agree on the interpretation
> of the contents.

If you want to propose a stable interface, don't use the x- prefix.
That's for unstable stuff.  x- like experimental.