Children are automatically unparented so manually unparenting is
unnecessary.

Worse, automatic unparenting happens before the insntance_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.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/vfio/pci.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 07257d0fa049b09fc296ac2279a6fafbdf93d277..2e909c190f86a722e1022fa7c45a96d2dde8d58e 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2000,7 +2000,6 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
         vfio_region_finalize(&bar->region);
         if (bar->mr) {
             assert(bar->size);
-            object_unparent(OBJECT(bar->mr));
             g_free(bar->mr);
             bar->mr = NULL;
         }
@@ -2008,9 +2007,6 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
 
     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);
     }
 }

-- 
2.51.0

On Sat, Sep 06, 2025 at 04:11:11AM +0200, Akihiko Odaki wrote:
> Children are automatically unparented so manually unparenting is
> unnecessary.
> 
> Worse, automatic unparenting happens before the insntance_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.
> 
> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> ---
>  hw/vfio/pci.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 07257d0fa049b09fc296ac2279a6fafbdf93d277..2e909c190f86a722e1022fa7c45a96d2dde8d58e 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2000,7 +2000,6 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
>          vfio_region_finalize(&bar->region);
>          if (bar->mr) {
>              assert(bar->size);
> -            object_unparent(OBJECT(bar->mr));
>              g_free(bar->mr);
>              bar->mr = NULL;
>          }
> @@ -2008,9 +2007,6 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
>  
>      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);
>      }
>  }

