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

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2025/01/11 0:18, Peter Xu wrote:
> On Fri, Jan 10, 2025 at 05:43:15PM +0900, Akihiko Odaki wrote:
>> On 2025/01/10 4:37, Peter Xu wrote:
>>> On Thu, Jan 09, 2025 at 02:29:21PM -0500, Peter Xu wrote:
>>>> On Thu, Jan 09, 2025 at 01:30:35PM +0100, BALATON Zoltan wrote:
>>>>> On Thu, 9 Jan 2025, Akihiko Odaki wrote:
>>>>>> Do not refer to "memory region's reference count"
>>>>>> -------------------------------------------------
>>>>>>
>>>>>> Now MemoryRegions do have their own reference counts, but they will not
>>>>>> be used when their owners are not themselves. However, the documentation
>>>>>> of memory_region_ref() says it adds "1 to a memory region's reference
>>>>>> count", which is confusing. Avoid referring to "memory region's
>>>>>> reference count" and just say: "Add a reference to a memory region".
>>>>>> Make a similar change to memory_region_unref() too.
>>>>>>
>>>>>> Refer to docs/devel/memory.rst for "owner"
>>>>>> ------------------------------------------
>>>>>>
>>>>>> memory_region_ref() and memory_region_unref() used to have their own
>>>>>> descriptions of "owner", but they are somewhat out-of-date and
>>>>>> misleading.
>>>>>>
>>>>>> In particular, they say "whenever memory regions are accessed outside
>>>>>> the BQL, they need to be preserved against hot-unplug", but protecting
>>>>>> against hot-unplug is not mandatory if it is known that they will never
>>>>>> be hot-unplugged. They also say "MemoryRegions actually do not have
>>>>>> their own reference count", but they actually do. They just will not be
>>>>>> used unless their owners are not themselves.
>>>>>>
>>>>>> Refer to docs/devel/memory.rst as the single source of truth instead of
>>>>>> maintaining duplicate descriptions of "owner".
>>>>>>
>>>>>> Clarify that owner may be missing
>>>>>>
>>>>>> ---------------------------------
>>>>>> A memory region may not have an owner, and memory_region_ref() and
>>>>>> memory_region_unref() do nothing for such.
>>>>>>
>>>>>> memory: Clarify owner must not call memory_region_ref()
>>>>>> --------------------------------------------------------
>>>>>>
>>>>>> The owner must not call this function as it results in a circular
>>>>>> reference.
>>>>>>
>>>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>>>> Reviewed-by: Peter Xu <peterx@redhat.com>
>>>>>> ---
>>>>>> include/exec/memory.h | 59 ++++++++++++++++++++++++---------------------------
>>>>>> 1 file changed, 28 insertions(+), 31 deletions(-)
>>>>>>
>>>>>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>>>>>> index 9458e2801d50..ca247343f433 100644
>>>>>> --- a/include/exec/memory.h
>>>>>> +++ b/include/exec/memory.h
>>>>>> @@ -1210,7 +1210,7 @@ void memory_region_section_free_copy(MemoryRegionSection *s);
>>>>>>    * memory_region_add_subregion() to add subregions.
>>>>>>    *
>>>>>>    * @mr: the #MemoryRegion to be initialized
>>>>>> - * @owner: the object that tracks the region's reference count
>>>>>> + * @owner: the object that keeps the region alive
>>>>>>    * @name: used for debugging; not visible to the user or ABI
>>>>>>    * @size: size of the region; any subregions beyond this size will be clipped
>>>>>>    */
>>>>>> @@ -1220,29 +1220,26 @@ void memory_region_init(MemoryRegion *mr,
>>>>>>                           uint64_t size);
>>>>>>
>>>>>> /**
>>>>>> - * memory_region_ref: Add 1 to a memory region's reference count
>>>>>> + * memory_region_ref: Add a reference to the owner of a memory region
>>>>>>    *
>>>>>> - * Whenever memory regions are accessed outside the BQL, they need to be
>>>>>> - * preserved against hot-unplug.  MemoryRegions actually do not have their
>>>>>> - * own reference count; they piggyback on a QOM object, their "owner".
>>>>>> - * This function adds a reference to the owner.
>>>>>> - *
>>>>>> - * All MemoryRegions must have an owner if they can disappear, even if the
>>>>>> - * device they belong to operates exclusively under the BQL.  This is because
>>>>>> - * the region could be returned at any time by memory_region_find, and this
>>>>>> - * is usually under guest control.
>>>>>> + * This function adds a reference to the owner of a memory region to keep the
>>>>>> + * memory region alive. It does nothing if the owner is not present as a memory
>>>>>> + * region without owner will never die.
>>>>>> + * For references internal to the owner, use object_ref() instead to avoid a
>>>>>> + * circular reference.
>>>>>
>>>>> Reading this again I'm still confused by this last sentence. Do you mean
>>>>> references internal to the memory region should use object_ref on the memory
>>>>> region or that other references to the owner should use object_ref on the
>>>>> owner? This sentence is still not clear about that.
>>>>
>>>> Having two refcounts are definitely confusing.. especially IIRC all MRs'
>>>> obj->free==NULL, so the MR's refcount isn't working.  Dynamic MR's needs
>>>> its g_free() on its own.
>>
>> We still have instance_finalize that will fire when the MR's refcount gets
>> zero so it has its own use cases.
>>
>>>>
>>>> I acked both patches, but maybe it could indeed be slightly better we drop
>>>> this sentence, meanwhile in patch 2 we can drop the object_ref() too: it
>>>> means for parent/child MRs that share the same owner, QEMU does nothing on
>>>> the child MRs when add subregion, because it assumes the child MR will
>>>> never go away when the parent is there who shares the owner.
>>>>
>>>> So maybe we try not to touch MR's refcount manually, but fix what can be
>>>> problematic for owner->ref only.
>>>
>>> As an attached comment: I may have forgot some context on this issue, but I
>>> still remember I used to have a patch that simply detach either parent or
>>> child MR links when finalize().  It's here:
>>>
>>> https://lore.kernel.org/all/ZsenKpu1czQGYz7m@x1n/
>>>
>>> I see this issue was there for a long time so maybe we want to fix it one
>>> way or another.  I don't strongly feel which way to go, but personally I
>>> still prefer that way (I assume that can fix the same issue), and it
>>> doesn't have MR's refcount involved at all, meanwhile I don't see an issue
>>> yet with it..
>>>
>>
>> For this particular topic I have somewhat a strong opinion that we should
>> care the two reference counters.
>>
>> Indeed, dealing with two reference counters is not fun, but sometimes it is
>> necessary to do reference counting correctly. Your patch is to avoid
>> reference counting for tracking dependencies among regions with the same
>> owner, and it does so by ignoring the reference from container to subregion.
> 
> I don't think so?  When with that patch, container will reference subregion
> the same way as others, which is to take a refcount on the owner.  That
> kept at least the refcount behavior consistent within memory_region_ref().

memory_region_ref() is not the only place that is responsible for 
reference management. memory_region_do_init() also calls 
object_property_add_child(), which in turn calls object_ref() to create 
a reference from the owner to the memory region. We should keep using 
object_ref() for object references originating from the owner.

> 
> That patch removes the circular reference by always properly release the
> circular reference due to sub-regioning internally.

Calling memory_region_del_subregion() is not consistent with the 
direction of object references. A container references its subregion so 
the container should remove references to its subregion when 
appropriate. A subregion should not remove the reference its container 
holds.

> 
>>
>> I prefer to keep reference counting correct instead of having an additional
>> ad-hoc measure that breaks reference relationships.
> 
> Your patch added more complexity to me on refcounting, meanwhile it's also
> not always "correct".  It can boil down to how you define "correct" - if
> you mean one should always boost a refcount somewhere if it references one
> MR, then it's still not 100% correct at least when mr->owner==NULL.  We
> never yet did it alright, so to me it's a matter of working around current
> sanitizer issue, and that's only about it yet so far.

mr->owner == NULL is an exceptional case that we allow for performance 
reasons. We have luxury to spend more time in our case.

> 
> Meanwhile I _think_ adding such complexity also means MR's finalize() will
> be called in specific order when parent/child MRs belong to the same owner.
> In my patch the order shouldn't matter, IIUC, which I preferred because
> that reduces details that we may not care much (or I could have overlooked
> why we need to care about it).  Basically that's simpler to maintain to me,
> but again, I don't feel strongly until someone would like / be able to
> rework MR refcounting completely.

We need to take care of the semantics of subregion. A container needs 
its subregions to satisfy accesses to the memory it represents. So it 
refers to the subregions, and the reference must keep the subregions 
alive; that's why we must keep the ordering.

Regards,
Akihiko Odaki