| Message ID | 20250104-san-v5-1-8b430457b09d@daynix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix check-qtest-ppc64 sanitizer errors | expand |
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 > >
On 2025/01/04 21:51, BALATON Zoltan wrote: > 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. It's odd that the commit message is longer but there are a few reasons: 1. It describes why the old documentation is wrong. Some of such statements are replaced with the reference to docs/devel/memory.rst to make them shorter. 2. It contains information that is already described in the existing documentation for context. In this particular case, the owner is already described in docs/devel/memory.rst extensively. > >> 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. I wrote the title looking at other comments that describes the owner as "the object that tracks the region's reference count", which implies the region's reference count is part of the owner. But it is confusing to to describe the owner as "the object that tracks the region's reference count" in the first place. A memory region has its own reference count that can be used to express references internal to the owner (and actually used in the following patch). With v6 I have just sent, I changed to describe the owner as "the object that keeps the region alive". The title of this function now says "add a reference to the owner of a memory region". I replaced "add 1 to ~'s reference count" with "add a reference to" to make in concise (to fit into 80 columns in particular). I think it's fine not to mention the use of counter as it is an implementation detail users don't have to care. With v6, I also noted that this function does nothing when the owner is not present in the comment body as suggested. > >> + * 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. I modified the comment to tell that this function may destroy the owner along with the memory region in v6. Regards, Akihiko Odaki
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. + * 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. * * @mr: the #MemoryRegion */
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> --- include/exec/memory.h | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-)