So the 2nd object_unparent() here should be no-op, seeing empty list of
properties (but shouldn't causing anything severe), is that correct?

I think it still makes some sense to theoretically allow object_unparent()
to happen, at least when it happens before owner's finalize().  IIUC that
was the intention of the doc, pairing the memory_region_init*() operation.

It might depend on two use cases:

1. MRs dynamically created, it'll share the same lifecycle of the owner
   after creation (just like an embeded MemoryRegion)

   I feel like most, if not all, VFIO's dynamic mrs follows this trend,
   that this patch touched.

   In this case, IMHO instead of object_unparent(), we could also allow the
   owner / device to take ownership of the MR completely, by replacing:

       mr = g_new0(MemoryRegion, 1);

   with:

       mr = object_new(TYPE_MEMORY_REGION);

   Then after memory_region_init*(), essentially the owner will be in
   charge of the memory, as it'll be g_free()ed when remove the mr from
   property list (in owner's finalize() automatically).

   With that, the device impl can not only avoid object_unparent(), but
   avoid g_free() altogether.  That would make some more sense to me,
   instead of relying on memory internal to unparent, and rely on device
   impl to g_free().

2. MRs dynamically created, and it may be freed even before the owner
   finishes its lifecycle

   This is the case that I _think_ an object_unparent() should still be
   allowed, because when the mr is unparented (aka, not used), we should
   still provide a way for the device impl to detach and free the mr
   resources on the fly.

There're a bunch of object_unparent() users, I didn't check whether there's
any real user of case (2), though.

AFAIU for such case maybe it's always better to provide real refcounting
for the mr, since the mr can always be address_space_map()ed.. with an
elevated refcount. In that case, the owner shouldn't be the device impl,
but a temp obj that represents the mr (and do refcounts).

Thanks,

-- 
Peter Xu

On 2025/09/11 5:41, Peter Xu wrote:
> On Sat, Sep 06, 2025 at 04:11:11AM +0200, Akihiko Odaki wrote:
>> Children are automatically unparented so manually unparenting is
>> unnecessary.
>>
>> Worse, automatic unparenting happens before the insntance_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.
>>
>> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
>> ---
>>   hw/vfio/pci.c | 4 ----
>>   1 file changed, 4 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 07257d0fa049b09fc296ac2279a6fafbdf93d277..2e909c190f86a722e1022fa7c45a96d2dde8d58e 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2000,7 +2000,6 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
>>           vfio_region_finalize(&bar->region);
>>           if (bar->mr) {
>>               assert(bar->size);
>> -            object_unparent(OBJECT(bar->mr));
>>               g_free(bar->mr);
>>               bar->mr = NULL;
>>           }
>> @@ -2008,9 +2007,6 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
>>   
>>       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);
>>       }
>>   }
> 
> So the 2nd object_unparent() here should be no-op, seeing empty list of
> properties (but shouldn't causing anything severe), is that correct?

No. The object is finalized with the first object_unparent() if there is 
no referrer other than the parent. The second object_unparent() will 
access the finalized, invalid object in that case.

> 
> I think it still makes some sense to theoretically allow object_unparent()
> to happen, at least when it happens before owner's finalize().  IIUC that
> was the intention of the doc, pairing the memory_region_init*() operation.

Perhaps so, but this patch is only about the case where 
object_unparent() is called in finalize().

Regards,
Akihiko Odaki

On Thu, Sep 11, 2025 at 12:47:24PM +0900, Akihiko Odaki wrote:
> On 2025/09/11 5:41, Peter Xu wrote:
> > On Sat, Sep 06, 2025 at 04:11:11AM +0200, Akihiko Odaki wrote:
> > > Children are automatically unparented so manually unparenting is
> > > unnecessary.
> > > 
> > > Worse, automatic unparenting happens before the insntance_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.
> > > 
> > > Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> > > ---
> > >   hw/vfio/pci.c | 4 ----
> > >   1 file changed, 4 deletions(-)
> > > 
> > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > > index 07257d0fa049b09fc296ac2279a6fafbdf93d277..2e909c190f86a722e1022fa7c45a96d2dde8d58e 100644
> > > --- a/hw/vfio/pci.c
> > > +++ b/hw/vfio/pci.c
> > > @@ -2000,7 +2000,6 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
> > >           vfio_region_finalize(&bar->region);
> > >           if (bar->mr) {
> > >               assert(bar->size);
> > > -            object_unparent(OBJECT(bar->mr));
> > >               g_free(bar->mr);
> > >               bar->mr = NULL;
> > >           }
> > > @@ -2008,9 +2007,6 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
> > >       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);
> > >       }
> > >   }
> > 
> > So the 2nd object_unparent() here should be no-op, seeing empty list of
> > properties (but shouldn't causing anything severe), is that correct?
> 
> No. The object is finalized with the first object_unparent() if there is no
> referrer other than the parent. The second object_unparent() will access the
> finalized, invalid object in that case.

Yes, it's logically wrong.  I was trying to understand the impact when it's
invoked.  In this specific case of above two changes, I believe both MR
structs are still available, so it does look fine, e.g. nothing would crash.

For example, I think it doesn't need to copy stable if there's no real
functional issue involved.

> 
> > 
> > I think it still makes some sense to theoretically allow object_unparent()
> > to happen, at least when it happens before owner's finalize().  IIUC that
> > was the intention of the doc, pairing the memory_region_init*() operation.
> 
> Perhaps so, but this patch is only about the case where object_unparent() is
> called in finalize().

You ignored my other comment.  That (using object_new() on MRs) was what I
was thinking might be better than a split model you discussed here, so
that's also a comment for patch 1 of your series.

Btw, this patch also didn't change all occurances of such for VFIO?
E.g. there's at least vfio_vga_quirk_finalize().  I didn't check the rest.

-- 
Peter Xu

On 2025/09/12 6:37, Peter Xu wrote:
> On Thu, Sep 11, 2025 at 12:47:24PM +0900, Akihiko Odaki wrote:
>> On 2025/09/11 5:41, Peter Xu wrote:
>>> On Sat, Sep 06, 2025 at 04:11:11AM +0200, Akihiko Odaki wrote:
>>>> Children are automatically unparented so manually unparenting is
>>>> unnecessary.
>>>>
>>>> Worse, automatic unparenting happens before the insntance_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.
>>>>
>>>> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
>>>> ---
>>>>    hw/vfio/pci.c | 4 ----
>>>>    1 file changed, 4 deletions(-)
>>>>
>>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>>> index 07257d0fa049b09fc296ac2279a6fafbdf93d277..2e909c190f86a722e1022fa7c45a96d2dde8d58e 100644
>>>> --- a/hw/vfio/pci.c
>>>> +++ b/hw/vfio/pci.c
>>>> @@ -2000,7 +2000,6 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
>>>>            vfio_region_finalize(&bar->region);
>>>>            if (bar->mr) {
>>>>                assert(bar->size);
>>>> -            object_unparent(OBJECT(bar->mr));
>>>>                g_free(bar->mr);
>>>>                bar->mr = NULL;
>>>>            }
>>>> @@ -2008,9 +2007,6 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
>>>>        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);
>>>>        }
>>>>    }
>>>
>>> So the 2nd object_unparent() here should be no-op, seeing empty list of
>>> properties (but shouldn't causing anything severe), is that correct?
>>
>> No. The object is finalized with the first object_unparent() if there is no
>> referrer other than the parent. The second object_unparent() will access the
>> finalized, invalid object in that case.
> 
> Yes, it's logically wrong.  I was trying to understand the impact when it's
> invoked.  In this specific case of above two changes, I believe both MR
> structs are still available, so it does look fine, e.g. nothing would crash.
> 
> For example, I think it doesn't need to copy stable if there's no real
> functional issue involved.

You are right. Cc: stable is unnecessary.

> 
>>
>>>
>>> I think it still makes some sense to theoretically allow object_unparent()
>>> to happen, at least when it happens before owner's finalize().  IIUC that
>>> was the intention of the doc, pairing the memory_region_init*() operation.
>>
>> Perhaps so, but this patch is only about the case where object_unparent() is
>> called in finalize().
> 
> You ignored my other comment.  That (using object_new() on MRs) was what I
> was thinking might be better than a split model you discussed here, so
> that's also a comment for patch 1 of your series.

I'm not sure what you mean by the "split model".

This change removes object_unparent() in vfio_bars_finalize(). 
object_new() will allow removing even g_free(), but we can do these 
changes incrementally:
1. remove object_unparent() in finalize(),
    which fixes a semantic problem (this patch)
2. allow object_new() for MRs and remove g_free() in finalize()
    as a refactoring.

So I suggest focusing on object_unparent() in finalize() to keep this 
patch and review concise.

> 
> Btw, this patch also didn't change all occurances of such for VFIO?
> E.g. there's at least vfio_vga_quirk_finalize().  I didn't check the rest.
> 

Indeed. I only removed object_unparent() calls hw/vfio/pci.c because it 
was mentioned in the documentation. I suspect you will find more cases 
that subregions are used in instance_finalize() in general if you check 
the code base; "[PATCH 11/22] vfio-user: Do not delete the subregion" 
also removes memory_region_del_subregion() during finalization, for example.

Regards,
Akihiko Odaki

On Fri, Sep 12, 2025 at 11:09:51AM +0900, Akihiko Odaki wrote:
> > > > I think it still makes some sense to theoretically allow object_unparent()
> > > > to happen, at least when it happens before owner's finalize().  IIUC that
> > > > was the intention of the doc, pairing the memory_region_init*() operation.
> > > 
> > > Perhaps so, but this patch is only about the case where object_unparent() is
> > > called in finalize().
> > 
> > You ignored my other comment.  That (using object_new() on MRs) was what I
> > was thinking might be better than a split model you discussed here, so
> > that's also a comment for patch 1 of your series.
> 
> I'm not sure what you mean by the "split model".

I meant after similar change as what this patch proposes, (a) "owner of the
MR lifecycle" (aka, who decides to finalize() the MR) is not the same as
(b) "owner of the memory" (aka, who decides to free() the memory backing
the MR struct), so the ownership model itself is more or less "split".

Now it's very hard to tell who owns the MR, because each owns only part of
it.

IMHO it'll be slightly better to have the instance lifecycle and the memory
allocation of the MR struct be managed by the same object, no matter
automatically by the memory core, or manually by the device (in the case of
the current doc, it went with the latter, even though I agree with you it
looks wrong).

> 
> This change removes object_unparent() in vfio_bars_finalize(). object_new()
> will allow removing even g_free(), but we can do these changes
> incrementally:
> 1. remove object_unparent() in finalize(),
>    which fixes a semantic problem (this patch)
> 2. allow object_new() for MRs and remove g_free() in finalize()
>    as a refactoring.
> 
> So I suggest focusing on object_unparent() in finalize() to keep this patch
> and review concise.

I agree that is the minimal change, but this is a common pattern.  It's not
high risk, so I think we could still discuss it thoroughly.

I further analyzed the risk here, it turns out object_unparent() in
finalize() is still very safe so the current code is actually bug-free if
it all works similarly like the vfio code: The object_property_del_all()
(on top of the owner device) would do prop->release(), and here MR will
kickoff object_finalize_child_property(), which resets mr->parent to NULL.

So the 2nd object_unparent() will already see obj->parent==NULL.

Directly dropping object_unparent() should work, but leads to confusion as
"split ownership model" as I discussed above.

Thanks to all recent discussions, IMHO we have much clearer picture of how
MRs can be used.  I discussed it almostly in the first reply:

https://lore.kernel.org/all/aMHidDl1tdx-2G4e@x1.local/

I suspect we don't really have 2nd user I mentioned, because if so it'll
likely require strict mr refcounting due to address_space_map(), in which
case we should go the "create a temp obj as the owner of MR" idea, that you
used to fix the virgl issue in patch 2 of your other series.

For the current issue, I'd suggest as simple as below (I observed at least
the current VFIO use case only uses MMIO memory regions, so we only need
one such helper):

