On Fri, Sep 12, 2025 at 11:09:51AM +0900, Akihiko Odaki wrote: > > > > I think it still makes some sense to theoretically allow > > > > object_unparent() > > > > to happen, at least when it happens before owner's finalize(). IIUC > > > > that > > > > was the intention of the doc, pairing the memory_region_init*() > > > > operation. > > > > > > Perhaps so, but this patch is only about the case where object_unparent() > > > is > > > called in finalize(). > > > > You ignored my other comment. That (using object_new() on MRs) was what I > > was thinking might be better than a split model you discussed here, so > > that's also a comment for patch 1 of your series. > > I'm not sure what you mean by the "split model".
I meant after similar change as what this patch proposes, (a) "owner of the MR lifecycle" (aka, who decides to finalize() the MR) is not the same as (b) "owner of the memory" (aka, who decides to free() the memory backing the MR struct), so the ownership model itself is more or less "split". Now it's very hard to tell who owns the MR, because each owns only part of it. IMHO it'll be slightly better to have the instance lifecycle and the memory allocation of the MR struct be managed by the same object, no matter automatically by the memory core, or manually by the device (in the case of the current doc, it went with the latter, even though I agree with you it looks wrong). > > This change removes object_unparent() in vfio_bars_finalize(). object_new() > will allow removing even g_free(), but we can do these changes > incrementally: > 1. remove object_unparent() in finalize(), > which fixes a semantic problem (this patch) > 2. allow object_new() for MRs and remove g_free() in finalize() > as a refactoring. > > So I suggest focusing on object_unparent() in finalize() to keep this patch > and review concise. I agree that is the minimal change, but this is a common pattern. It's not high risk, so I think we could still discuss it thoroughly. I further analyzed the risk here, it turns out object_unparent() in finalize() is still very safe so the current code is actually bug-free if it all works similarly like the vfio code: The object_property_del_all() (on top of the owner device) would do prop->release(), and here MR will kickoff object_finalize_child_property(), which resets mr->parent to NULL. So the 2nd object_unparent() will already see obj->parent==NULL. Directly dropping object_unparent() should work, but leads to confusion as "split ownership model" as I discussed above. Thanks to all recent discussions, IMHO we have much clearer picture of how MRs can be used. I discussed it almostly in the first reply: https://lore.kernel.org/all/aMHidDl1tdx-2G4e@x1.local/ I suspect we don't really have 2nd user I mentioned, because if so it'll likely require strict mr refcounting due to address_space_map(), in which case we should go the "create a temp obj as the owner of MR" idea, that you used to fix the virgl issue in patch 2 of your other series. For the current issue, I'd suggest as simple as below (I observed at least the current VFIO use case only uses MMIO memory regions, so we only need one such helper): /* * Unlike memory_region_init_io(), @memory_region_alloc_io allocates an IO * memory region object and returns. * * After the function returns, the MemoryRegion object will share the same * lifecycle of the owner object. If owner is not specified, the MR will * never be released. * * The caller doesn't need to either detach or unref/free the MR object. * It will be automatically detached and returned when the owner finalize. * The caller can cache the MR object pointer, but it's only valid to * operate before the owner finalizes. */ MemoryRegion * memory_region_alloc_io(MemoryRegion *mr, Object *owner, const MemoryRegionOps *ops, void *opaque, const char *name, uint64_t size) { MemoryRegion = object_new(TYPE_MEMORY_REGION); memory_region_do_init(mr, owner, name, size); mr->ops = ops ? ops : &unassigned_mem_ops; mr->opaque = opaque; mr->terminates = true; } Here, IIUC if we can allocate the MR using memory_region_alloc_io() here, then the ownership of both (a)+(b) above will be done automatically by the memory core / object core code. The device impl doesn't need to care about either removal of subregions, or free of MR struct, anymore. Then we can drop not only the object_unparent(), but also g_free(), altogether. Would that sound like a better approach in general? Again, I don't think this is anything urgent, so we can take time to think it through. Thanks, -- Peter Xu
