Roman Kagan <rka...@virtuozzo.com> writes: > On Thu, Feb 25, 2016 at 11:46:59AM +0100, Markus Armbruster wrote: >> "Michael S. Tsirkin" <m...@redhat.com> 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" <d...@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 <rka...@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 <red...@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 <red...@virtuozzo.com> >> >> > > >>>>> > >>Signed-off-by: Denis V. Lunev <d...@openvz.org> >> >> > > >>>>> > >>CC: Michael S. Tsirkin <m...@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. >> >> 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.