/*
 * Unlike memory_region_init_io(), @memory_region_alloc_io allocates an IO
 * memory region object and returns.
 *
 * After the function returns, the MemoryRegion object will share the same
 * lifecycle of the owner object.  If owner is not specified, the MR will
 * never be released.
 *
 * The caller doesn't need to either detach or unref/free the MR object.
 * It will be automatically detached and returned when the owner finalize.
 * The caller can cache the MR object pointer, but it's only valid to
 * operate before the owner finalizes.
 */
MemoryRegion *
memory_region_alloc_io(MemoryRegion *mr,
                       Object *owner,
                       const MemoryRegionOps *ops,
                       void *opaque,
                       const char *name,
                       uint64_t size)
{
    MemoryRegion = object_new(TYPE_MEMORY_REGION);
    memory_region_do_init(mr, owner, name, size);
    mr->ops = ops ? ops : &unassigned_mem_ops;
    mr->opaque = opaque;
    mr->terminates = true;
}

Here, IIUC if we can allocate the MR using memory_region_alloc_io() here,
then the ownership of both (a)+(b) above will be done automatically by the
memory core / object core code.  The device impl doesn't need to care about
either removal of subregions, or free of MR struct, anymore.  Then we can
drop not only the object_unparent(), but also g_free(), altogether.

