| Message ID | 20240829-memory-v1-1-ac07af2f4fa5@daynix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | docs/devel: Prohibit calling object_unparent() for memory region | expand |
On Thu, Aug 29, 2024 at 02:46:48PM +0900, Akihiko Odaki wrote: > Previously it was allowed to call object_unparent() for a memory region > in instance_finalize() of its parent. However, such a call typically > has no effect because child objects get unparented before > instance_finalize(). > > Worse, memory regions typically gets finalized when they get unparented > before instance_finalize(). This means calling object_unparent() for > them in instance_finalize() is to call the function for an object > already finalized, which should be avoided. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> Acked-by: Michael S. Tsirkin <mst@redhat.com> who's applying this? Paolo? > --- > docs/devel/memory.rst | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst > index 69c5e3f914ac..83760279e3db 100644 > --- a/docs/devel/memory.rst > +++ b/docs/devel/memory.rst > @@ -168,11 +168,10 @@ and VFIOQuirk in hw/vfio/pci.c. > > You must not destroy a memory region as long as it may be in use by a > device or CPU. In order to do this, as a general rule do not create or > -destroy memory regions dynamically during a device's lifetime, and only > -call object_unparent() in the memory region owner's instance_finalize > -callback. The dynamically allocated data structure that contains the > -memory region then should obviously be freed in the instance_finalize > -callback as well. > +destroy memory regions dynamically during a device's lifetime, and do not > +call object_unparent(). The dynamically allocated data structure that contains > +the memory region then should be freed in the instance_finalize callback, which > +is called after it gets unparented. > > If you break this rule, the following situation can happen: > > @@ -199,8 +198,9 @@ but nevertheless it is used in a few places. > > For regions that "have no owner" (NULL is passed at creation time), the > machine object is actually used as the owner. Since instance_finalize is > -never called for the machine object, you must never call object_unparent > -on regions that have no owner, unless they are aliases or containers. > +never called for the machine object, you must never free regions that have no > +owner, unless they are aliases or containers, which you can manually call > +object_unparent() for. > > > Overlapping regions and priority > > --- > base-commit: 31669121a01a14732f57c49400bc239cf9fd505f > change-id: 20240829-memory-cfd3ee0af44d > > Best regards, > -- > Akihiko Odaki <akihiko.odaki@daynix.com>
On Tue, 10 Sept 2024 at 18:27, Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, Aug 29, 2024 at 02:46:48PM +0900, Akihiko Odaki wrote: > > Previously it was allowed to call object_unparent() for a memory region > > in instance_finalize() of its parent. However, such a call typically > > has no effect because child objects get unparented before > > instance_finalize(). > > > > Worse, memory regions typically gets finalized when they get unparented > > before instance_finalize(). This means calling object_unparent() for > > them in instance_finalize() is to call the function for an object > > already finalized, which should be avoided. > > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > who's applying this? Paolo? It's a docs change to clarify a complex point that's under active discussion in a different patch thread, so it needs review before anybody applies it... -- PMM
diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst index 69c5e3f914ac..83760279e3db 100644 --- a/docs/devel/memory.rst +++ b/docs/devel/memory.rst @@ -168,11 +168,10 @@ and VFIOQuirk in hw/vfio/pci.c. You must not destroy a memory region as long as it may be in use by a device or CPU. In order to do this, as a general rule do not create or -destroy memory regions dynamically during a device's lifetime, and only -call object_unparent() in the memory region owner's instance_finalize -callback. The dynamically allocated data structure that contains the -memory region then should obviously be freed in the instance_finalize -callback as well. +destroy memory regions dynamically during a device's lifetime, and do not +call object_unparent(). The dynamically allocated data structure that contains +the memory region then should be freed in the instance_finalize callback, which +is called after it gets unparented. If you break this rule, the following situation can happen: @@ -199,8 +198,9 @@ but nevertheless it is used in a few places. For regions that "have no owner" (NULL is passed at creation time), the machine object is actually used as the owner. Since instance_finalize is -never called for the machine object, you must never call object_unparent -on regions that have no owner, unless they are aliases or containers. +never called for the machine object, you must never free regions that have no +owner, unless they are aliases or containers, which you can manually call +object_unparent() for. Overlapping regions and priority
Previously it was allowed to call object_unparent() for a memory region in instance_finalize() of its parent. However, such a call typically has no effect because child objects get unparented before instance_finalize(). Worse, memory regions typically gets finalized when they get unparented before instance_finalize(). This means calling object_unparent() for them in instance_finalize() is to call the function for an object already finalized, which should be avoided. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- docs/devel/memory.rst | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) --- base-commit: 31669121a01a14732f57c49400bc239cf9fd505f change-id: 20240829-memory-cfd3ee0af44d Best regards,