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

[Peter Xu <peterx@redhat.com>]

On Sat, Jan 18, 2025 at 07:15:56PM +0900, Akihiko Odaki wrote:
> On 2025/01/18 2:46, Peter Xu wrote:
> > On Fri, Jan 17, 2025 at 03:24:34PM +0900, Akihiko Odaki wrote:
> > > On 2025/01/16 23:33, Peter Xu wrote:
> > > > On Thu, Jan 16, 2025 at 02:37:38PM +0900, Akihiko Odaki wrote:
> > > > > On 2025/01/16 1:14, Peter Xu wrote:
> > > > > > On Thu, Jan 16, 2025 at 12:52:56AM +0900, Akihiko Odaki wrote:
> > > > > > > Functionally, the ordering of container/subregion finalization matters if
> > > > > > > some device tries to a container during finalization. In such a case,
> > > > > >                          |
> > > > > >                          ^ something is missing here, feel free to complete this.
> > > > > 
> > > > > Oops, I meant: functionally, the ordering of container/subregion
> > > > > finalization matters if some device tries to use a container during
> > > > > finalization.
> > > > 
> > > > This is true, though if we keep the concept of "all the MRs share the same
> > > > lifecycle of the owner" idea, another fix of such is simply moving the
> > > > container access before any detachment of MRs.
> > > > 
> > > > > 
> > > > > > 
> > > > > > > removing subregions from the container at random timing can result in an
> > > > > > > unexpected behavior. There is little chance to have such a scenario but we
> > > > > > > should stay the safe side if possible.
> > > > > > 
> > > > > > It sounds like a future feature, and I'm not sure we'll get there, so I
> > > > > > don't worry that much.  Keeping refcount core idea simple is still very
> > > > > > attractive to me.  I still prefer we have complete MR refcounting iff when
> > > > > > necessary.  It's also possible it'll never happen to QEMU.
> > > > > > 
> > > > > 
> > > > > It's not just about the future but also about compatibility with the current
> > > > > device implementations. I will not be surprised even if the random ordering
> > > > > of subregion finalization breaks one of dozens of devices we already have.
> > > > > We should pay attention the details as we are touching the core
> > > > > infrastructure.
> > > > 
> > > > Yes, if we can find any such example that we must follow the order of MR
> > > > destruction, I think that could justify your approach will be required but
> > > > not optional.  It's just that per my understanding there should be none,
> > > > and even if there're very few outliers, it can still be trivially fixed as
> > > > mentioned above.
> > > 
> > > It can be fixed but that means we need auditing the code of devices or wait
> > > until we get a bug report.
> > 
> > We'd better have a solid example.
> > 
> > And for this specific question, IIUC we can have such problem even if
> > internal-ref start to use MR refcounts.
> > 
> > It's because we have a not very straightforward way of finalize() an
> > object, which is freeing all properties before its own finalize()..
> > 
> > static void object_finalize(void *data)
> > {
> >      ...
> >      object_property_del_all(obj);
> >      object_deinit(obj, ti);
> >      ...
> > }
> > 
> > I think it used to be the other way round (which will be easier to
> > understand to me..), but changed after 76a6e1cc7cc.  It could boil down to
> > two dependencies: (1) children's unparent() callback wanting to have the
> > parent being present and valid, and (2) parent's finalize() callback
> > wanting to have all children being present and valid.  I guess we chose (1)
> > as of now.
> > 
> > So I suppose it means even with your patch, it won't help either as long as
> > MRs are properties, and they can already all be gone in a device finalize()
> > even with your new patch.
> 
> The owner can object_ref() to keep the memory region alive.

Do you mean explicitly (rather by the add_subregion)?  Why an owner need to
do it at all, if it knows the MR is part of itself?

-- 
Peter Xu