Would that sound like a better approach in general?

Again, I don't think this is anything urgent, so we can take time to think
it through.

Thanks,

-- 
Peter Xu

On 2025/09/13 6:26, Peter Xu wrote:
> On Fri, Sep 12, 2025 at 11:09:51AM +0900, Akihiko Odaki wrote:
>>>>> I think it still makes some sense to theoretically allow object_unparent()
>>>>> to happen, at least when it happens before owner's finalize().  IIUC that
>>>>> was the intention of the doc, pairing the memory_region_init*() operation.
>>>>
>>>> Perhaps so, but this patch is only about the case where object_unparent() is
>>>> called in finalize().
>>>
>>> You ignored my other comment.  That (using object_new() on MRs) was what I
>>> was thinking might be better than a split model you discussed here, so
>>> that's also a comment for patch 1 of your series.
>>
>> I'm not sure what you mean by the "split model".
> 
> I meant after similar change as what this patch proposes, (a) "owner of the
> MR lifecycle" (aka, who decides to finalize() the MR) is not the same as
> (b) "owner of the memory" (aka, who decides to free() the memory backing
> the MR struct), so the ownership model itself is more or less "split".
> 
> Now it's very hard to tell who owns the MR, because each owns only part of
> it.
> 
> IMHO it'll be slightly better to have the instance lifecycle and the memory
> allocation of the MR struct be managed by the same object, no matter
> automatically by the memory core, or manually by the device (in the case of
> the current doc, it went with the latter, even though I agree with you it
> looks wrong).

