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
