QEMU device refcounting when device creates a container MR

QEMU device refcounting when device creates a container MR

[Peter Maydell]

Hi; does anybody know how device reference counting is supposed
to work when the device creates a "container" MemoryRegion which
it then puts some of its own subregions in to?

As far as I can see when you do memory_region_add_subregion it
increases the refcount on the owner of the subregion. So if a
device creates a container MR in its own init or realize method
and adds sub-MRs that it owns to that container, this increases
the refcount on the device permanently, and so the device won't
ever be deinited.

As a specific example, the usb-chipidea device does this in its
init method, so if you run the arm device-introspect-test under
leak-sanitizer it complains about a memory leak that happens
when the device is put through the "init-introspect-deref" cycle.

-- PMM

Re: QEMU device refcounting when device creates a container MR

[Philippe Mathieu-Daudé]

Cc'ing David / Peter

On 9/3/22 11:33, Peter Maydell wrote:
> Hi; does anybody know how device reference counting is supposed
> to work when the device creates a "container" MemoryRegion which
> it then puts some of its own subregions in to?
> 
> As far as I can see when you do memory_region_add_subregion it
> increases the refcount on the owner of the subregion. So if a
> device creates a container MR in its own init or realize method
> and adds sub-MRs that it owns to that container, this increases
> the refcount on the device permanently, and so the device won't
> ever be deinited.
> 
> As a specific example, the usb-chipidea device does this in its
> init method, so if you run the arm device-introspect-test under
> leak-sanitizer it complains about a memory leak that happens
> when the device is put through the "init-introspect-deref" cycle.
> 
> -- PMM
> 

Re: QEMU device refcounting when device creates a container MR

[Peter Xu]

On Wed, Mar 09, 2022 at 11:40:15AM +0100, Philippe Mathieu-Daudé wrote:
> Cc'ing David / Peter
> 
> On 9/3/22 11:33, Peter Maydell wrote:
> > Hi; does anybody know how device reference counting is supposed
> > to work when the device creates a "container" MemoryRegion which
> > it then puts some of its own subregions in to?
> > 
> > As far as I can see when you do memory_region_add_subregion it
> > increases the refcount on the owner of the subregion. So if a
> > device creates a container MR in its own init or realize method
> > and adds sub-MRs that it owns to that container, this increases
> > the refcount on the device permanently, and so the device won't
> > ever be deinited.
> > 
> > As a specific example, the usb-chipidea device does this in its
> > init method, so if you run the arm device-introspect-test under
> > leak-sanitizer it complains about a memory leak that happens
> > when the device is put through the "init-introspect-deref" cycle.

I'm not extremely sure about this, but.. does it mean that the device may
better put any of the add-subregion operations into realize() rather than
instance_init()?  Then in the unrealize() of the devices we should do
proper del-subregion to release these refcounts.

Otherwise indeed I don't see a good way to destroy the device anymore,
because the assumption is after device initialized, only with that will the
object_unref() continue to work on the device..

That means, perhaps in object_init_with_type() we should make sure the
object refcount==1 after the ->instance_init() call?

-- 
Peter Xu

Re: QEMU device refcounting when device creates a container MR

[Peter Maydell]

On Thu, 10 Mar 2022 at 13:19, Peter Xu <peterx@redhat.com> wrote:
>
> On Wed, Mar 09, 2022 at 11:40:15AM +0100, Philippe Mathieu-Daudé wrote:
> > Cc'ing David / Peter
> >
> > On 9/3/22 11:33, Peter Maydell wrote:
> > > Hi; does anybody know how device reference counting is supposed
> > > to work when the device creates a "container" MemoryRegion which
> > > it then puts some of its own subregions in to?
> > >
> > > As far as I can see when you do memory_region_add_subregion it
> > > increases the refcount on the owner of the subregion. So if a
> > > device creates a container MR in its own init or realize method
> > > and adds sub-MRs that it owns to that container, this increases
> > > the refcount on the device permanently, and so the device won't
> > > ever be deinited.
> > >
> > > As a specific example, the usb-chipidea device does this in its
> > > init method, so if you run the arm device-introspect-test under
> > > leak-sanitizer it complains about a memory leak that happens
> > > when the device is put through the "init-introspect-deref" cycle.
>
> I'm not extremely sure about this, but.. does it mean that the device may
> better put any of the add-subregion operations into realize() rather than
> instance_init()?  Then in the unrealize() of the devices we should do
> proper del-subregion to release these refcounts.

That would be one option. In the specific case of chipidea it would
alter the ordering in which the container gets set up, which is
currently:
 * chipidea's MRs added to container (in chipidea instance_init)
 * usb-ehci's MRs added to container (in usb_ehci_realize)
