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

[Peter Xu]

On Fri, Aug 30, 2024 at 03:11:38PM +0900, Akihiko Odaki wrote:
> On 2024/08/30 4:48, Peter Xu wrote:
> > On Thu, Aug 29, 2024 at 01:39:36PM +0900, Akihiko Odaki wrote:
> > > > > I am calling the fact that embedded memory regions are accessible in
> > > > > instance_finalize() "live". A device can perform operations on its 
> > > > > memory
> > > > > regions during instance_finalize() and we should be aware of that.
> > > > 
> > > > This part is true.  I suppose we should still suggest device finalize() 
> > > > to
> > > > properly detach MRs, and that should normally be done there.
> > > 
> > > It is better to avoid manual resource deallocation in general because it 
> > > is
> > > too error-prone.
> > 
> > I had an impression that you mixed up "finalize()" and "free()" in the
> > context of our discussion.. let us clarify this first before everything
> > else below, just in case I overlook stuff..
> I am aware of that distinction. I dealt with it with patch "virtio-gpu:
> Handle resource blob commands":
> https://lore.kernel.org/r/20240822185110.1757429-12-dmitry.osipenko@collabora.com
> 
> > 
> > MR is very special as an object, in that it should have no free() hook,
> > hence by default nothing is going to be freed when mr->refcount==0.  It
> > means MRs need to be freed manually always.
> > 
> > For example:
> > 
> > (gdb) p system_memory->parent_obj->free
> > $2 = (ObjectFree *) 0x0
> > 
> > It plays perfect because the majority of QEMU device model is using MR as a
> > field (rather than a pointer) of another structure, so that's exactly what
> > we're looking for: we don't want to free() the MR as it's allocated
> > together with the owner device.  That'll be released when the owner free().
> > 
> > When dynamic allocation gets into the picture for MR, it's more complicated
> > for sure, because it means the user (like VFIOQuirk) will need to manually
> > allocate the MRs, then it requires explicit object_unparent() to detach
> > that from the device / owner when finalize().  NOTE!  object_unparent()
> > will NOT free the MR yet so far.  The MR still need to be manually freed
> > with an explicit g_free(), normally.  Again, I'd suggest you refer to the
> > VFIOQuirk code just as an example.  In that case this part is done with
> > e.g. vfio_bar_quirk_finalize().
> > 
> >          for (i = 0; i < quirk->nr_mem; i++) {
> >              object_unparent(OBJECT(&quirk->mem[i]));
> >          }
> >          g_free(quirk->mem);
> > 
> > Here quirk->mem is a pointer to an array of MR which can contain one or
> > more MRs, but the idea is the same.
> > 
> > > 
> > > I have an impression with QEMU code base that it is failing manual 
> > > resource
> > > deallocation so frequently although such deallocation can be easily
> > > automated by binding resources to objects and free them when objects die 
> > > by
> > > providing a function like Linux's devm_kmalloc(). Unfortunately I haven't
> > > found time to do that though.
> > 
> > AFAICT, the property list is exactly what you're saying.  IIUC normally an
> > object will be properly finalized()ed + free()ed when the parent object is
> > finalize()ed.  Here MR is just special as it bypasses all the free() part.
> > 
> > > 
> > > So my opinion here is 1) we should automate resource deallocation as much 
> > > as
> > > possible but 2) we shouldn't disturb code that performs manual resource
> > > management.
> > 
> > Agree.  However note again that in whatever case cross-device MR links will
> > still require explicit detachment or it's prone to memory leak.
> 
> I am not sure what you refer to with cross-device MR links so can you
> clarify it?

I was referring to Peter Maydell's example, where the MR can be used
outside of its owner.  In that case manual operation is a must before
finalize(), as finalize() can only make sense to resolve internal links
automatically.

But now knowing that you're explicitly mentioning "deallocation" rather
than "finalize()", and you're aware of diff between deallocation
v.s. finalize(), I suppose I misunderstood what you meant, and now I'm not
sure I get what you're suggesting.

