From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37327) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aYso1-0006gT-Ns for qemu-devel@nongnu.org; Thu, 25 Feb 2016 05:05:58 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aYsny-0002wx-83 for qemu-devel@nongnu.org; Thu, 25 Feb 2016 05:05:57 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43209) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aYsny-0002wm-0h for qemu-devel@nongnu.org; Thu, 25 Feb 2016 05:05:54 -0500 Date: Thu, 25 Feb 2016 12:05:49 +0200 From: "Michael S. Tsirkin" Message-ID: <20160225114706-mutt-send-email-mst@redhat.com> References: <20160223172048-mutt-send-email-mst@redhat.com> <56CC7ADD.5080508@openvz.org> <20160223174837-mutt-send-email-mst@redhat.com> <20160224100133.GA2519@rkaganb.sw.ru> <20160224162344-mutt-send-email-mst@redhat.com> <56CDCFA7.90308@redhat.com> <56CE962B.40809@openvz.org> <87si0hp4bt.fsf@blackfin.pond.sub.org> <20160225085417.GA9708@redhat.com> <20160225093020.GA26102@rkaganb.sw.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160225093020.GA26102@rkaganb.sw.ru> Subject: Re: [Qemu-devel] [PATCH 1/2] virtio-balloon: export all balloon statistics List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Roman Kagan , Markus Armbruster , "Denis V. Lunev" , Eric Blake , Igor Redko , qemu-devel@nongnu.org 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" writes: > > > > > > > On 02/24/2016 06:43 PM, Eric Blake wrote: > > > >> On 02/24/2016 07:31 AM, Michael S. Tsirkin wrote: > > > >>> Roman Kagan 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 > > > >>>>> > >> > > > >>>>> > >>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 > > > >>>>> > >>Signed-off-by: Denis V. Lunev > > > >>>>> > >>CC: Michael S. Tsirkin > > > >>>>> > >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