Message ID | 20250105-san-v6-2-11fc859b99b7@daynix.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Fix check-qtest-ppc64 sanitizer errors | expand |
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 >
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); + 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(); }
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(-)