Re: [PATCH 5/5] qom: Make container_get() strict to always walk or return container

[Peter Xu <peterx@redhat.com>]

On Tue, Nov 19, 2024 at 09:09:16AM +0100, Paolo Bonzini wrote:
> Il mar 19 nov 2024, 00:06 Peter Xu <peterx@redhat.com> ha scritto:
> 
> > On Mon, Nov 18, 2024 at 05:13:30PM -0500, Peter Xu wrote:
> > > When used incorrectly, container_get() can silently create containers
> > even
> > > if the caller may not intend to do so.  Add a rich document describing
> > the
> > > helper, as container_get() should only be used in path lookups.
> > >
> > > Add one object_dynamic_cast() check to make sure whatever objects the
> > > helper walks will be a container object (including the one to be
> > returned).
> > > It is a programming error otherwise, hence assert that.
> > >
> > > It may make container_get() tiny slower than before, but the hope is the
> > > change is neglictable, as object_class_dynamic_cast() has a fast path
> > just
> > > for similar leaf use case.
> >
> > Just a heads up: out of curiosity, I tried to see whether the fast path hit
> > that I mentioned here (mostly, commit 793c96b54032 of Paolo's), and it
> > didn't..
> >
> > It's fundamentally because all TypeImpl was allocated dynamically from
> > heap, including its type->name.
> 
> 
> Ah, that was supposed to be the difference between type_register() and
> type_register_static().

Ah... looks like they're the same now? As type_register_static() looks like
a wrapper of type_register().

I gave it a shot on booting a Linux guest with some pretty generic devices,
and see how much the pointer check hit.  Until I got the root login prompt,
I got 8 hits out of 35488.  So it's indeed hard to yet hit.. at least with
the current code base. :(

> 
> Perhaps type->name could be allocated with g_intern_string()? And then if
> object_dynamic_cast() is changed into a macro, with something like
> 
> #define qemu_cache_interned_string(s) \
>   (__builtin_constant_p(s) \
>    ? ({ static const char *interned; \
>         interned = interned ?: g_intern_static_string(s); }) \
>    : g_intern_string(s))
> 
> as the third parameter. This allows object_dynamic_cast() to use a simple
> pointer equality for type name comparison, and the same can be applied to
> object_class_dynamic_cast().

Interesting to know this facility!  Though, IIUC this may:

  - For builtin-consts, it grows 8 bytes for each call sites on the binary
    generated, even if (I think...) most of the sites are slow paths, and
    there're plenty of such calls..

  - For non-builtin strings, g_intern_string() will add one more hash
    operation for the whole string (and per discussed previously, looks
    like the string can be not always short..).

So I'm not 100% sure yet if this is what we want.

Do we have known places that we care a lot on object[_class]_dynamic_cast()
performance?  I can give it some measurement if there is, otherwise I'm
guessing whatever changes could fall into the noise.. then we can also
leave that for later, knowing that the fast path will hardly hit for now,
but that shouldn't be a major issue either, I assume.

> 
> Whatever we do, we should do it before Rust code starts using
> object_dynamic_cast!

Thanks!

-- 
Peter Xu