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

[Markus Armbruster]

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