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@rsg.ci.i.u-tokyo.ac.jp>
---
 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.
-
 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.
 
 
 Overlapping regions and priority

-- 
2.51.0

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@rsg.ci.i.u-tokyo.ac.jp>
> ---
>  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.

> -
>  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?

-- 
Peter Xu

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@rsg.ci.i.u-tokyo.ac.jp>
> > ---
> >  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.
> 
> > -
> >  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 didn't see anything elsewhere.  If you agree with above, I can queue this
series with above touched up, then no need to repost.

Thanks,

-- 
Peter Xu

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@rsg.ci.i.u-tokyo.ac.jp>
>>> ---
>>>   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

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@rsg.ci.i.u-tokyo.ac.jp>
> ---
>  docs/devel/memory.rst | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|