Because chipidea is a subclass, moving its "add MRs to container"
to its own realize would put them after the addition of the
usb-ehci MRs. I haven't checked yet whether this would change
the semantics. (If it does, we can probably fix it by using
explicit priorities, which we should anyway if the various
sub-region MRs overlap.)

We don't document anywhere that this must be done in realize
and not instance_init, though.

> Otherwise indeed I don't see a good way to destroy the device anymore,
> because the assumption is after device initialized, only with that will the
> object_unref() continue to work on the device..

The other theory I had was "maybe if you put an MR into another
MR and they both have the same owner then don't bump the refcount"
but I haven't thought that through at all.

> That means, perhaps in object_init_with_type() we should make sure the
> object refcount==1 after the ->instance_init() call?

That's probably a useful invariant, which I bet we don't
currently get right for every object :-)

-- PMM

Re: QEMU device refcounting when device creates a container MR

[Peter Xu]

On Thu, Mar 10, 2022 at 01:45:31PM +0000, Peter Maydell wrote:
> The other theory I had was "maybe if you put an MR into another
> MR and they both have the same owner then don't bump the refcount"
> but I haven't thought that through at all.

Maybe it'll work, it just sounds a bit more tricky.

> 
> > That means, perhaps in object_init_with_type() we should make sure the
> > object refcount==1 after the ->instance_init() call?
> 
> That's probably a useful invariant, which I bet we don't
> currently get right for every object :-)

Yeah same feeling.. :)

We could have looped over each device and try creating them and fix one by
one?  But I think that doesn't cover all the cases, because instance_init()
could have some sub-regions conditionally added depending on the device
parameters and so on, so they may not be covered by the default values
being tested with.

-- 
Peter Xu

Re: QEMU device refcounting when device creates a container MR

[Paolo Bonzini]

On 3/9/22 11:33, Peter Maydell wrote:
> Hi; does anybody know how device reference counting is supposed
> to work when the device creates a "container" MemoryRegion which
> it then puts some of its own subregions in to?
> 
> As far as I can see when you do memory_region_add_subregion it
> increases the refcount on the owner of the subregion. So if a
> device creates a container MR in its own init or realize method
> and adds sub-MRs that it owns to that container, this increases
> the refcount on the device permanently, and so the device won't
> ever be deinited.

The unparent method is supposed to break reference cycles.

In the case of QOM, unparent calls unrealize, so unrealize should remove 
the subregions it created.

Paolo

Re: QEMU device refcounting when device creates a container MR

[Peter Maydell]

On Wed, 9 Mar 2022 at 16:21, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 3/9/22 11:33, Peter Maydell wrote:
> > Hi; does anybody know how device reference counting is supposed
> > to work when the device creates a "container" MemoryRegion which
> > it then puts some of its own subregions in to?
> >
> > As far as I can see when you do memory_region_add_subregion it
> > increases the refcount on the owner of the subregion. So if a
> > device creates a container MR in its own init or realize method
> > and adds sub-MRs that it owns to that container, this increases
> > the refcount on the device permanently, and so the device won't
> > ever be deinited.
>
> The unparent method is supposed to break reference cycles.
>
> In the case of QOM, unparent calls unrealize, so unrealize should remove
> the subregions it created.

It seems asymmetric for unrealize to undo something that was
done in instance_init, though. I would expect unrealize to
undo the effects of realize, and instance_finalize to undo
the effects of instance_init.

-- PMM

Re: QEMU device refcounting when device creates a container MR

[Peter Maydell]

On Wed, 9 Mar 2022 at 16:53, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 9 Mar 2022 at 16:21, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > On 3/9/22 11:33, Peter Maydell wrote:
> > > Hi; does anybody know how device reference counting is supposed
> > > to work when the device creates a "container" MemoryRegion which
> > > it then puts some of its own subregions in to?
> > >
> > > As far as I can see when you do memory_region_add_subregion it
> > > increases the refcount on the owner of the subregion. So if a
> > > device creates a container MR in its own init or realize method
> > > and adds sub-MRs that it owns to that container, this increases
> > > the refcount on the device permanently, and so the device won't
> > > ever be deinited.
> >
> > The unparent method is supposed to break reference cycles.
> >
> > In the case of QOM, unparent calls unrealize, so unrealize should remove
> > the subregions it created.
>
> It seems asymmetric for unrealize to undo something that was
> done in instance_init, though. I would expect unrealize to
> undo the effects of realize, and instance_finalize to undo
> the effects of instance_init.

...also, in the device-introspect-test where I see this problem,
unrealize is never going to be called anyway, because the device
is only put through "instance_init" and then dereffed (which
does not result in instance_finalize being called, because the
refcount is still non-zero).

