On Thu, 10 Mar 2022 at 13:19, Peter Xu <pet...@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