The instance lifecycle and the memory allocation is managed by the same 
object (i.e., the owner). When the owner is being finalized, the owner 
performs the following operations in order in object_finalize():

1. Unparent all children.
2. Call the instance_finalize() callback.

We can say the timing is "split", but the split timing exists even 
without this patch; unparenting happens before instance_finalize(), 
which calls g_free(), with or without this patch. Fixing the split 
timing can be done later.

> 
>>
>> This change removes object_unparent() in vfio_bars_finalize(). object_new()
>> will allow removing even g_free(), but we can do these changes
>> incrementally:
>> 1. remove object_unparent() in finalize(),
>>     which fixes a semantic problem (this patch)
>> 2. allow object_new() for MRs and remove g_free() in finalize()
>>     as a refactoring.
>>
>> So I suggest focusing on object_unparent() in finalize() to keep this patch
>> and review concise.
> 
> I agree that is the minimal change, but this is a common pattern.  It's not
> high risk, so I think we could still discuss it thoroughly.
> 
> I further analyzed the risk here, it turns out object_unparent() in
> finalize() is still very safe so the current code is actually bug-free if
> it all works similarly like the vfio code: The object_property_del_all()
> (on top of the owner device) would do prop->release(), and here MR will
> kickoff object_finalize_child_property(), which resets mr->parent to NULL.
> 
> So the 2nd object_unparent() will already see obj->parent==NULL.
> 
> Directly dropping object_unparent() should work, but leads to confusion as
> "split ownership model" as I discussed above.
> 
> Thanks to all recent discussions, IMHO we have much clearer picture of how
> MRs can be used.  I discussed it almostly in the first reply:
> 
> https://lore.kernel.org/all/aMHidDl1tdx-2G4e@x1.local/
> 
> I suspect we don't really have 2nd user I mentioned, because if so it'll
> likely require strict mr refcounting due to address_space_map(), in which
> case we should go the "create a temp obj as the owner of MR" idea, that you
> used to fix the virgl issue in patch 2 of your other series.
> 
> For the current issue, I'd suggest as simple as below (I observed at least
> the current VFIO use case only uses MMIO memory regions, so we only need
> one such helper):
> 
> /*
>   * Unlike memory_region_init_io(), @memory_region_alloc_io allocates an IO
>   * memory region object and returns.
>   *
>   * After the function returns, the MemoryRegion object will share the same
>   * lifecycle of the owner object.  If owner is not specified, the MR will
>   * never be released.
>   *
>   * The caller doesn't need to either detach or unref/free the MR object.
>   * It will be automatically detached and returned when the owner finalize.
>   * The caller can cache the MR object pointer, but it's only valid to
>   * operate before the owner finalizes.
>   */
> MemoryRegion *
> memory_region_alloc_io(MemoryRegion *mr,
>                         Object *owner,
>                         const MemoryRegionOps *ops,
>                         void *opaque,
>                         const char *name,
>                         uint64_t size)
> {
>      MemoryRegion = object_new(TYPE_MEMORY_REGION);
>      memory_region_do_init(mr, owner, name, size);
>      mr->ops = ops ? ops : &unassigned_mem_ops;
>      mr->opaque = opaque;
>      mr->terminates = true;
> }
> 
> Here, IIUC if we can allocate the MR using memory_region_alloc_io() here,
> then the ownership of both (a)+(b) above will be done automatically by the
> memory core / object core code.  The device impl doesn't need to care about
> either removal of subregions, or free of MR struct, anymore.  Then we can
> drop not only the object_unparent(), but also g_free(), altogether.
> 
> Would that sound like a better approach in general?
> 
> Again, I don't think this is anything urgent, so we can take time to think
> it through.
It makes sense to have a through review, but my argument here is the 
de-duplication of object_unparent() and the replacement of g_free() with 
object_new() are logically distinct and should be split into distinct 
patches. Each patch can independently have through review, be 
applied/backported, or be reverted in case of regression.

