Re: [PATCH RFC 00/10] qapi: remove all TARGET_* conditionals from the schema

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, May 09, 2025 at 03:43:30PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>> > I think we have to distinguish between what made sense in the context
>> > of our original design, from what makes sense for our future design(s)
>> > and plans.
>> 
>> No argument.
>> 
>> The original design idea is simple: #ifdef CONFIG_FOO for QAPI schema,
>> to not generate dead code, and to not have silly stubs.  Even if you
>> don't care about wasting disk and address space, you probably care about
>> wasting developer time on writing silly stubs and waiting for dead code
>> to compile.
>> 
>> Initially, target-specific macros did not work in conditions.  That was
>> easy enough to fix, so we did.
>
> Ah, yes, forgot that bit of history.
>
>> > I can understand why we took the direction we did historically, but I
>> > think we have unwittingly created problems for ourselves that are
>> > looking increasingly worse than the problems we considered solved.
>> >
>> >
>> > In the other thread I pointed out my dislike for QAPI schema not being
>> > fully self-describing when we have conditionals present, even today,
>> > but there are other aspects that trouble me more wrt conditionals.
>> 
>> I'm not sure I have all this present in my mind...  Can you summarize
>> what troubles you?  Or point me to the message(s)?
>
> Oppps, sorry, should have cross-linked to:
>
>   https://lists.nongnu.org/archive/html/qemu-devel/2025-05/msg01947.html

Hadn't gotten to that thread...  Thanks!

>> > Since the schema is currently target specific, a mgmt app has to probe
>> > for QEMU capabilities once for every target binary. QEMU has ~30 system
>> > binaries, and if querying capabilities takes 250 milliseconds, then
>> > querying all binaries is ~ 7 seconds of work. Libvirt mitigates this
>> > overhead by caching the results of capabilities probes. We periodically
>> > suffer delays when we must invalidate our cache though, typically on
>> > software upgrades, and this is unpleasant for users who think we've
>> > got stuck or just have bad slow code.
>> 
>> How does cache invalidation work?  Timestamp of binary?
>
> ctime of libvirt itself and ctime of QEMU binary are the two
> primary factors.
>
> We had to throw in other factors on top for various edge cases
> over time. So we also check the mtime of the directory containing
> QEMU loadable modules, as features reported can vary if the user
> installs new device/backend modules. Also check kernel version,
> microcode version, CPUID signature, because that can affect
> availability of some features.
>
> NB, this is caching more than just the QMP schema - we issue
> many 'query-xxxx' commands when probing QEMU.
>
> https://gitlab.com/libvirt/libvirt/-/blame/master/src/qemu/qemu_capabilities.c#L5406
>
>> > Even if we had a QAPI schema that didn't vary per target, this is
>> > repeated probing is tricky to avoid when we have completely independant
>> > binaries. We would need QEMU to have some internal "build id", so that
>> > we could detect that all binaries came from the same build, to let us
>> > avoid re-probing each binary.
>> 
>> Back when I created QAPI/QMP introspection, I floated the idea to put
>> something into the QMP greeting to enable safe caching.  Libvirt
>> developers told me they didn't need that.  I don't remember the details,
>> but I guess the cache invalidation they already had was deemed good
>> enough.
>
> I don't recall that discussion, but I would think the problem is
> that we probe much more than just QMP schema. Actually thinking
> about it, the fact that we probe more than just QMP schema means
> my idea of probing once and getting the answer for all targets
> may not be practical. Some of the query-xxx  commands we run will
> likely need to know the target.

Yes.

We could split the cache along validity conditions.  Possibly worth the
extra complexity if expensive probes then live longer in their cache.

I'd love to have a list of probes libvirt runs and why.