-- PMM

Re: QEMU device refcounting when device creates a container MR

[Igor Mammedov]

On Wed, 9 Mar 2022 16:56:21 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Wed, 9 Mar 2022 at 16:53, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Wed, 9 Mar 2022 at 16:21, Paolo Bonzini <pbonzini@redhat.com> wrote:  
> > >
> > > On 3/9/22 11:33, Peter Maydell wrote:  
> > > > Hi; does anybody know how device reference counting is supposed
> > > > to work when the device creates a "container" MemoryRegion which
> > > > it then puts some of its own subregions in to?
> > > >
> > > > As far as I can see when you do memory_region_add_subregion it
> > > > increases the refcount on the owner of the subregion. So if a
> > > > device creates a container MR in its own init or realize method
> > > > and adds sub-MRs that it owns to that container, this increases
> > > > the refcount on the device permanently, and so the device won't
> > > > ever be deinited.  
> > >
> > > The unparent method is supposed to break reference cycles.
> > >
> > > In the case of QOM, unparent calls unrealize, so unrealize should remove
> > > the subregions it created.  

well, making it asymmetric doesn't sound nice.
I think unrealize() should deal only with stuff initialized in
realize(), and instance_finalize() undo whatever  instance_init() did.
And if we got beyond Device type, i.e. directly inherited from "Object"
(/me thinking about backends) attempt to hijack unrealize() won't help.

> > It seems asymmetric for unrealize to undo something that was
> > done in instance_init, though. I would expect unrealize to
> > undo the effects of realize, and instance_finalize to undo
> > the effects of instance_init.  
> 
> ...also, in the device-introspect-test where I see this problem,
> unrealize is never going to be called anyway, because the device
> is only put through "instance_init" and then dereffed (which
> does not result in instance_finalize being called, because the
> refcount is still non-zero).

question is why introspected device is deferred instead of being
destroyed if it's no longer needed?

> 
> -- PMM
> 

Re: QEMU device refcounting when device creates a container MR

[Peter Maydell]

On Thu, 10 Mar 2022 at 15:36, Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Wed, 9 Mar 2022 16:56:21 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
> > ...also, in the device-introspect-test where I see this problem,
> > unrealize is never going to be called anyway, because the device
> > is only put through "instance_init" and then dereffed (which
> > does not result in instance_finalize being called, because the
> > refcount is still non-zero).
>
> question is why introspected device is deferred instead of being
> destroyed if it's no longer needed?

...because the reference count is not zero.

What is supposed to happen is:
 * device is created (inited), and has refcount of 1
 * introspection code does its thing
 * introspection code derefs the device, and it gets deinited

This bug means that when the device is inited it has a refcount
that is too high, and so despite the code that creates it
correctly dereffing it, it's still lying around.

thanks
-- PMM

Re: QEMU device refcounting when device creates a container MR

[Igor Mammedov]

Do On Thu, 10 Mar 2022 16:05:24 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Thu, 10 Mar 2022 at 15:36, Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Wed, 9 Mar 2022 16:56:21 +0000
> > Peter Maydell <peter.maydell@linaro.org> wrote:  
> > > ...also, in the device-introspect-test where I see this problem,
> > > unrealize is never going to be called anyway, because the device
> > > is only put through "instance_init" and then dereffed (which
> > > does not result in instance_finalize being called, because the
> > > refcount is still non-zero).  
> >
> > question is why introspected device is deferred instead of being
> > destroyed if it's no longer needed?  
> 
> ...because the reference count is not zero.
> 
> What is supposed to happen is:
>  * device is created (inited), and has refcount of 1
>  * introspection code does its thing
>  * introspection code derefs the device, and it gets deinited
> 
> This bug means that when the device is inited it has a refcount
> that is too high, and so despite the code that creates it
> correctly dereffing it, it's still lying around.

looks like ref count leak somewhere, instance_finalize() take care
of cleaning up instance_init() actions.
Do you have an example/reproducer?

> thanks
> -- PMM
> 

Re: QEMU device refcounting when device creates a container MR

[Peter Maydell]

