Re: [PATCH v4 6/7] memory: Do not create circular reference with subregion

[Peter Maydell]

On Mon, 26 Aug 2024 at 16:22, Peter Xu <peterx@redhat.com> wrote:
>
> On Fri, Aug 23, 2024 at 03:13:11PM +0900, Akihiko Odaki wrote:
> > memory_region_update_container_subregions() used to call
> > memory_region_ref(), which creates a reference to the owner of the
> > subregion, on behalf of the owner of the container. This results in a
> > circular reference if the subregion and container have the same owner.
> >
> > memory_region_ref() creates a reference to the owner instead of the
> > memory region to match the lifetime of the owner and memory region. We
> > do not need such a hack if the subregion and container have the same
> > owner because the owner will be alive as long as the container is.
> > Therefore, create a reference to the subregion itself instead ot its
> > owner in such a case; the reference to the subregion is still necessary
> > to ensure that the subregion gets finalized after the container.
> >
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > ---
> >  system/memory.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/system/memory.c b/system/memory.c
> > index 5e6eb459d5de..e4d3e9d1f427 100644
> > --- a/system/memory.c
> > +++ b/system/memory.c
> > @@ -2612,7 +2612,9 @@ static void 
> > memory_region_update_container_subregions(MemoryRegion *subregion)
> >
> >      memory_region_transaction_begin();
> >
> > -    memory_region_ref(subregion);
> > +    object_ref(mr->owner == subregion->owner ?
> > +               OBJECT(subregion) : subregion->owner);
>
> The only place that mr->refcount is used so far is the owner with the
> object property attached to the mr, am I right (ignoring name-less MRs)?
>
> I worry this will further complicate refcounting, now we're actively using
> two refcounts for MRs..
>
> Continue discussion there:
>
> https://lore.kernel.org/r/067b17a4-cdfc-4f7e-b7e4-28c38e1c10f0@daynix.com
>
> What I don't see is how mr->subregions differs from mr->container, so we
> allow subregions to be attached but not the container when finalize()
> (which is, afaict, the other way round).
>
> It seems easier to me that we allow both container and subregions to exist
> as long as within the owner itself, rather than start heavier use of
> mr->refcount.

I don't think just "same owner" necessarily will be workable --
you can have a setup like:
  * device A has a container C_A
  * device A has a child-device B
  * device B has a memory region R_B
  * device A's realize method puts R_B into C_A

R_B's owner is B, and the container's owner is A,
but we still want to be able to get rid of A (in the process
getting rid of B because it gets unparented and unreffed,
and R_B and C_A also).

-- PMM