On Tue, 2025-08-26 at 18:17 -0400, Peter Xu wrote:
> Currently, QEMU refcounts the MR by always taking it from the owner.
>
> It's common that one object will have multiple MR objects embeded in the
> object itself. All the MRs in this case share the same lifespan of the
> owner object.
>
> It's also common that in the instance_init() of an object, MR A can be a
> container of MR B, C, D, by using memory_region_add_subregion*() set of
> memory region APIs.
>
> Now we have a circular reference issue, as when adding subregions for MR A,
> we essentially incremented the owner's refcount within the instance_init(),
> meaning the object will be self-boosted and its refcount can never go down
> to zero if the MRs won't get detached properly before object's finalize().
>
> Delete subregions within object's finalize() won't work either, because
> finalize() will be invoked only if the refcount goes to zero first. What
> is worse, object_finalize() will do object_property_del_all() first before
> object_deinit(). Since embeded MRs will be properties of the owner object,
> it means they'll be freed _before_ the owner's finalize().
>
> To fix that, teach memory API to stop refcount on MRs that share the same
> owner. Because if they share the lifecycle of the owner, then they share
> the same lifecycle between themselves, hence the refcount doesn't help but
> only introduce troubles.
>
> Meanwhile, allow auto-detachments of MRs during finalize() of MRs even
> against its container, as long as they belong to the same owner.
>
> The latter is needed because now it's possible to have MRs' finalize()
> happen in any order when they share the same lifespan with a same owner.
> In this case, we should allow finalize() to happen in any order of either
> the parent or child MR. Loose the mr->container check in MR's finalize()
> to allow auto-detach. Double check it shares the same owner.
>
> Proper document this behavior in code.
>
> This patch is heavily based on the work done by Akihiko Odaki:
>
> [https://lore.kernel.org/r/cafeaca8dv40fgsci76r4yep1p-sp_qjnrdd2ozpxjx5wrs0...@mail.gmail.com](https://lore.kernel.org/r/cafeaca8dv40fgsci76r4yep1p-sp_qjnrdd2ozpxjx5wrs0...@mail.gmail.com)
>
> Signed-off-by: Peter Xu <[pet...@redhat.com](mailto:pet...@redhat.com)>
> ---
> docs/devel/memory.rst | 7 +++++--
> system/memory.c | 45 ++++++++++++++++++++++++++++++++++---------
> 2 files changed, 41 insertions(+), 11 deletions(-)
>
> diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
> index 57fb2aec76..a325e97d7b 100644
> --- a/docs/devel/memory.rst
> +++ b/docs/devel/memory.rst
> @@ -158,8 +158,11 @@ ioeventfd) can be changed during the region lifecycle.
> They take effect
> as soon as the region is made visible. This can be immediately, later,
> or never.
>
> -Destruction of a memory region happens automatically when the owner
> -object dies.
> +Destruction of a memory region happens automatically when the owner object
> +dies. When there are multiple memory regions under the same owner object,
> +the memory API will guarantee all memory regions will be properly detached
> +and finalized one by one. The order which memory region will be finalized
> +first is not guaranteed.
>
> If however the memory region is part of a dynamically allocated data
> structure, you should call object_unparent() to destroy the memory region
> diff --git a/system/memory.c b/system/memory.c
> index 5646547940..d7f6ad9be2 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -1796,16 +1796,36 @@ static void memory_region_finalize(Object *obj)
> {
> MemoryRegion *mr = MEMORY_REGION(obj);
>
> - assert(!mr->container);
> -
> - /* We know the region is not visible in any address space (it
> - * does not have a container and cannot be a root either because
> - * it has no references, so we can blindly clear mr->enabled.
> - * memory_region_set_enabled instead could trigger a transaction
> - * and cause an infinite loop.
> + /*
> + * Each memory region (that can be dynamically freed..) must has an
"must have" or "has"
> + * owner, and it always has the same lifecycle of its owner. It means
> + * when reaching here, the memory region's owner refcount is zero.
> + *
> + * Here it is possible that the MR has:
> + *
> + * (1) mr->container set, which means this MR can be a subregion of a
> + * container MR, in this case it must share the same owner
s/it/they ?
> + * (otherwise the container should have kept a refcount of this
> + * MR's owner), or,
> + *
> + * (2) mr->subregions non-empty, which means this MR can be a container
> + * of other MRs (share the owner or not).
> + *
> + * We know the MR, or any MR that is attached to this one as either
> + * container or children, is not visible in any address space, because
> + * otherwise the address space should have taken at least one refcount
> + * of this MR's owner. So we can blindly clear mr->enabled.
> + *
> + * memory_region_set_enabled instead could trigger a transaction and
> + * cause an infinite loop.
> */
> mr->enabled = false;
> memory_region_transaction_begin();
> + if (mr->container) {
> + /* Must share the owner; see above comments */
> + assert(mr->container->owner == mr->owner);
> + memory_region_del_subregion(mr->container, mr);
> + }
> while (!QTAILQ_EMPTY(&mr->subregions)) {
> MemoryRegion *subregion = QTAILQ_FIRST(&mr->subregions);
> memory_region_del_subregion(mr, subregion);
> @@ -2625,7 +2645,10 @@ static void
> memory_region_update_container_subregions(MemoryRegion *subregion)
>
> memory_region_transaction_begin();
>
> - memory_region_ref(subregion);
> + if (mr->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);
> @@ -2683,7 +2706,11 @@ void memory_region_del_subregion(MemoryRegion *mr,
> assert(alias->mapped_via_alias >= 0);
> }
> QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
> - memory_region_unref(subregion);
> +
> + if (mr->owner != subregion->owner) {
> + memory_region_unref(subregion);
> + }
> +
> memory_region_update_pending |= mr->enabled && subregion->enabled;
> memory_region_transaction_commit();
> }
Reviewed-by: Clément Mathieu--Drif <clement.mathieu--d...@eviden.com>