> 
> > 
> > > 
> > > instance_finalize() is for manual resource management. It is better to 
> > > have
> > > less code in instance_finalize() and fortunately MemoryRegion don't 
> > > require
> > > any code in instance_finalize() in most cases. If instance_finalize() 
> > > still
> > > insists to call object_unparent(), we shouldn't prevent that. (I changed 
> > > my
> > > mind regarding this particular case of object_unparent() however as I
> > > describe below.)
> > > 
> > > > 
> > > > > 
> > > > > object_unparent() is such an example. instance_finalize() of a device 
> > > > > can
> > > > > call object_unparent() for a subregion and for its container. If we
> > > > > automatically finalize the container when calling object_unparent() 
> > > > > for the
> > > > > subregion, calling object_unparent() for its container will result in 
> > > > > the
> > > > > second finalization, which is not good.
> > > > 
> > > > IMHO we don't finalize the container at all - what I suggested was we 
> > > > call
> > > > del_subregion() for the case where container != NULL.  Since in this 
> > > > case
> > > > both container & mr belong to the same owner, it shouldn't change any
> > > > refcount, but only remove the link.
> > > > 
> > > > However I think I see what you pointed out.  I wonder why we remove all
> > > > properties now before reaching instance_finalze(): shouldn't finalize() 
> > > > be
> > > > allowed to access some of the properties?
> > > > 
> > > > It goes back to this commit:
> > > > 
> > > > commit 76a6e1cc7cc3ad022e7159b37b291b75bc4615bf
> > > > Author: Paolo Bonzini <pbonzini@redhat.com>
> > > > Date:   Wed Jun 11 11:58:30 2014 +0200
> > > > 
> > > >       qom: object: delete properties before calling instance_finalize
> > > >       This ensures that the children's unparent callback will still
> > > >       have a usable parent.
> > > >       Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> > > >       Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > > > 
> > > >   From this series (as the 1st patch there):
> > > > 
> > > > https://lore.kernel.org/qemu-devel/1406716032-21795-1-git-send-email-pbonzini@redhat.com/
> > > > 
> > > > I can't say I fully understand the commit yet so far.. because it seems
> > > > this patch was trying to pave way so that MR's unparent() can have a 
> > > > usable
> > > > parent.  However... I don't think mr implemented unparent() at all..
> > > > while it just sounds more reasonable that properties shouldn't be
> > > > auto-removed before calling instance_finalize() from my gut feeling.
> > > > 
> > > > I tried to revert 76a6e1cc7c ("qom: object: delete properties before
> > > > calling instance_finalize"), "make check" all passes here.  I am a bit
> > > > confused on where it applies, and whether we should revert it.
> > > > 
> > > > If with 76a6e1cc7cc reverted, I think your concern should go away 
> > > > because
> > > > then properties (including MRs) will only be detached after owner's
> > > > instance_finalize().  Again, I wished Paolo could chime in as he should
> > > > know the best.
> > > 
> > > I didn't know QOM deletes properties before instance_finalize().
> > > 
> > > I think it is a bad idea to delete properties before instance_finalize().
> > > The intention is that to keep a usable parent during unparenting, but it 
> > > is
> > > inevitable that an object is in a semi-finalized state during 
> > > finalization.
> > > If the order of finalization matters, it should be explicitly described in
> > > instance_finalize(). Deleting properties before instance_finalize() 
> > > prevents
> > > that.
> > > 
> > > That said, I think it is too late to revert that change. "make check" 
> > > works
> > > as a preliminary check, but ultimately we need manual tree-wide review,
> > > which is too costly.
> > 
> > I don't think it's too late. :)
> > 
> > IMHO if that statement is true, then QEMU will gradually become not
> > maintainable anymore with tons of such code nobody understands and nobody
> > can touch.  To me it can destine its death soon afterwards if so, sooner or
> > later.
> > 
> > OTOH, it'll be harder to justify or change indeed if some piece of code
> > stays longer in-tree.  So I agree with you even if we want to change
> > anything around this we should be with extreme cautions, and I don't think
> > we should merge anything like this too late of a release, just to give it
> > time to expose and break things.
> > 
> > > 
> > > Going back to the original topic, I had the (incorrect) assumption that 
> > > QOM
> > > deletes properties *after* instance_finalize(). Under such an assumption, 
> > > it
> > 
> > Let's stick with this model; so far I still think this is the right thing
> > to do, and I'm happy to know that you seem to at least agree (irrelevant of
> > whether we should move on with a change).
> > 
> > > would be unsafe to remove a subregion from its container when a subregion 
> > > is
> > > gone. As you said, we don't have to object_unparent(mr->container) and
> > 
> > IMHO we need to do this. Please recheck this after reading above on
> > finalize() v.s. free().  Here object_unparent() is needed because the MR is
> > allocated by the owner, it means only the owner knows how to free() it
> > (again: MR's own free()==NULL).  Then if we want to free it properly, we
> > need to detach it from the owner object first, hence object_unparent().
> > 
> > > instead we can just call memory_region_del_subregion() instead to keep
> > > object_unparent() functional. However, strictly speaking,
> > 
> > I think we can do that, or we don't.  It's the same as whether we want to
> > explicitly detach links for an embeded MR: if it's not explicitly done, we
> > can rely on the finalize() of the MR to do so. >
> > In all cases, object_unparent() will still be needed because it will do
> > more things than memory_region_del_subregion(): it removes the property
> > link itself.
> > 
> > This is also one reason why I keep thinking what I suggested might be good:
> > it covers not only mr->subregions and also mr->container, it means the
> > detachment has no ordering constraints on which mr is finalize()ed first
> > (in this case, either the container mr or the child mr).
> 
> I referred to the following statement you made by saying we don't have to
> object_unparent(mr->container):
> > IMHO we don't finalize the container at all - what I suggested was we
> > call del_subregion() for the case where container != NULL.  Since in
> > this case both container & mr belong to the same owner, it shouldn't
> > change any refcount, but only remove the link.
> 
> I discussed calling del_subregion() is still problematic below.
> 
> Regards,
> Akihiko Odaki
> 
> > 
> > > memory_region_del_subregion() is also a side effect and may affect
> > > operations with the container later. Such a side effect is better to be
> > > avoided whenever possible.
> > > 
> > > My point in this discussion is that the device has a control of its memory
> > > region even during finalization. We shouldn't call object_unparent(),

I assume we're talking about embeded MRs, then I agree until here.

> > > memory_region_del_subregion(), or perform whatever operation on another
> > > memory region; otherwise the device may see an unexpected side effect.

I don't get it here.

We're still taking in the context of calling finalize() before property
list being detached, right?

In that case detachment of the property list should be the last thing we
do.  What unexpected side effect do we worry about?

To be explicit, IMHO the right sequence of doing it is (which remove
property later):

        static void object_finalize(void *data)
        {
                Object *obj = data;
                TypeImpl *ti = obj->class->type;

                object_deinit(obj, ti);
                object_property_del_all(obj); <------------ [1]

                g_assert(obj->ref == 0);
                g_assert(obj->parent == NULL);
                if (obj->free) {
                        obj->free(obj);
                }
        }

We're talking about MR automatically unlink both mr->container and
mr->subregions at [1] (comparing to the old world where we only auto detach
mr->subregions).  After that it calls obj->free() and memory is freed.
I don't see what can be the side effect you mentioned.

The only side effect I can think of is when the container MR finalize() is
called later than the subregion's, then the container will see empty
mr->subregions on its own, but that's exactly what we want to work out
here: we want to make sure mr finalize() work in random order.

> > > 
> > > Now, let's consider the correct assumption that QOM deletes properties
> > > *before* instance_finalize(). Under this assumption, the statement in
> > > docs/devel/memory.rst saying that calling object_unparent() in
> > > instance_finalize() is fine is wrong because the memory region is already
> > > finalized and we shouldn't call object_unparent() for a finalized object.
> > > 
> > > This also means a device does not have a control of its memory regions
> > > during finalization and the device will not see the side effect of calling
> > > memory_region_del_subregion(). However, if a device really needs to 
> > > control
> > > the order of memory region finalization, it can still call object_ref() 
> > > for
> > > memory regions to keep them alive until instance_finalize(). The effect of
> > > memory_region_del_subregion() will be  visible to the device if a device
> > > does such a trick. While it is very unlikely that any device does such a
> > > trick, it is still better to avoid having any assumption on devices.
> > > 
> > > With the proposed object_ref(mr->owner == subregion->owner ?
> > > OBJECT(subregion) : subregion->owner) call, we don't have to make any
> > > assumption on how finalization works, which is too complicated as this
> > > discussion shows. It certainly makes the use of two reference counters, 
> > > but
> > > it does not deviate from the current policy on their usage and it is also
> > > trivial to adapt when the reference counter in memory region gets removed.
> > > Considering all above, I believe my patch is the most straightforward
> > > solution.
> > 
> > Let's see how you thinks after you read above first.  I hope we can get
> > more onto the same page at least on the context.
> > 
> > So far I still prefer using mr->refcount as less as possible.  Now it plays
> > the only role as "whether this MR is put onto an owner property list", and
> > for all the rest refcounts on MR it should always routes to the owner.  I
> > still think we need to be extremely cautious on further complicating this
> > refcount.  It's already complicated indeed, but hopefully this model is the
> > best we can trivially have right now, and so far it's clear to me, but it's
> > always possible I overlooked something.
> 

-- 
Peter Xu