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

[Peter Xu]

On Wed, Aug 28, 2024 at 02:33:59PM +0900, Akihiko Odaki wrote:
> On 2024/08/28 1:11, Peter Xu wrote:
> > On Tue, Aug 27, 2024 at 01:14:51PM +0900, Akihiko Odaki wrote:
> > > On 2024/08/27 4:42, Peter Xu wrote:
> > > > On Mon, Aug 26, 2024 at 06:10:25PM +0100, Peter Maydell wrote:
> > > > > 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..
> > > 
> > > The actor of object_ref() is the owner of the memory region also in this
> > > case. We are calling object_ref() on behalf of mr->owner so we use
> > > mr->refcount iff mr->owner == subregion->owner. In this sense there is 
> > > only
> > > one user of mr->refcount even after this change.
> > 
> > Yes it's still one user, but it's not that straightforward to see, also
> > it's still an extension to how we use mr->refcount right now.  Currently
> > it's about "true / false" just to describe, now it's a real counter.
> > 
> > I wished that counter doesn't even exist if we'd like to stick with device
> > / owner's counter.  Adding this can definitely also make further effort
> > harder if we want to remove mr->refcount.
> 
> I don't think it will make removing mr->refcount harder. With this change,
> mr->refcount will count the parent and container. If we remove mr->refcount,
> we need to trigger object_finalize() in a way other than checking
> mr->refcount, which can be achieved by simply evaluating OBJECT(mr)->parent
> && mr->container.
> 
> > 
> > > 
> > > > > > 
> > > > > > 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).
> > > > 
> > > > For cross-device references, should we rely on an explicit call to
> > > > memory_region_del_subregion(), so as to detach the link between C_A and
> > > > R_B?
> > > 
> > > Yes, I agree.
> > > 
> > > > 
> > > > My understanding so far: logically when MR finalize() it should 
> > > > guarantee
> > > > both (1) mr->container==NULL, and (2) mr->subregions empty.  That's 
> > > > before
> > > > commit 2e2b8eb70fdb7dfb and could be the ideal world (though at the very
> > > > beginning we don't assert on ->container==NULL yet).  It requires all
> > > > device emulations to do proper unrealize() to unlink all the MRs.
> > > > 
> > > > However what I'm guessing is QEMU probably used to have lots of devices
> > > > that are not following the rules and leaking these links.  Hence we have
> > > > had 2e2b8eb70fdb7dfb, allowing that to happen as long as it's safe, and
> > > > it's justified by comment in 2e2b8eb70fdb7dfb on why it's safe.
> > > > 
> > > > What I was thinking is this comment seems to apply too to 
> > > > mr->container, so
> > > > that it should be safe too to unlink ->container the same way as its own
> > > > subregions. >
> > > > IIUC that means for device-internal MR links we should be fine leaving
> > > > whatever link between MRs owned by such device; the device->refcount
> > > > guarantees none of them will be visible in any AS.  But then we need to
> > > > always properly unlink the MRs when the link is across >1 device owners,
> > > > otherwise it's prone to leak.
> > > 
> > > There is one principle we must satisfy in general: keep a reference to a
> > > memory region if it is visible to the guest.
> > > 
> > > It is safe to call memory_region_del_subregion() and to trigger the
> > > finalization of subregions when the container is not referenced because 
> > > they
> > > are no longer visible. This is not true for the other way around; even 
> > > when
> > > subregions are not referenced by anyone else, they are still visible to 
> > > the
> > > guest as long as the container is visible to the guest. It is not safe to
> > > unref and finalize them in such a case.
> > > 
> > > A memory region and its owner will leak if a memory region kept visible 
> > > for
> > > a too long period whether the chain of reference contains a
> > > container/subregion relationship or not.
> > 
> > Could you elaborate why it's still visible to the guest if
> > owner->refcount==0 && mr->container!=NULL?
> > 
> > Firstly, mr->container != NULL means the MR has an user indeed.  It's the
> > matter of who's using it.  If that came from outside this device, it should
> > require memory_region_ref(mr) before hand when adding the subregion, and
> > that will hold one reference on the owner->refcount.
> > 
> > Here owner->refcount==0 means there's no such reference, so it seems to me
> > it's guaranteed to not be visible to anything outside of this device / 
> > owner.
> > Then from that POV it's safe to unlink when the owner is finalizing just
> > like what we do with mr->subregions, no?
> 
> An object is alive during instance_finalize even though its refcount == 0.
> We can't assume all memory regions are dead even if owner->refcount == 0
> because of that.

When you referred to "an object", do you mean the MR being finalized here?

IIUC when the MR reaches its finalize(), it should mean it's not live
anymore.

We have two forms of MR usages right now: either embeded in another Object
/ Device, or dynamically, like VFIOQuirk.

When used embeded, the MR is only finalized when being removed from the
object's property list, that should only happen when the object / device
triggered finalize().  Since the MR will use the owner->refcount so I
suppose it means the MR is not live anymore.

When used dynamically, object_unparent() is needed but that should only
happen when the object / owner is during finalize(), per document:

        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.

Then the MR is also not live.

> In particular, docs/devel/memory.rst says you can call object_unparent()
> in the instance_finalize of the owner. This assumes a memory region will
> not vanish during the execution of the function unless object_unparent()
> is already called for the memory region.

Yes, the MR will not vanish during finalize() of the owner object. However
I don't think it's "live"?  Again, it's based on my definition of
"liveness" as "taking one refcount of its owner", here the owner refcount
is zero.  IOW, I don't expect it to be accessible anywhere from any address
space (e.g. address_space_map()), because they'll all use
memory_region_ref() and that'll ultimately stops the owner from being
finalized.

Thanks,

-- 
Peter Xu