docs/devel: Prohibit calling object_unparent() for memory region

Previously it was allowed to call object_unparent() for a memory region
in instance_finalize() of its parent. However, such a call typically
has no effect because child objects get unparented before
instance_finalize().

Worse, memory regions typically gets finalized when they get unparented
before instance_finalize(). This means calling object_unparent() for
them in instance_finalize() is to call the function for an object
already finalized, which should be avoided.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 docs/devel/memory.rst | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)


---
base-commit: 31669121a01a14732f57c49400bc239cf9fd505f
change-id: 20240829-memory-cfd3ee0af44d

Best regards,

On Thu, Aug 29, 2024 at 02:46:48PM +0900, Akihiko Odaki wrote:
> Previously it was allowed to call object_unparent() for a memory region
> in instance_finalize() of its parent. However, such a call typically
> has no effect because child objects get unparented before
> instance_finalize().
> 
> Worse, memory regions typically gets finalized when they get unparented
> before instance_finalize(). This means calling object_unparent() for
> them in instance_finalize() is to call the function for an object
> already finalized, which should be avoided.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>


Acked-by: Michael S. Tsirkin <mst@redhat.com>

who's applying this? Paolo?

> ---
>  docs/devel/memory.rst | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
> index 69c5e3f914ac..83760279e3db 100644
> --- a/docs/devel/memory.rst
> +++ b/docs/devel/memory.rst
> @@ -168,11 +168,10 @@ 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, and do not
> +call object_unparent().  The dynamically allocated data structure that contains
> +the memory region then should be freed in the instance_finalize callback, which
> +is called after it gets unparented.
>  
>  If you break this rule, the following situation can happen:
>  
> @@ -199,8 +198,9 @@ 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.
> +never called for the machine object, you must never free regions that have no
> +owner, unless they are aliases or containers, which you can manually call
> +object_unparent() for.
>  
>  
>  Overlapping regions and priority
> 
> ---
> base-commit: 31669121a01a14732f57c49400bc239cf9fd505f
> change-id: 20240829-memory-cfd3ee0af44d
> 
> Best regards,
> -- 
> Akihiko Odaki <akihiko.odaki@daynix.com>

On Tue, 10 Sept 2024 at 18:27, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Aug 29, 2024 at 02:46:48PM +0900, Akihiko Odaki wrote:
> > Previously it was allowed to call object_unparent() for a memory region
> > in instance_finalize() of its parent. However, such a call typically
> > has no effect because child objects get unparented before
> > instance_finalize().
> >
> > Worse, memory regions typically gets finalized when they get unparented
> > before instance_finalize(). This means calling object_unparent() for
> > them in instance_finalize() is to call the function for an object
> > already finalized, which should be avoided.
> >
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
> who's applying this? Paolo?

It's a docs change to clarify a complex point that's
under active discussion in a different patch thread,
so it needs review before anybody applies it...

-- PMM

On Thu, 29 Aug 2024 at 06:46, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:

Hi; sorry it's taken me so long to get back to this patch,
but I've now re-read some of the discussion in the other threads.
I generally agree with your reasoning and think we do need
to update the docs here.

