| Message ID | 20240627-san-v2-9-750bb0946dbd@daynix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix check-qtest-ppc64 sanitizer errors | expand |
On Thu, Jun 27, 2024 at 10:37:52PM +0900, Akihiko Odaki wrote: > A memory region does not use their own reference counters, but instead > piggybacks on another QOM object, "owner" (unless the owner is not the > memory region itself). When creating a subregion, a new reference to the > owner of the container must be created. However, if the subregion is > owned by the same QOM object, this result in a self-reference, and make > the owner immortal. Avoid such a self-reference. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > system/memory.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/system/memory.c b/system/memory.c > index 74cd73ebc78b..949f5016a68d 100644 > --- a/system/memory.c > +++ b/system/memory.c > @@ -2638,7 +2638,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); > @@ -2696,7 +2699,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(); > } This does look like a real issue.. the patch looks reasonable to me, but I wonder whether we should start to add some good comments in code to reflect that complexity starting from this one. The MR refcount isn't easy to understand to me. It also lets me start to wonder how MR refcount went through until it looks like today.. It's definitely not extremely intuitive to use mr->owner as the object to do refcounting if mr itself does has its own QObject, meanwhile it has other tricks around. E.g. the first thing I stumbled over when looking was the optimization where we will avoid refcounting the mr when there's no owner, and IIUC it was for the case when the "guest memory" (which will never be freed) used to have no owner so we can speedup DMA if we know it won't go away. https://lore.kernel.org/qemu-devel/1450263601-2828-5-git-send-email-pbonzini@redhat.com/ commit 612263cf33062f7441a5d0e3b37c65991fdc3210 Author: Paolo Bonzini <pbonzini@redhat.com> Date: Wed Dec 9 11:44:25 2015 +0100 memory: avoid unnecessary object_ref/unref For the common case of DMA into non-hotplugged RAM, it is unnecessary but expensive to do object_ref/unref. Add back an owner field to MemoryRegion, so that these memory regions can skip the reference counting. If so, it looks like it will stop working with memory-backends get involved? As I think those MRs will have owner set always, and I wonder whether memory-backends should be the major way to specify guest memory now and in the future. So I'm not sure how important that optimization is as of now, and whether we could "simplify" it back to always do the refcount if the major scenarios will not adopt it. The other issue is we used owner refcount from the start of memory_region_ref() got introduced, since: commit 46637be269aaaceb9867ffdf176e906401138fff Author: Paolo Bonzini <pbonzini@redhat.com> Date: Tue May 7 09:06:00 2013 +0200 memory: add ref/unref And we still have that in our document, even though I don't think it's true anymore: * ... MemoryRegions actually do not have their * own reference count; they piggyback on a QOM object, their "owner". * This function adds a reference to the owner. It looks like what happened is when introduced the change, MR is not a QOM object yet. But it later is.. I mentioned all these only because I found that _if_ we can keep mr refcounting as simple as other objects: memory_region_ref(mr) { object_ref(OBJECT(mr)); } Then looks like this "recursive refcount" problem can also go away. I'm curious whether you or anyone tried to explore that path, or whether above doesn't make sense at all. Thanks,
On 2024/07/03 2:44, Peter Xu wrote: > On Thu, Jun 27, 2024 at 10:37:52PM +0900, Akihiko Odaki wrote: >> A memory region does not use their own reference counters, but instead >> piggybacks on another QOM object, "owner" (unless the owner is not the >> memory region itself). When creating a subregion, a new reference to the >> owner of the container must be created. However, if the subregion is >> owned by the same QOM object, this result in a self-reference, and make >> the owner immortal. Avoid such a self-reference. >> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >> --- >> system/memory.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/system/memory.c b/system/memory.c >> index 74cd73ebc78b..949f5016a68d 100644 >> --- a/system/memory.c >> +++ b/system/memory.c >> @@ -2638,7 +2638,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); >> @@ -2696,7 +2699,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(); >> } > > This does look like a real issue.. the patch looks reasonable to me, but I > wonder whether we should start to add some good comments in code to reflect > that complexity starting from this one. The MR refcount isn't easy to > understand to me. > > It also lets me start to wonder how MR refcount went through until it looks > like today.. It's definitely not extremely intuitive to use mr->owner as > the object to do refcounting if mr itself does has its own QObject, > meanwhile it has other tricks around. > > E.g. the first thing I stumbled over when looking was the optimization > where we will avoid refcounting the mr when there's no owner, and IIUC it > was for the case when the "guest memory" (which will never be freed) used > to have no owner so we can speedup DMA if we know it won't go away. > > https://lore.kernel.org/qemu-devel/1450263601-2828-5-git-send-email-pbonzini@redhat.com/ > > commit 612263cf33062f7441a5d0e3b37c65991fdc3210 > Author: Paolo Bonzini <pbonzini@redhat.com> > Date: Wed Dec 9 11:44:25 2015 +0100 > > memory: avoid unnecessary object_ref/unref > > For the common case of DMA into non-hotplugged RAM, it is unnecessary > but expensive to do object_ref/unref. Add back an owner field to > MemoryRegion, so that these memory regions can skip the reference > counting. > > If so, it looks like it will stop working with memory-backends get > involved? As I think those MRs will have owner set always, and I wonder > whether memory-backends should be the major way to specify guest memory now > and in the future. So I'm not sure how important that optimization is as > of now, and whether we could "simplify" it back to always do the refcount > if the major scenarios will not adopt it. > > The other issue is we used owner refcount from the start of > memory_region_ref() got introduced, since: > > commit 46637be269aaaceb9867ffdf176e906401138fff > Author: Paolo Bonzini <pbonzini@redhat.com> > Date: Tue May 7 09:06:00 2013 +0200 > > memory: add ref/unref > > And we still have that in our document, even though I don't think it's true > anymore: > > * ... MemoryRegions actually do not have their > * own reference count; they piggyback on a QOM object, their "owner". > * This function adds a reference to the owner. > > It looks like what happened is when introduced the change, MR is not a QOM > object yet. But it later is.. > > I mentioned all these only because I found that _if_ we can keep mr > refcounting as simple as other objects: > > memory_region_ref(mr) > { > object_ref(OBJECT(mr)); > } > > Then looks like this "recursive refcount" problem can also go away. I'm > curious whether you or anyone tried to explore that path, or whether above > doesn't make sense at all. It unfortunately does not solve the problem. The underlying problem is that the whole device must be kept alive while its memory region are. Indeed MemoryRegions do have refcounts, but incrementing them do not extend the lifetime of the devices (i.e., the owners). The refcount of the owners must be incremented for correctness. Referencing a subregion MemoryRegion from its container MemoryRegion owned by the same device is an exceptional case. Incrementing the refcount of the owner extends the owner's lifetime to forever. Regards, Akihiko Odaki
On 6/7/24 13:59, Akihiko Odaki wrote: > On 2024/07/03 2:44, Peter Xu wrote: >> On Thu, Jun 27, 2024 at 10:37:52PM +0900, Akihiko Odaki wrote: >>> A memory region does not use their own reference counters, but instead >>> piggybacks on another QOM object, "owner" (unless the owner is not the >>> memory region itself). When creating a subregion, a new reference to the >>> owner of the container must be created. However, if the subregion is >>> owned by the same QOM object, this result in a self-reference, and make >>> the owner immortal. Avoid such a self-reference. >>> >>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>> --- >>> system/memory.c | 11 +++++++++-- >>> 1 file changed, 9 insertions(+), 2 deletions(-) >>> >>> diff --git a/system/memory.c b/system/memory.c >>> index 74cd73ebc78b..949f5016a68d 100644 >>> --- a/system/memory.c >>> +++ b/system/memory.c >>> @@ -2638,7 +2638,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); >>> @@ -2696,7 +2699,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(); >>> } >> >> This does look like a real issue.. the patch looks reasonable to me, >> but I >> wonder whether we should start to add some good comments in code to >> reflect >> that complexity starting from this one. The MR refcount isn't easy to >> understand to me. >> >> It also lets me start to wonder how MR refcount went through until it >> looks >> like today.. It's definitely not extremely intuitive to use mr->owner as >> the object to do refcounting if mr itself does has its own QObject, >> meanwhile it has other tricks around. >> >> E.g. the first thing I stumbled over when looking was the optimization >> where we will avoid refcounting the mr when there's no owner, and IIUC it >> was for the case when the "guest memory" (which will never be freed) used >> to have no owner so we can speedup DMA if we know it won't go away. >> >> https://lore.kernel.org/qemu-devel/1450263601-2828-5-git-send-email-pbonzini@redhat.com/ >> >> commit 612263cf33062f7441a5d0e3b37c65991fdc3210 >> Author: Paolo Bonzini <pbonzini@redhat.com> >> Date: Wed Dec 9 11:44:25 2015 +0100 >> >> memory: avoid unnecessary object_ref/unref >> For the common case of DMA into non-hotplugged RAM, it is >> unnecessary >> but expensive to do object_ref/unref. Add back an owner field to >> MemoryRegion, so that these memory regions can skip the reference >> counting. >> >> If so, it looks like it will stop working with memory-backends get >> involved? As I think those MRs will have owner set always, and I wonder >> whether memory-backends should be the major way to specify guest >> memory now >> and in the future. So I'm not sure how important that optimization is as >> of now, and whether we could "simplify" it back to always do the refcount >> if the major scenarios will not adopt it. >> >> The other issue is we used owner refcount from the start of >> memory_region_ref() got introduced, since: >> >> commit 46637be269aaaceb9867ffdf176e906401138fff >> Author: Paolo Bonzini <pbonzini@redhat.com> >> Date: Tue May 7 09:06:00 2013 +0200 >> >> memory: add ref/unref >> >> And we still have that in our document, even though I don't think it's >> true >> anymore: >> >> * ... MemoryRegions actually do not have their >> * own reference count; they piggyback on a QOM object, their "owner". >> * This function adds a reference to the owner. >> >> It looks like what happened is when introduced the change, MR is not a >> QOM >> object yet. But it later is.. >> >> I mentioned all these only because I found that _if_ we can keep mr >> refcounting as simple as other objects: >> >> memory_region_ref(mr) >> { >> object_ref(OBJECT(mr)); >> } >> >> Then looks like this "recursive refcount" problem can also go away. I'm >> curious whether you or anyone tried to explore that path, or whether >> above >> doesn't make sense at all. > > It unfortunately does not solve the problem. > > The underlying problem is that the whole device must be kept alive while > its memory region are. Indeed MemoryRegions do have refcounts, but > incrementing them do not extend the lifetime of the devices (i.e., the > owners). The refcount of the owners must be incremented for correctness. > > Referencing a subregion MemoryRegion from its container MemoryRegion > owned by the same device is an exceptional case. Incrementing the > refcount of the owner extends the owner's lifetime to forever. Is it really an exceptional case? What I'm seeing are a lot of devices creating MR and never bother to destroy them, so indeed owner (device) refcount never reaches 0. Most of the time when we create MR in .instance_init/.realize, we neglect to implement the undo path (.instance_finalize or .unrealize).
On 2024/07/08 17:06, Philippe Mathieu-Daudé wrote: > On 6/7/24 13:59, Akihiko Odaki wrote: >> On 2024/07/03 2:44, Peter Xu wrote: >>> On Thu, Jun 27, 2024 at 10:37:52PM +0900, Akihiko Odaki wrote: >>>> A memory region does not use their own reference counters, but instead >>>> piggybacks on another QOM object, "owner" (unless the owner is not the >>>> memory region itself). When creating a subregion, a new reference to >>>> the >>>> owner of the container must be created. However, if the subregion is >>>> owned by the same QOM object, this result in a self-reference, and make >>>> the owner immortal. Avoid such a self-reference. >>>> >>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>>> --- >>>> system/memory.c | 11 +++++++++-- >>>> 1 file changed, 9 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/system/memory.c b/system/memory.c >>>> index 74cd73ebc78b..949f5016a68d 100644 >>>> --- a/system/memory.c >>>> +++ b/system/memory.c >>>> @@ -2638,7 +2638,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); >>>> @@ -2696,7 +2699,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(); >>>> } >>> >>> This does look like a real issue.. the patch looks reasonable to me, >>> but I >>> wonder whether we should start to add some good comments in code to >>> reflect >>> that complexity starting from this one. The MR refcount isn't easy to >>> understand to me. >>> >>> It also lets me start to wonder how MR refcount went through until it >>> looks >>> like today.. It's definitely not extremely intuitive to use >>> mr->owner as >>> the object to do refcounting if mr itself does has its own QObject, >>> meanwhile it has other tricks around. >>> >>> E.g. the first thing I stumbled over when looking was the optimization >>> where we will avoid refcounting the mr when there's no owner, and >>> IIUC it >>> was for the case when the "guest memory" (which will never be freed) >>> used >>> to have no owner so we can speedup DMA if we know it won't go away. >>> >>> https://lore.kernel.org/qemu-devel/1450263601-2828-5-git-send-email-pbonzini@redhat.com/ >>> >>> commit 612263cf33062f7441a5d0e3b37c65991fdc3210 >>> Author: Paolo Bonzini <pbonzini@redhat.com> >>> Date: Wed Dec 9 11:44:25 2015 +0100 >>> >>> memory: avoid unnecessary object_ref/unref >>> For the common case of DMA into non-hotplugged RAM, it is >>> unnecessary >>> but expensive to do object_ref/unref. Add back an owner field to >>> MemoryRegion, so that these memory regions can skip the reference >>> counting. >>> >>> If so, it looks like it will stop working with memory-backends get >>> involved? As I think those MRs will have owner set always, and I wonder >>> whether memory-backends should be the major way to specify guest >>> memory now >>> and in the future. So I'm not sure how important that optimization >>> is as >>> of now, and whether we could "simplify" it back to always do the >>> refcount >>> if the major scenarios will not adopt it. >>> >>> The other issue is we used owner refcount from the start of >>> memory_region_ref() got introduced, since: >>> >>> commit 46637be269aaaceb9867ffdf176e906401138fff >>> Author: Paolo Bonzini <pbonzini@redhat.com> >>> Date: Tue May 7 09:06:00 2013 +0200 >>> >>> memory: add ref/unref >>> >>> And we still have that in our document, even though I don't think >>> it's true >>> anymore: >>> >>> * ... MemoryRegions actually do not have their >>> * own reference count; they piggyback on a QOM object, their "owner". >>> * This function adds a reference to the owner. >>> >>> It looks like what happened is when introduced the change, MR is not >>> a QOM >>> object yet. But it later is.. >>> >>> I mentioned all these only because I found that _if_ we can keep mr >>> refcounting as simple as other objects: >>> >>> memory_region_ref(mr) >>> { >>> object_ref(OBJECT(mr)); >>> } >>> >>> Then looks like this "recursive refcount" problem can also go away. I'm >>> curious whether you or anyone tried to explore that path, or whether >>> above >>> doesn't make sense at all. >> >> It unfortunately does not solve the problem. >> >> The underlying problem is that the whole device must be kept alive >> while its memory region are. Indeed MemoryRegions do have refcounts, >> but incrementing them do not extend the lifetime of the devices (i.e., >> the owners). The refcount of the owners must be incremented for >> correctness. >> >> Referencing a subregion MemoryRegion from its container MemoryRegion >> owned by the same device is an exceptional case. Incrementing the >> refcount of the owner extends the owner's lifetime to forever. > > Is it really an exceptional case? > > What I'm seeing are a lot of devices creating MR and never bother > to destroy them, so indeed owner (device) refcount never reaches 0. > > Most of the time when we create MR in .instance_init/.realize, > we neglect to implement the undo path (.instance_finalize or > .unrealize). I meant memory_region_update_container_subregions() is the only code that creates such a reference. The function itself is called in many code paths.
On Mon, Jul 08, 2024 at 10:06:44AM +0200, Philippe Mathieu-Daudé wrote: > On 6/7/24 13:59, Akihiko Odaki wrote: > > On 2024/07/03 2:44, Peter Xu wrote: > > > On Thu, Jun 27, 2024 at 10:37:52PM +0900, Akihiko Odaki wrote: > > > > A memory region does not use their own reference counters, but instead > > > > piggybacks on another QOM object, "owner" (unless the owner is not the > > > > memory region itself). When creating a subregion, a new reference to the > > > > owner of the container must be created. However, if the subregion is > > > > owned by the same QOM object, this result in a self-reference, and make > > > > the owner immortal. Avoid such a self-reference. > > > > > > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > > > --- > > > > system/memory.c | 11 +++++++++-- > > > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/system/memory.c b/system/memory.c > > > > index 74cd73ebc78b..949f5016a68d 100644 > > > > --- a/system/memory.c > > > > +++ b/system/memory.c > > > > @@ -2638,7 +2638,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); > > > > @@ -2696,7 +2699,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(); > > > > } > > > > > > This does look like a real issue.. the patch looks reasonable to me, > > > but I > > > wonder whether we should start to add some good comments in code to > > > reflect > > > that complexity starting from this one. The MR refcount isn't easy to > > > understand to me. > > > > > > It also lets me start to wonder how MR refcount went through until > > > it looks > > > like today.. It's definitely not extremely intuitive to use mr->owner as > > > the object to do refcounting if mr itself does has its own QObject, > > > meanwhile it has other tricks around. > > > > > > E.g. the first thing I stumbled over when looking was the optimization > > > where we will avoid refcounting the mr when there's no owner, and IIUC it > > > was for the case when the "guest memory" (which will never be freed) used > > > to have no owner so we can speedup DMA if we know it won't go away. > > > > > > https://lore.kernel.org/qemu-devel/1450263601-2828-5-git-send-email-pbonzini@redhat.com/ > > > > > > commit 612263cf33062f7441a5d0e3b37c65991fdc3210 > > > Author: Paolo Bonzini <pbonzini@redhat.com> > > > Date: Wed Dec 9 11:44:25 2015 +0100 > > > > > > memory: avoid unnecessary object_ref/unref > > > For the common case of DMA into non-hotplugged RAM, it is > > > unnecessary > > > but expensive to do object_ref/unref. Add back an owner field to > > > MemoryRegion, so that these memory regions can skip the reference > > > counting. > > > > > > If so, it looks like it will stop working with memory-backends get > > > involved? As I think those MRs will have owner set always, and I wonder > > > whether memory-backends should be the major way to specify guest > > > memory now > > > and in the future. So I'm not sure how important that optimization is as > > > of now, and whether we could "simplify" it back to always do the refcount > > > if the major scenarios will not adopt it. > > > > > > The other issue is we used owner refcount from the start of > > > memory_region_ref() got introduced, since: > > > > > > commit 46637be269aaaceb9867ffdf176e906401138fff > > > Author: Paolo Bonzini <pbonzini@redhat.com> > > > Date: Tue May 7 09:06:00 2013 +0200 > > > > > > memory: add ref/unref > > > > > > And we still have that in our document, even though I don't think > > > it's true > > > anymore: > > > > > > * ... MemoryRegions actually do not have their > > > * own reference count; they piggyback on a QOM object, their "owner". > > > * This function adds a reference to the owner. > > > > > > It looks like what happened is when introduced the change, MR is not > > > a QOM > > > object yet. But it later is.. > > > > > > I mentioned all these only because I found that _if_ we can keep mr > > > refcounting as simple as other objects: > > > > > > memory_region_ref(mr) > > > { > > > object_ref(OBJECT(mr)); > > > } > > > > > > Then looks like this "recursive refcount" problem can also go away. I'm > > > curious whether you or anyone tried to explore that path, or whether > > > above > > > doesn't make sense at all. > > > > It unfortunately does not solve the problem. > > > > The underlying problem is that the whole device must be kept alive while > > its memory region are. Indeed MemoryRegions do have refcounts, but > > incrementing them do not extend the lifetime of the devices (i.e., the > > owners). The refcount of the owners must be incremented for correctness. > > > > Referencing a subregion MemoryRegion from its container MemoryRegion > > owned by the same device is an exceptional case. Incrementing the > > refcount of the owner extends the owner's lifetime to forever. > > Is it really an exceptional case? > > What I'm seeing are a lot of devices creating MR and never bother > to destroy them, so indeed owner (device) refcount never reaches 0. > > Most of the time when we create MR in .instance_init/.realize, > we neglect to implement the undo path (.instance_finalize or > .unrealize). Right. The cross-reference of MR <-> device makes sense from the concept level, but I'm not sure how much we rely on that from QEMU perspective currently. It complicates refcount and looks like it needs some thoughts. One proof is, when I replied I actually tested all qemu qtests with below patch and nothing yet explode: ===8<=== diff --git a/system/memory.c b/system/memory.c index 2d69521360..a1d2c1f808 100644 --- a/system/memory.c +++ b/system/memory.c @@ -1801,26 +1801,12 @@ Object *memory_region_owner(MemoryRegion *mr) void memory_region_ref(MemoryRegion *mr) { - /* MMIO callbacks most likely will access data that belongs - * to the owner, hence the need to ref/unref the owner whenever - * the memory region is in use. - * - * The memory region is a child of its owner. As long as the - * owner doesn't call unparent itself on the memory region, - * ref-ing the owner will also keep the memory region alive. - * Memory regions without an owner are supposed to never go away; - * we do not ref/unref them because it slows down DMA sensibly. - */ - if (mr && mr->owner) { - object_ref(mr->owner); - } + object_ref(OBJECT(mr)); } void memory_region_unref(MemoryRegion *mr) { - if (mr && mr->owner) { - object_unref(mr->owner); - } + object_unref(OBJECT(mr)); } ===8<=== I'm not sure how much it covers dynamic destruction of devices, though, where the device creates internal MRs to the guest address space. I'm actually wondering whether we have other barriers already to avoid having any MR ops being invoked if a device was removed. So I think above indeed would remove the cross-ref we used to have. It might be a matter of whether that's fine for now to fix the cyclic ref issue alone, and even if we still want some cross-ref.. whether we should do it the old way. I mean, the cross-ref can also be done in other forms, and it may not block MR has its own refcounts. Even if we want to have dev<->MR refcount, I wonder whether we should avoid having MR refcount the device, because it looks like "sub-object refs the parent" which sounds like a good source of cyclic refcount issue. I wonder whether device should ref the MR instead (when MR is dynamically created; I am not sure whether we even need that if in Phil's comment where the MR object is a sub-struct-field of the Device object). Then the device can provide a proper destroy() function to release the MRs under it, perhaps by disabling them first (then any ops should already fail after that), then unref the MRs with the hope that they're the last ref holder then MRs destroys there too. Thanks,
On 2024/07/09 1:40, Peter Xu wrote: > On Mon, Jul 08, 2024 at 10:06:44AM +0200, Philippe Mathieu-Daudé wrote: >> On 6/7/24 13:59, Akihiko Odaki wrote: >>> On 2024/07/03 2:44, Peter Xu wrote: >>>> On Thu, Jun 27, 2024 at 10:37:52PM +0900, Akihiko Odaki wrote: >>>>> A memory region does not use their own reference counters, but instead >>>>> piggybacks on another QOM object, "owner" (unless the owner is not the >>>>> memory region itself). When creating a subregion, a new reference to the >>>>> owner of the container must be created. However, if the subregion is >>>>> owned by the same QOM object, this result in a self-reference, and make >>>>> the owner immortal. Avoid such a self-reference. >>>>> >>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>>>> --- >>>>> system/memory.c | 11 +++++++++-- >>>>> 1 file changed, 9 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/system/memory.c b/system/memory.c >>>>> index 74cd73ebc78b..949f5016a68d 100644 >>>>> --- a/system/memory.c >>>>> +++ b/system/memory.c >>>>> @@ -2638,7 +2638,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); >>>>> @@ -2696,7 +2699,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(); >>>>> } >>>> >>>> This does look like a real issue.. the patch looks reasonable to me, >>>> but I >>>> wonder whether we should start to add some good comments in code to >>>> reflect >>>> that complexity starting from this one. The MR refcount isn't easy to >>>> understand to me. >>>> >>>> It also lets me start to wonder how MR refcount went through until >>>> it looks >>>> like today.. It's definitely not extremely intuitive to use mr->owner as >>>> the object to do refcounting if mr itself does has its own QObject, >>>> meanwhile it has other tricks around. >>>> >>>> E.g. the first thing I stumbled over when looking was the optimization >>>> where we will avoid refcounting the mr when there's no owner, and IIUC it >>>> was for the case when the "guest memory" (which will never be freed) used >>>> to have no owner so we can speedup DMA if we know it won't go away. >>>> >>>> https://lore.kernel.org/qemu-devel/1450263601-2828-5-git-send-email-pbonzini@redhat.com/ >>>> >>>> commit 612263cf33062f7441a5d0e3b37c65991fdc3210 >>>> Author: Paolo Bonzini <pbonzini@redhat.com> >>>> Date: Wed Dec 9 11:44:25 2015 +0100 >>>> >>>> memory: avoid unnecessary object_ref/unref >>>> For the common case of DMA into non-hotplugged RAM, it is >>>> unnecessary >>>> but expensive to do object_ref/unref. Add back an owner field to >>>> MemoryRegion, so that these memory regions can skip the reference >>>> counting. >>>> >>>> If so, it looks like it will stop working with memory-backends get >>>> involved? As I think those MRs will have owner set always, and I wonder >>>> whether memory-backends should be the major way to specify guest >>>> memory now >>>> and in the future. So I'm not sure how important that optimization is as >>>> of now, and whether we could "simplify" it back to always do the refcount >>>> if the major scenarios will not adopt it. >>>> >>>> The other issue is we used owner refcount from the start of >>>> memory_region_ref() got introduced, since: >>>> >>>> commit 46637be269aaaceb9867ffdf176e906401138fff >>>> Author: Paolo Bonzini <pbonzini@redhat.com> >>>> Date: Tue May 7 09:06:00 2013 +0200 >>>> >>>> memory: add ref/unref >>>> >>>> And we still have that in our document, even though I don't think >>>> it's true >>>> anymore: >>>> >>>> * ... MemoryRegions actually do not have their >>>> * own reference count; they piggyback on a QOM object, their "owner". >>>> * This function adds a reference to the owner. >>>> >>>> It looks like what happened is when introduced the change, MR is not >>>> a QOM >>>> object yet. But it later is.. >>>> >>>> I mentioned all these only because I found that _if_ we can keep mr >>>> refcounting as simple as other objects: >>>> >>>> memory_region_ref(mr) >>>> { >>>> object_ref(OBJECT(mr)); >>>> } >>>> >>>> Then looks like this "recursive refcount" problem can also go away. I'm >>>> curious whether you or anyone tried to explore that path, or whether >>>> above >>>> doesn't make sense at all. >>> >>> It unfortunately does not solve the problem. >>> >>> The underlying problem is that the whole device must be kept alive while >>> its memory region are. Indeed MemoryRegions do have refcounts, but >>> incrementing them do not extend the lifetime of the devices (i.e., the >>> owners). The refcount of the owners must be incremented for correctness. >>> >>> Referencing a subregion MemoryRegion from its container MemoryRegion >>> owned by the same device is an exceptional case. Incrementing the >>> refcount of the owner extends the owner's lifetime to forever. >> >> Is it really an exceptional case? >> >> What I'm seeing are a lot of devices creating MR and never bother >> to destroy them, so indeed owner (device) refcount never reaches 0. >> >> Most of the time when we create MR in .instance_init/.realize, >> we neglect to implement the undo path (.instance_finalize or >> .unrealize). > > Right. The cross-reference of MR <-> device makes sense from the concept > level, but I'm not sure how much we rely on that from QEMU perspective > currently. It complicates refcount and looks like it needs some thoughts. > > One proof is, when I replied I actually tested all qemu qtests with below > patch and nothing yet explode: > > ===8<=== > diff --git a/system/memory.c b/system/memory.c > index 2d69521360..a1d2c1f808 100644 > --- a/system/memory.c > +++ b/system/memory.c > @@ -1801,26 +1801,12 @@ Object *memory_region_owner(MemoryRegion *mr) > > void memory_region_ref(MemoryRegion *mr) > { > - /* MMIO callbacks most likely will access data that belongs > - * to the owner, hence the need to ref/unref the owner whenever > - * the memory region is in use. > - * > - * The memory region is a child of its owner. As long as the > - * owner doesn't call unparent itself on the memory region, > - * ref-ing the owner will also keep the memory region alive. > - * Memory regions without an owner are supposed to never go away; > - * we do not ref/unref them because it slows down DMA sensibly. > - */ > - if (mr && mr->owner) { > - object_ref(mr->owner); > - } > + object_ref(OBJECT(mr)); > } > > void memory_region_unref(MemoryRegion *mr) > { > - if (mr && mr->owner) { > - object_unref(mr->owner); > - } > + object_unref(OBJECT(mr)); > } > ===8<=== > > I'm not sure how much it covers dynamic destruction of devices, though, > where the device creates internal MRs to the guest address space. I'm > actually wondering whether we have other barriers already to avoid having > any MR ops being invoked if a device was removed. > > So I think above indeed would remove the cross-ref we used to have. It > might be a matter of whether that's fine for now to fix the cyclic ref > issue alone, and even if we still want some cross-ref.. whether we should > do it the old way. I mean, the cross-ref can also be done in other forms, > and it may not block MR has its own refcounts. We don't even need reference-counting memory regions if we are sure there would be no reference to a MR when the device owning it is gone because a MR is already alive when the device owning it is. Unfortunately that is not the case. If you grep for memory_region_ref(), you will find lot of places that asynchronously calls memory_region_unref(). Devices can be removed whenever the BQL is unlocked so we certainly need to count references. > > Even if we want to have dev<->MR refcount, I wonder whether we should avoid > having MR refcount the device, because it looks like "sub-object refs the > parent" which sounds like a good source of cyclic refcount issue. I wonder > whether device should ref the MR instead (when MR is dynamically created; I > am not sure whether we even need that if in Phil's comment where the MR > object is a sub-struct-field of the Device object). Then the device can > provide a proper destroy() function to release the MRs under it, perhaps by > disabling them first (then any ops should already fail after that), then > unref the MRs with the hope that they're the last ref holder then MRs > destroys there too. I think it is better to remove the reference counter from Object and create a subclass like RefCounted to add one for sub-struct-field cases. There are exceptional cases in virtio-gpu-gl that dynamically allocate MRs but such a device can create Objects that functions as owners of dynamically-allocated MRs. Such a change is independent and out of the scope of this change though. It also requires to convert all other usage of Object to RefCounted, which is not really trivial. Regards, Akihiko Odaki
On Thu, 27 Jun 2024 at 14:40, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > A memory region does not use their own reference counters, but instead > piggybacks on another QOM object, "owner" (unless the owner is not the > memory region itself). When creating a subregion, a new reference to the > owner of the container must be created. However, if the subregion is > owned by the same QOM object, this result in a self-reference, and make > the owner immortal. Avoid such a self-reference. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > system/memory.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/system/memory.c b/system/memory.c > index 74cd73ebc78b..949f5016a68d 100644 > --- a/system/memory.c > +++ b/system/memory.c > @@ -2638,7 +2638,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); > @@ -2696,7 +2699,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(); > } I was having another look at leaks this week, and I tried this patch to see how many of the leaks I was seeing it fixed. I found however that for arm it results in an assertion when the device-introspection-test exercises the "imx7.analog" device. By-hand repro: $ ./build/asan/qemu-system-aarch64 -display none -machine none -accel qtest -monitor stdio ==712838==WARNING: ASan doesn't fully support makecontext/swapcontext functions and may produce false positives in some cases! QEMU 9.0.92 monitor - type 'help' for more information (qemu) device_add imx7.analog,help qemu-system-aarch64: ../../system/memory.c:1777: void memory_region_finalize(Object *): Assertion `!mr->container' failed. Aborted (core dumped) It may be well be that this is a preexisting bug that's only exposed by this refcount change causing us to actually try to dispose of the memory regions. I think that what's happening here is that the device object has multiple MemoryRegions, each of which is a child QOM property. One of these MRs is a "container MR", and the other three are actual-content MRs which the device put into the container when it created them. When we deref the device, we go through all the child QOM properties unparenting and unreffing them. However, there's no particular ordering here, and it happens that we try to unref one of the actual-content MRs first. That MR is still inside the container MR, so we hit the assert. If we had happened to unref the container MR first then memory_region_finalize() would have removed all the subregions from it, avoiding the problem. I'm not sure what the best fix would be here -- that assert is there as a guard that the region isn't visible in any address space, so maybe it needs to be made a bit cleverer about the condition it checks? e.g. in this example although mr->container is not NULL, mr->container->container is NULL. Or we could check whether the mr->container->owner is the same as the mr->owner and allow a non-NULL mr->container in that case. I don't know this subsystem well enough so I'm just making random stabs here, though. thanks -- PMM
On Thu, Aug 22, 2024 at 06:10:43PM +0100, Peter Maydell wrote: > On Thu, 27 Jun 2024 at 14:40, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > > > A memory region does not use their own reference counters, but instead > > piggybacks on another QOM object, "owner" (unless the owner is not the > > memory region itself). When creating a subregion, a new reference to the > > owner of the container must be created. However, if the subregion is > > owned by the same QOM object, this result in a self-reference, and make > > the owner immortal. Avoid such a self-reference. > > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > --- > > system/memory.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/system/memory.c b/system/memory.c > > index 74cd73ebc78b..949f5016a68d 100644 > > --- a/system/memory.c > > +++ b/system/memory.c > > @@ -2638,7 +2638,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); > > @@ -2696,7 +2699,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(); > > } > > I was having another look at leaks this week, and I tried > this patch to see how many of the leaks I was seeing it > fixed. I found however that for arm it results in an > assertion when the device-introspection-test exercises > the "imx7.analog" device. By-hand repro: > > $ ./build/asan/qemu-system-aarch64 -display none -machine none -accel > qtest -monitor stdio > ==712838==WARNING: ASan doesn't fully support makecontext/swapcontext > functions and may produce false positives in some cases! > QEMU 9.0.92 monitor - type 'help' for more information > (qemu) device_add imx7.analog,help > qemu-system-aarch64: ../../system/memory.c:1777: void > memory_region_finalize(Object *): Assertion `!mr->container' failed. > Aborted (core dumped) > > It may be well be that this is a preexisting bug that's only > exposed by this refcount change causing us to actually try > to dispose of the memory regions. > > I think that what's happening here is that the device > object has multiple MemoryRegions, each of which is a child > QOM property. One of these MRs is a "container MR", and the > other three are actual-content MRs which the device put into > the container when it created them. When we deref the device, > we go through all the child QOM properties unparenting and > unreffing them. However, there's no particular ordering > here, and it happens that we try to unref one of the > actual-content MRs first. That MR is still inside the > container MR, so we hit the assert. If we had happened to > unref the container MR first then memory_region_finalize() > would have removed all the subregions from it, avoiding > the problem. > > I'm not sure what the best fix would be here -- that assert > is there as a guard that the region isn't visible in > any address space, so maybe it needs to be made a bit > cleverer about the condition it checks? e.g. in this > example although mr->container is not NULL, > mr->container->container is NULL. If we keep looking at ->container we'll always see NULL, IIUC, because either it's removed from its parent MR so it's NULL already, or at some point it can start to point to a root mr of an address space, where should also be NULL, afaiu. > Or we could check whether the mr->container->owner is the same as the > mr->owner and allow a non-NULL mr->container in that case. I don't know > this subsystem well enough so I'm just making random stabs here, though. If with the assumption of this patch applied, then looks like it's pretty legal a container MR and the child MRs be finalized in any order when the owner device is being destroyed. IIUC the MR should be destined to be invisible until this point, with or without the fact that mr->container is NULL. It's because anyone who references the MR should do memory_region_ref() first, which takes the owner's refcount. Here if MR finalize() is reached I think it means the owner refcount must be zero. So it looks to me the only possible case when mr->container is non-NULL is it's used internally like this. Then it's invisible and also safe to be detached even if container != NULL. So.. I wonder whether below would make sense, on top of this existing patch. ===8<=== diff --git a/system/memory.c b/system/memory.c index 1c00df8305..54a9d9e5f9 100644 --- a/system/memory.c +++ b/system/memory.c @@ -1771,16 +1771,23 @@ 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. + /* + * We know the region is not visible in any address space, because + * normally MR's finalize() should be invoked by finalize() of the + * owner, which will remove all the properties including the MRs, and + * that can only happen when prior memory_region_ref() of the MR (which + * will ultimately take the owner's refcount) from elsewhere got + * properly released. + * + * So we can blindly clear mr->enabled, unlink both the upper container + * or all subregions. memory_region_set_enabled() won't work instead, + * as it could trigger a transaction and cause an infinite loop. */ mr->enabled = false; memory_region_transaction_begin(); + if (mr->container) { + memory_region_del_subregion(mr->container, mr); + } while (!QTAILQ_EMPTY(&mr->subregions)) { MemoryRegion *subregion = QTAILQ_FIRST(&mr->subregions); ===8<=== Thanks,
On 2024/08/23 6:01, Peter Xu wrote: > On Thu, Aug 22, 2024 at 06:10:43PM +0100, Peter Maydell wrote: >> On Thu, 27 Jun 2024 at 14:40, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >>> >>> A memory region does not use their own reference counters, but instead >>> piggybacks on another QOM object, "owner" (unless the owner is not the >>> memory region itself). When creating a subregion, a new reference to the >>> owner of the container must be created. However, if the subregion is >>> owned by the same QOM object, this result in a self-reference, and make >>> the owner immortal. Avoid such a self-reference. >>> >>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>> --- >>> system/memory.c | 11 +++++++++-- >>> 1 file changed, 9 insertions(+), 2 deletions(-) >>> >>> diff --git a/system/memory.c b/system/memory.c >>> index 74cd73ebc78b..949f5016a68d 100644 >>> --- a/system/memory.c >>> +++ b/system/memory.c >>> @@ -2638,7 +2638,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); >>> @@ -2696,7 +2699,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(); >>> } >> >> I was having another look at leaks this week, and I tried >> this patch to see how many of the leaks I was seeing it >> fixed. I found however that for arm it results in an >> assertion when the device-introspection-test exercises >> the "imx7.analog" device. By-hand repro: >> >> $ ./build/asan/qemu-system-aarch64 -display none -machine none -accel >> qtest -monitor stdio >> ==712838==WARNING: ASan doesn't fully support makecontext/swapcontext >> functions and may produce false positives in some cases! >> QEMU 9.0.92 monitor - type 'help' for more information >> (qemu) device_add imx7.analog,help >> qemu-system-aarch64: ../../system/memory.c:1777: void >> memory_region_finalize(Object *): Assertion `!mr->container' failed. >> Aborted (core dumped) >> >> It may be well be that this is a preexisting bug that's only >> exposed by this refcount change causing us to actually try >> to dispose of the memory regions. >> >> I think that what's happening here is that the device >> object has multiple MemoryRegions, each of which is a child >> QOM property. One of these MRs is a "container MR", and the >> other three are actual-content MRs which the device put into >> the container when it created them. When we deref the device, >> we go through all the child QOM properties unparenting and >> unreffing them. However, there's no particular ordering >> here, and it happens that we try to unref one of the >> actual-content MRs first. That MR is still inside the >> container MR, so we hit the assert. If we had happened to >> unref the container MR first then memory_region_finalize() >> would have removed all the subregions from it, avoiding >> the problem. >> >> I'm not sure what the best fix would be here -- that assert >> is there as a guard that the region isn't visible in >> any address space, so maybe it needs to be made a bit >> cleverer about the condition it checks? e.g. in this >> example although mr->container is not NULL, >> mr->container->container is NULL. > > If we keep looking at ->container we'll always see NULL, IIUC, because > either it's removed from its parent MR so it's NULL already, or at some > point it can start to point to a root mr of an address space, where should > also be NULL, afaiu. > >> Or we could check whether the mr->container->owner is the same as the >> mr->owner and allow a non-NULL mr->container in that case. I don't know >> this subsystem well enough so I'm just making random stabs here, though. > > If with the assumption of this patch applied, then looks like it's pretty > legal a container MR and the child MRs be finalized in any order when the > owner device is being destroyed. > > IIUC the MR should be destined to be invisible until this point, with or > without the fact that mr->container is NULL. It's because anyone who > references the MR should do memory_region_ref() first, which takes the > owner's refcount. Here if MR finalize() is reached I think it means the > owner refcount must be zero. So it looks to me the only possible case when > mr->container is non-NULL is it's used internally like this. Then it's > invisible and also safe to be detached even if container != NULL. It is still nice if we can protect internal uses; it is theoretically possible for the owner to remove the subregion at runtime, and we want to ensure the subregion will be kept alive while its container is even in such a case. We can do so by creating a reference to the subregion itself instead of its owner. I sent v4 for this change. Regards, Akihiko Odaki
diff --git a/system/memory.c b/system/memory.c index 74cd73ebc78b..949f5016a68d 100644 --- a/system/memory.c +++ b/system/memory.c @@ -2638,7 +2638,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); @@ -2696,7 +2699,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(); }
A memory region does not use their own reference counters, but instead piggybacks on another QOM object, "owner" (unless the owner is not the memory region itself). When creating a subregion, a new reference to the owner of the container must be created. However, if the subregion is owned by the same QOM object, this result in a self-reference, and make the owner immortal. Avoid such a self-reference. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- system/memory.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)