Regards,
Akihiko Odaki

On Sun, Sep 14, 2025 at 06:06:44PM +0900, Akihiko Odaki wrote:
> It makes sense to have a through review, but my argument here is the
> de-duplication of object_unparent() and the replacement of g_free() with
> object_new() are logically distinct and should be split into distinct
> patches. Each patch can independently have through review, be
> applied/backported, or be reverted in case of regression.

We're discussing a change in the memory.rst on suggested way to use dynamic
MRs, so I think we can do it in one shot rather than making it confusing.
It's not a huge change even in one go.

It's fine.  You're right we can remove the object_unparent() first when
it's always a no-op.  We'll update the doc twice, though I assume it's fine.

If you would, please consider sending this part as a separate series.

The subject should be something like "remove unnecessary object_unparent()
for dynamic MRs" or something like that.  It has nothing to do with
memleaks on this part.

Please cover tests as much as possible and if we touch the doc we need to
convert everything that uses dynamic MRs, including the missing ones in
VFIO, and also the rest occurances.

Thanks,

-- 
Peter Xu

On 2025/09/16 5:30, Peter Xu wrote:
> On Sun, Sep 14, 2025 at 06:06:44PM +0900, Akihiko Odaki wrote:
>> It makes sense to have a through review, but my argument here is the
>> de-duplication of object_unparent() and the replacement of g_free() with
>> object_new() are logically distinct and should be split into distinct
>> patches. Each patch can independently have through review, be
>> applied/backported, or be reverted in case of regression.
> 
> We're discussing a change in the memory.rst on suggested way to use dynamic
> MRs, so I think we can do it in one shot rather than making it confusing.
> It's not a huge change even in one go.
 > > It's fine.  You're right we can remove the object_unparent() first when
> it's always a no-op.  We'll update the doc twice, though I assume it's fine.
> 
> If you would, please consider sending this part as a separate series.
> 
> The subject should be something like "remove unnecessary object_unparent()
> for dynamic MRs" or something like that.  It has nothing to do with
> memleaks on this part.

I think "Do not unparent in instance_finalize()" is good enough.

"unnecessary" sounds it is correct but only extraneous; in reality it is 
semantically problematic.

"In instance_finalize()" is more appropriate to represent the scope of 
the change than "for dynamic MRs". Unparenting objects when finalizing 
their parents is wrong, whether they are MRs or not. On the other hand, 
unparenting MRs before instance_finalize() is still allowed.

In v2, I dropped patches to fix memory leaks so the series only contains 
ones to fix use-after-finalization. I'll rename the series accordingly.

> 
> Please cover tests as much as possible and if we touch the doc we need to
> convert everything that uses dynamic MRs, including the missing ones in
> VFIO, and also the rest occurances.

I'll search for object_unparent() called from instance_finalize().

Regards,
Akihiko Odaki