On Thu, 10 Mar 2022 at 16:30, Igor Mammedov <imammedo@redhat.com> wrote:
>
> Do On Thu, 10 Mar 2022 16:05:24 +0000
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
> > On Thu, 10 Mar 2022 at 15:36, Igor Mammedov <imammedo@redhat.com> wrote:
> > >
> > > On Wed, 9 Mar 2022 16:56:21 +0000
> > > Peter Maydell <peter.maydell@linaro.org> wrote:
> > > > ...also, in the device-introspect-test where I see this problem,
> > > > unrealize is never going to be called anyway, because the device
> > > > is only put through "instance_init" and then dereffed (which
> > > > does not result in instance_finalize being called, because the
> > > > refcount is still non-zero).
> > >
> > > question is why introspected device is deferred instead of being
> > > destroyed if it's no longer needed?
> >
> > ...because the reference count is not zero.
> >
> > What is supposed to happen is:
> >  * device is created (inited), and has refcount of 1
> >  * introspection code does its thing
> >  * introspection code derefs the device, and it gets deinited
> >
> > This bug means that when the device is inited it has a refcount
> > that is too high, and so despite the code that creates it
> > correctly dereffing it, it's still lying around.
>
> looks like ref count leak somewhere, instance_finalize() take care
> of cleaning up instance_init() actions.

If you read the rest of the thread, we know why the refcount
is too high. And instance_finalize *is never called*, so it
cannot clean up what instance_init has done.

> Do you have an example/reproducer?

Yes, see the thread -- device-introspect-test shows it.
(You can put printfs in ehci_sysbus_init and ehci_sysbus_finalize
and see that for some devices we don't ever call finalize.)

-- PMM

Re: QEMU device refcounting when device creates a container MR

[Igor Mammedov]

On Thu, 10 Mar 2022 17:11:14 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On Thu, 10 Mar 2022 at 16:30, Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > Do On Thu, 10 Mar 2022 16:05:24 +0000
> > Peter Maydell <peter.maydell@linaro.org> wrote:
> >  
> > > On Thu, 10 Mar 2022 at 15:36, Igor Mammedov <imammedo@redhat.com> wrote:  
> > > >
> > > > On Wed, 9 Mar 2022 16:56:21 +0000
> > > > Peter Maydell <peter.maydell@linaro.org> wrote:  
> > > > > ...also, in the device-introspect-test where I see this problem,
> > > > > unrealize is never going to be called anyway, because the device
> > > > > is only put through "instance_init" and then dereffed (which
> > > > > does not result in instance_finalize being called, because the
> > > > > refcount is still non-zero).  
> > > >
> > > > question is why introspected device is deferred instead of being
> > > > destroyed if it's no longer needed?  
> > >
> > > ...because the reference count is not zero.
> > >
> > > What is supposed to happen is:
> > >  * device is created (inited), and has refcount of 1
> > >  * introspection code does its thing
> > >  * introspection code derefs the device, and it gets deinited
> > >
> > > This bug means that when the device is inited it has a refcount
> > > that is too high, and so despite the code that creates it
> > > correctly dereffing it, it's still lying around.  
> >
> > looks like ref count leak somewhere, instance_finalize() take care
> > of cleaning up instance_init() actions.  
> 
> If you read the rest of the thread, we know why the refcount
> is too high. And instance_finalize *is never called*, so it
> cannot clean up what instance_init has done.
> 
> > Do you have an example/reproducer?  
> 
> Yes, see the thread -- device-introspect-test shows it.
> (You can put printfs in ehci_sysbus_init and ehci_sysbus_finalize
> and see that for some devices we don't ever call finalize.)

something like following might work.

basic idea is avoid cyclic references when subregion and container
have the same owner.
And properly handle references of subregion itsef when it's added to container,
this is necessary to prevent subregion being freed (when it's removed as a child property)
since container might still exist and 'referencing' subregion.
So that later when container is finalized it would call del_region()
on still alive subregion.


diff --git a/softmmu/memory.c b/softmmu/memory.c
index 8060c6de78..499c20fcef 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -2527,8 +2527,11 @@ static void memory_region_update_container_subregions(MemoryRegion *subregion)
     MemoryRegion *other;
 
     memory_region_transaction_begin();
+    object_ref(subregion);
 
-    memory_region_ref(subregion);
+    if (subregion->container->owner != subregion->owner) {
+        memory_region_ref(subregion);
+    }
     QTAILQ_FOREACH(other, &mr->subregions, subregions_link) {
         if (subregion->priority >= other->priority) {
             QTAILQ_INSERT_BEFORE(other, subregion, subregions_link);
@@ -2580,14 +2583,17 @@ void memory_region_del_subregion(MemoryRegion *mr,
 
     memory_region_transaction_begin();
     assert(subregion->container == mr);
-    subregion->container = NULL;
     for (alias = subregion->alias; alias; alias = alias->alias) {
         alias->mapped_via_alias--;
         assert(alias->mapped_via_alias >= 0);
     }
     QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
-    memory_region_unref(subregion);
+    if (subregion->container->owner != subregion->owner) {
+        memory_region_unref(subregion);
+    }
+    subregion->container = NULL;
     memory_region_update_pending |= mr->enabled && subregion->enabled;
+    object_unref(subregion);
     memory_region_transaction_commit();
 }

> 
> -- PMM
>