Re: [PATCH v3 1/7] docs/devel: Do not unparent in instance_finalize()

[Akihiko Odaki <odaki@xxxxxxxxxxxxxxxxxxxxxx>]

On 2025/09/19 5:11, Peter Xu wrote:
> On Thu, Sep 18, 2025 at 04:03:49PM -0400, Peter Xu wrote:
> > On Wed, Sep 17, 2025 at 07:13:26PM +0900, Akihiko Odaki wrote:
> > > Children are automatically unparented so manually unparenting is
> > > unnecessary.
> > >
> > > Worse, automatic unparenting happens before the instance_finalize()
> > > callback of the parent gets called, so object_unparent() calls in
> > > the callback will refer to objects that are already unparented, which
> > > is semantically incorrect.
> > >
> > > Remove the instruction to call object_unparent(), and the exception
> > > of the "do not call object_unparent()" rule for instance_finalize().
> > >
> > > Signed-off-by: Akihiko Odaki <odaki@xxxxxxxxxxxxxxxxxxxxxx>
> > > ---
> > >   docs/devel/memory.rst | 19 ++++++-------------
> > >   1 file changed, 6 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
> > > index 57fb2aec76e0..749f11d8a4dd 100644
> > > --- a/docs/devel/memory.rst
> > > +++ b/docs/devel/memory.rst
> > > @@ -161,18 +161,11 @@ or never.
> > >   Destruction of a memory region happens automatically when the owner
> > >   object dies.
> > >   
> > > -If however the memory region is part of a dynamically allocated data
> > > -structure, you should call object_unparent() to destroy the memory region
> > > -before the data structure is freed.  For an example see VFIOMSIXInfo
> > > -and VFIOQuirk in hw/vfio/pci.c.
> > Should we still keep some of these examples?  After the series they'll be
> > doing the right things.  Dynamic MRs are still slightly tricky, I think
> > it's still good to have some references.
I agree. I'll restore it with the next version.

> > > -
> > >   You must not destroy a memory region as long as it may be in use by a
> > >   device or CPU.  In order to do this, as a general rule do not create or
> > > -destroy memory regions dynamically during a device's lifetime, and only
> > > -call object_unparent() in the memory region owner's instance_finalize
> > > -callback.  The dynamically allocated data structure that contains the
> > > -memory region then should obviously be freed in the instance_finalize
> > > -callback as well.
> > > +destroy memory regions dynamically during a device's lifetime.
> > > +The dynamically allocated data structure that contains the
> > > +memory region should be freed in the instance_finalize callback.
> > >   
> > >   If you break this rule, the following situation can happen:
> > >   
> > > @@ -198,9 +191,9 @@ this exception is rarely necessary, and therefore it is discouraged,
> > >   but nevertheless it is used in a few places.
> > >   
> > >   For regions that "have no owner" (NULL is passed at creation time), the
> > > -machine object is actually used as the owner.  Since instance_finalize is
> > > -never called for the machine object, you must never call object_unparent
> > > -on regions that have no owner, unless they are aliases or containers.
> > > +machine object is actually used as the owner.  You must never call
> > > +object_unparent on regions that have no owner, unless they are aliases
> > > +or containers.
> > This looks like a completely separate change.  So we start to allow
> > machines to be finalized now?  I'm not familiar with machine object
> > lifecycles.  Maybe split it out even if it's true?
I intended to remove phrase "since instance_finalize is never called for 
the machine object" because whether instance_finalize is called or not 
is no longer relevant, and thus object_unparent is always prohibited, 
whether regions have owners or not, unless they are aliases or containers.
The statement still mentions "regions that have no owner"; the 
restriction of object_unparent is enforced whether the regions have 
owners, so it is a bit misleading.
> I didn't see anything elsewhere.  If you agree with above, I can queue this
> series with above touched up, then no need to repost.
I guess I will rewrite this patch, restoring the VFIOQuirk example, and 
re-check if this whole section is structured logically and consistently.
Regards,
Akihiko Odaki