diff options
| -rw-r--r-- | docs/devel/memory.rst | 7 | ||||
| -rw-r--r-- | system/memory.c | 46 |
2 files changed, 42 insertions, 11 deletions
diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst index 57fb2aec76..42d3ca29c4 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 in which memory regions will be +finalized 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 44701c465c..cf8cad6961 100644 --- a/system/memory.c +++ b/system/memory.c @@ -1796,16 +1796,37 @@ 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 freed) must have an owner, and it + * always has the same lifecycle of its owner. It means when reaching + * here, the memory region's owner's refcount is zero. + * + * Here it is possible that the MR has: + * + * (1) mr->container set, which means this MR is a subregion of a + * container MR. In this case they must share the same owner as the + * container (otherwise the container should have kept a refcount + * of this MR's owner). + * + * (2) mr->subregions non-empty, which means this MR is a container of + * one or more other MRs (which might have the the owner as this + * MR, or a different owner). + * + * 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); @@ -2640,7 +2661,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); @@ -2698,7 +2722,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(); } |