I think we could be more specific in this commit message; I've
suggested some expansions of it below. (Partly this is for
my own benefit working through what's going on.)

> Previously it was allowed to call object_unparent() for a memory region
> in instance_finalize() of its parent. However, such a call typically
> has no effect because child objects get unparented before
> instance_finalize().

"because child objects are properties of their owner object, and
(since commit 76a6e1cc7cc3a) we delete an object's properties before
calling the object's instance_finalize method. Deleting the
child property will unparent the child; the later calls to
object_unparent() in its owner's instance_finalize will do nothing.".

> Worse, memory regions typically gets finalized when they get unparented
> before instance_finalize().

"before instance_finalize(), because the only reference to the
MemoryRegion is the one we hold because it is a child property
of its owner, and so when object_finalize_child_property()
calls object_unref(child) the refcount drops to zero and
we finalize the MR."

> This means calling object_unparent() for
> them in instance_finalize() is to call the function for an object
> already finalized, which should be avoided.

"This doesn't cause any bad effects in the common case where we
know that the memory allocated for the MemoryRegion itself
is still valid, such as in the common case where the MR
struct is directly embedded in its owner's device state struct.
But it could potentially cause a crash if the MemoryRegion
struct was somewhere else and was freed.

Update the documentation to explicitly prohibit calling
object_unparent() in instance_finalize."

> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

> ---
>  docs/devel/memory.rst | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
> index 69c5e3f914ac..83760279e3db 100644
> --- a/docs/devel/memory.rst
> +++ b/docs/devel/memory.rst
> @@ -168,11 +168,10 @@ and VFIOQuirk in hw/vfio/pci.c.

Don't we need also to change the paragraph before this, which
reads:

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

?

Otherwise we have a paragraph that says "you should call
object_unparent()" followed by one that says "do not call
object_unparent()".

(Also that paragraph's reference to VFIOMSIXInfo is now
out of date -- in 2017 we removed the embedded MemoryRegion
from that struct.)

>  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, and do not
> +call object_unparent().  The dynamically allocated data structure that contains
> +the memory region then should be freed in the instance_finalize callback, which
> +is called after it gets unparented.

I think some of these lines are a touch over-length, and should
be wrapped a bit earlier.

Also, this now says "don't unparent in finalize", but
vfio_bars_finalize() has code which follows the old documentation:

    if (vdev->vga) {
        vfio_vga_quirk_finalize(vdev);
        for (i = 0; i < ARRAY_SIZE(vdev->vga->region); i++) {
            object_unparent(OBJECT(&vdev->vga->region[i].mem));
        }
        g_free(vdev->vga);
    }

Is this wrong and needs changing?

>  If you break this rule, the following situation can happen:
>
> @@ -199,8 +198,9 @@ 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.
> +never called for the machine object, you must never free regions that have no
> +owner, unless they are aliases or containers, which you can manually call
> +object_unparent() for.

thanks
-- PMM

On 2024/10/08 22:33, Peter Maydell wrote:
> On Thu, 29 Aug 2024 at 06:46, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> 
> Hi; sorry it's taken me so long to get back to this patch,
> but I've now re-read some of the discussion in the other threads.
> I generally agree with your reasoning and think we do need
> to update the docs here.
> 
> I think we could be more specific in this commit message; I've
> suggested some expansions of it below. (Partly this is for
> my own benefit working through what's going on.)
> 
>> Previously it was allowed to call object_unparent() for a memory region
>> in instance_finalize() of its parent. However, such a call typically
>> has no effect because child objects get unparented before
>> instance_finalize().
> 
> "because child objects are properties of their owner object, and
> (since commit 76a6e1cc7cc3a) we delete an object's properties before
> calling the object's instance_finalize method. Deleting the
> child property will unparent the child; the later calls to
> object_unparent() in its owner's instance_finalize will do nothing.".
> 
>> Worse, memory regions typically gets finalized when they get unparented
>> before instance_finalize().
> 
> "before instance_finalize(), because the only reference to the
> MemoryRegion is the one we hold because it is a child property
> of its owner, and so when object_finalize_child_property()
> calls object_unref(child) the refcount drops to zero and
> we finalize the MR."
> 
>> This means calling object_unparent() for
>> them in instance_finalize() is to call the function for an object
>> already finalized, which should be avoided.
> 
> "This doesn't cause any bad effects in the common case where we
> know that the memory allocated for the MemoryRegion itself
> is still valid, such as in the common case where the MR
> struct is directly embedded in its owner's device state struct.
> But it could potentially cause a crash if the MemoryRegion
> struct was somewhere else and was freed.

It won't cause a crash even if the MR struct is not embedded as long as 
the data structure is freed in the instance_finalize callback. I suggest 
the following:

"This doesn't cause any bad effects in the common case where the
data structure is managed by the owner because the memory allocated for
the MemoryRegion itself is valid until the owner is also finalized.
However, it is still semantically wrong to touch the MemoryRegion after
its finalization and confusing."

Your other suggestions look good so I'll apply them with v2.

> 
> Update the documentation to explicitly prohibit calling
> object_unparent() in instance_finalize."
> 
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> 
>> ---
>>   docs/devel/memory.rst | 14 +++++++-------
>>   1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
>> index 69c5e3f914ac..83760279e3db 100644
>> --- a/docs/devel/memory.rst
>> +++ b/docs/devel/memory.rst
>> @@ -168,11 +168,10 @@ and VFIOQuirk in hw/vfio/pci.c.
> 
> Don't we need also to change the paragraph before this, which
> reads:
> 
>> 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.
> 
> ?
> 
> Otherwise we have a paragraph that says "you should call
> object_unparent()" followed by one that says "do not call
> object_unparent()".

You are right. I'll remove the statement.

> 
> (Also that paragraph's reference to VFIOMSIXInfo is now
> out of date -- in 2017 we removed the embedded MemoryRegion
> from that struct.)
> 
>>   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, and do not
>> +call object_unparent().  The dynamically allocated data structure that contains
>> +the memory region then should be freed in the instance_finalize callback, which
>> +is called after it gets unparented.
> 
> I think some of these lines are a touch over-length, and should
> be wrapped a bit earlier.

I'll wrap them by 72 columns.

> 
> Also, this now says "don't unparent in finalize", but
> vfio_bars_finalize() has code which follows the old documentation:
> 
>      if (vdev->vga) {
>          vfio_vga_quirk_finalize(vdev);
>          for (i = 0; i < ARRAY_SIZE(vdev->vga->region); i++) {
>              object_unparent(OBJECT(&vdev->vga->region[i].mem));
>          }
>          g_free(vdev->vga);
>      }
> 
> Is this wrong and needs changing?

I'll add a patch to remove object_unparent() calls from hw/vfio/pci.c 
with v2.

Regards,
Akihiko Odaki

> 
>>   If you break this rule, the following situation can happen:
>>
>> @@ -199,8 +198,9 @@ 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.
>> +never called for the machine object, you must never free regions that have no
>> +owner, unless they are aliases or containers, which you can manually call
>> +object_unparent() for.
> 
> thanks
> -- PMM