Re: [PATCH 02/10] rust: qom: add reference counting functionality

[Zhao Liu <zhao1.liu@intel.com>]

On Wed, Jan 29, 2025 at 11:03:31AM +0100, Paolo Bonzini wrote:
> Date: Wed, 29 Jan 2025 11:03:31 +0100
> From: Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [PATCH 02/10] rust: qom: add reference counting functionality
> 
> On Sun, Jan 26, 2025 at 3:56 PM Zhao Liu <zhao1.liu@intel.com> wrote:
> >
> > Hi Paolo,
> >
> > On Fri, Jan 17, 2025 at 08:39:55PM +0100, Paolo Bonzini wrote:
> > > Date: Fri, 17 Jan 2025 20:39:55 +0100
> > > From: Paolo Bonzini <pbonzini@redhat.com>
> > > Subject: [PATCH 02/10] rust: qom: add reference counting functionality
> > > X-Mailer: git-send-email 2.47.1
> > >
> > > Add a smart pointer that allows to add and remove references from
> > > QOM objects.  It's important to note that while all QOM objects have a
> > > reference count, in practice not all of them have their lifetime guarded
> > > by it.
> >
> > About the background, I have a maybe common question...why Rust needs
> > extra reference count guarding?
> 
> Children properties are removed, and thus their reference is dropped,
> before instance_finalize() is called (see object_finalize() in
> qom/object.c). This is not valid in Rust, you need to keep the object
> alive until the last line of Rust code has run - which is after
> Drop::drop() has run.

I see, this is also a typical effort to eliminate unsafe crossing of FFI
boundaries.

> > Additionally, I felt that the ref count may be a bit confusing. After
> > creating Child<> property, the child object's ref count is sometimes 1,
> > and other times it's 2:
> >
> >  * With object_initialize_child(), child's ref count is 1.
> >
> >  * With object_property_add_child() (usually after a object_new() to
> >    create child first):
> >
> >    - sometimes user will call object_unref(), and then the ref count is 1.
> >      E.g., x86_cpu_apic_create() in target/i386/cpu-apic.c.
> >
> >    - sometimes no object_unref(), then ref count is 2.
> >      E.g., exynos4210_realize() in hw/arm/exynos4210.c, creats "cortex-a9".
> 
> In C, having a ref count of 2 is usually a latent memory leak (because
> most of the time there's not going to be an object_unref() in the
> instance_finalize() method). In this case the leak is latent, because
> TYPE_EXYNOS4210_SOC is not hot-unpluggable and thus will never really
> go away once realized.

Further, what about doing object_unref() in object_property_add_child()
or object_property_try_add_child()?

Then we can ensure the object will have ref count of 1 on the exit of
object_property_add_child() and people will no longer miss
object_unref().

Although, there are a few more devices involved to fix similar issues.

> In Rust, this class of leaks simply does not exist with the right API.
> ObjectMethods::property_add_child() could either:
> 
> - take an Owned<T> and consume it, thus always giving a ref count of 1
> on exit. If you want to keep the object you would have to clone it.
>
> - take "&'owner self, &'child T where 'owner: 'child", then you can
> pass an embedded object like object_initialize_child().
> 
> In the latter case however you *still* need to keep the reference
> count elevated until Drop runs. That is, unlike C, Rust code will
> always have a ref count of 2 for children. For this reason, instead of
> having a "T" in the struct you would have another wrapper---something
> like Child<'owner, T>. This wrapper cannot be cloned but it does an
> unref when dropped.

Thanks, the whole picture is nice.

> My expectation is that property_add_child() will be used exclusivel
> for the first case, i.e. it will take an Owned<T>. If you want to
> create a child property from an embedded object, something like
> object_initialize_child() can be used once pinned-init is used to
> rewrite how instance_init is used. It will look something like
> 
> pin_init! {
>   &this in MyClass {
>     ...,
>     iomem <- MemoryRegion::init_io(
>             this,
>             &MY_MR_OPS,
>             "memory-region-name",
>             0x1000,
>     ),
>     child_obj <- ChildClass::init().to_child(this, "prop-name")
>   }
> }
> 
> where to_child() wraps an "impl PinInit<T>" and turns it into an "impl
> PinInit<Child<'a, T>>". Or something like that. :)

Elegant code design, looking forward to pin_init.

> > From this description, I understand your goal is:
> >
> >  * For embedded child object, its lifetimer is managed by its parent
> >    object, through Child<> for the most cases.
> >
> >  * For non-embedded child - a pointer/reference in parent object, its
> >    lifetimer is managed by `Owned<>` (and with Child<>).
> >
> > Am I right?
> 
> Yes, you're right.
> 
> I am not sure if you meant Child<> as the QOM concept, or as a Rust
> struct. If the latter, you're really really right.
> 

Thank you :-) It seems virtio device will have an embedded case to adopt
Child<> struct (virtio_instance_init_common()).

Regards,
Zhao