Re: [PATCH v7 1/2] memory: Update inline documentation

[Akihiko Odaki]

On 2025/01/15 2:02, Peter Xu wrote:
> On Tue, Jan 14, 2025 at 05:43:09PM +0900, Akihiko Odaki wrote:
> > memory_region_finalize() is not a function to tell the owner is leaving, but
> > the memory region itself is being destroyed.
>
> It is when the lifecycle of the MR is the same as the owner.  That holds
> true I suppose if without this patch, and that's why I don't prefer this
> patch because it makes that part more complicated.

The lifecycle of the MR is not the same as the owner. The MR gets 
finalized during the finalization of the owner, and the owner is still 
alive at the moment. It is something you should always care when having 
a child object.

> > It should not happen when a container is still referencing it. That is
> > also why it has memory_region_ref(subregion) in
> > memory_region_update_container_subregions() and assert(!mr->container) in
> > memory_region_finalize().
>
> Again, the line I added was sololy for what you said "automation" elsewhere
> and only should work within MR-links within the same owner.  Otherwise
> anyone referencing the MR would hold the owner ref then this finalize()
> will never happen.
>
> Now, if I could go back to your original purpose of this work, quotting
> from your cover letter:
>
> > I saw various sanitizer errors when running check-qtest-ppc64. While
> > I could just turn off sanitizers, I decided to tackle them this time.
> >
> > Unfortunately, GLib versions older than 2.81.0 do not free test data in
> > some cases so some sanitizer errors remain. All sanitizer errors will be
> > gone with this patch series combined with the following change for GLib:
> > https://gitlab.gnome.org/GNOME/glib/-/merge_requests/4120
>
> Is check-qtest-ppc64 the only one that will trigger this issue?  Does it
> mean that most of the devices will do proper removal of device-owned
> subregions (hence, not prone to circular reference of owner refcount)
> except some devices in ppc64?

Searching for memory_region_add_subregion() gives 1078 instances where 
there are 142 instances of memory_region_del_subregion(). This is a 
rough estimate but there are potentially 936 instances of subregions 
without explicit deletion.

For example, hw/audio/intel-hda.c adds subregions immediately after 
their containers never deletes the subregions. I think that's fine 
because their lifetimes are obvious with reference counters.