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

[BALATON Zoltan]

On Sat, 4 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.

The commit message is longer than the documentation it changes :-) That 
probably means the docs could be more detailed. For example this relation 
to the owner may be mentioned unless it's something to be changed in the 
future to clean this up.

> 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>
> ---
> include/exec/memory.h | 22 +++++++---------------
> 1 file changed, 7 insertions(+), 15 deletions(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 9458e2801d50..cd91fe0c51cf 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1220,29 +1220,21 @@ 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 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 if present.

Maybe it's just not clear to me but the title says "Add a reference to a 
memory region" then here it says "adds a reference to the owner" and does 
not say what happens if there's no owner present. Maybe it's better to be 
explicit and say add 1 to the owner's ref count or do nothing if owner is 
not present.

> + * The owner must not call this function as it results in a circular reference.
> + * See docs/devel/memory.rst to know about owner.
>  *
>  * @mr: the #MemoryRegion
>  */
> void memory_region_ref(MemoryRegion *mr);
>
> /**
> - * memory_region_unref: Remove 1 to a memory region's reference count
> + * memory_region_unref: Remove a reference to 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 removes a reference to the owner and possibly destroys it.
> + * This function removes a reference to the owner and possibly destroys it if
> + * present. See docs/devel/memory.rst to know about owner.

In "destroys it if present" it's not clear what "it" refers to as it can 
either be the memory region or the owner. I guess it's the owner but 
better state that to avoid confusion.

Regards,
BALATON Zoltan

>  *
>  * @mr: the #MemoryRegion
>  */
>
> --
> 2.47.1