Re: [PATCH v6 2/2] memory: Do not create circular reference with subregion

[Peter Xu <peterx@redhat.com>]

On Sun, Jan 05, 2025 at 05:56:19PM +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 78e17e0efa83..16b051249c5f 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -2644,7 +2644,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);

Would you mind we make this slightly more readable?  E.g. introduce
memory_region_[un]ref_subregion(), with something like:

memory_region_ref_subregion(MemoryRegion *parent, MemoryRegion *child)
{
        if (parent->owner == child->owner) {
                /*
                 * Avoid possible circular ref on the owner object,
                 * fallback to use MR's own object refcount.
                 */
                object_ref(child);
        } else {
                memory_region_ref(child);
        }
}

So that we at least don't open code memory_region_ref(), meanwhile when
someone wants to grep memory_region_ref* these helpers will be obvious.

> +
>      QTAILQ_FOREACH(other, &mr->subregions, subregions_link) {
>          if (subregion->priority >= other->priority) {
>              QTAILQ_INSERT_BEFORE(other, subregion, subregions_link);
> @@ -2702,7 +2704,9 @@ void memory_region_del_subregion(MemoryRegion *mr,
>          assert(alias->mapped_via_alias >= 0);
>      }
>      QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
> -    memory_region_unref(subregion);
> +    object_unref(mr->owner == subregion->owner ?
> +                 OBJECT(subregion) : subregion->owner);
> +
>      memory_region_update_pending |= mr->enabled && subregion->enabled;
>      memory_region_transaction_commit();
>  }
> 
> -- 
> 2.47.1
> 

-- 
Peter Xu