Re: [RFC PATCH v2 4/4] Documentation/admin-guide/mm/damon: add DAMON-based Hugepage Management documentation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Severe contradictions and copy-paste errors between the commit message, documentation, and parameter descriptions.
- [Low] Missing documentation section for the `quota_sz` parameter.
- [Low] reStructuredText section underlines are shorter than their titles, which will cause Sphinx build warnings.
- [Low] Multiple typographical, grammatical, and phrasing errors.
--

commit fc9f4527e2269a43dfb3a88b19482a76212ae0e5
Author: Asier Gutierrez <gutierrez.asier@huawei-partners.com>

Documentation/admin-guide/mm/damon: add DAMON-based Hugepage Management documentation

Add documentation for the DAMON-based Hugepage Management
(DAMON_HUGEPAGE) feature, which automatically manages huge pages  by
identifying cold memory regions and collapsing them back to regular
pages. The documentation covers the module's features, operation, and
all available module parameters.

> Documentation/admin-guide/mm/damon: add DAMON-based Hugepage Management documentation
> 
> Add documentation for the DAMON-based Hugepage Management
> (DAMON_HUGEPAGE) feature, which automatically manages huge pages  by
> identifying cold memory regions and collapsing them back to regular
> pages.

[Severity: Medium]
This isn't a bug, but there's a contradiction between the commit message
and the actual behavior described in the documentation. The commit message
claims it manages huge pages by identifying cold memory regions and
collapsing them back to regular pages (which sounds like splitting),
while the documentation correctly states it aims to collapse hot regions
into huge pages.

> diff --git a/Documentation/admin-guide/mm/damon/hugepage.rst b/Documentation/admin-guide/mm/damon/hugepage.rst
> new file mode 100644
> index 0000000000000..4e7a8dad765bd
> --- /dev/null
> +++ b/Documentation/admin-guide/mm/damon/hugepage.rst
> @@ -0,0 +1,258 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=======================
> +DAMON-based huge page collapsing
> +=======================

[Severity: Low]
This isn't a bug, but the reStructuredText title underline is shorter than
the title text itself. In Sphinx, an underline must be at least as long as
the text to avoid build warnings. There are a few other headers in this
file with the same issue.

> +
> +DAMON-based huge page collapsing (DAMON_HUGEPAGE) is a static kernel module
> +that aimed to collapse hot regions into huge pages.

[Severity: Low]
This isn't a bug, but there's a grammatical error here: "that aimed to
collapse" should probably be "that aims to collapse".

> +
> +Where Proactive huge page collapsing is Required?
> +========================================
> +
> +The amount of available memory grows faster than the amount of TLB entries.
> +This leads to higher amount of TLB misses and excesive cycle wastes. Huge

[Severity: Low]
This isn't a bug, but there's a typo here: "excesive".

[ ... ]

> +commit_inputs
> +-------------
> +
> +Make DAMON_HUGEPAGE reads the input parameters again, except ``enabled``.
> +
> +Input parameters that updated while DAMON_HUGEPAGE is running are not applied
> +by default.  Once this parameter is set as ``Y``, DAMON_HUGEPAGE reads values
> +of parametrs except ``enabled`` again.  Once the re-reading is done, this

[Severity: Low]
This isn't a bug, but there's a typo here: "parametrs".

[ ... ]

> +quota_reset_interval_ms
> +-----------------------
> +
> +The time/size quota charge reset interval in milliseconds.
> +
> +The charget reset interval for the quota of time (quota_ms) and size

[Severity: Low]
This isn't a bug, but there's a typo here: "charget".

> +(quota_sz).  That is, DAMON_HUGEPAGE does not try collapsing for more than
> +quota_ms milliseconds or quota_sz bytes within quota_reset_interval_ms
> +milliseconds.

[Severity: Low]
Is there a missing documentation section for the quota_sz parameter? It is
explicitly mentioned here and used in the Example section later, but there
is no dedicated documentation block for it under the Interface: Module
Parameters section.

[ ... ]

> +quota_percentage_hugepage
> +----------------
> +
> +Huge page consumption to total memory anonymous memory consumption ratio goal

[Severity: Low]
This isn't a bug, but "total memory anonymous memory consumption" is
repetitive and slightly confusing.

> +in bp ``(10,000)``. DAMON_HUGEPAGE automatically increases and decreases page
> +collapse aggressiveness in order to achieve the given value.
> +
> +sample_interval
> +---------------
> +
> +Sampling interval for the monitoring in microseconds.
> +
> +The sampling interval of DAMON for the cold memory monitoring.  Please refer to
> +the DAMON documentation (:doc:`usage`) for more detail.

[Severity: Medium]
Are these references to cold memory monitoring intentional?

The commit message and earlier documentation indicate this module tracks
hot regions for collapsing. It appears the descriptions for sample_interval,
aggr_interval, min_nr_regions, and max_nr_regions were copied from
DAMON_RECLAIM and still refer to cold memory monitoring.

[ ... ]

> +addr_unit
> +---------
> +
> +A scale factor for memory addresses and bytes.
> +
> +This parameter is for setting and getting the :ref:`address unit
> +<damon_design_addr_unit>` parameter of the DAMON instance for DAMON_HUGEPAGE.
> +
> +``monitor_region_start`` and ``monitor_region_end`` should be provided in this
> +unit.  For example, let's suppose ``addr_unit``, ``monitor_region_start`` and
> +``monitor_region_end`` are set as ``1024``, ``0`` and ``10``, respectively.
> +Then DAMON_HUGEPAGE will work for 10 KiB length of physical address range that
> +starts from address zero (``[0 * 1024, 10 * 1024)`` in bytes).

[Severity: Medium]
Should this refer to a physical address range?

Since this module monitors a process via monitored_pid, it operates on
virtual memory rather than physical addresses. This also appears to be a
leftover from copying another DAMON module's documentation.

[ ... ]

> +Example
> +=======
> +
> +Below runtime example commands make DAMON_HUGEPAGE to find memory regions of
> +the task with PID 1234 that have been accessed in the last 100 millseconds or

[Severity: Low]
This isn't a bug, but there's a typo here: "millseconds".

> +more and pages out. The pagecollapsing is limited to be done only up to 1 GiB

[Severity: Medium]
Is the phrase "and pages out" a leftover copy-paste error from
DAMON_RECLAIM?

[Severity: Low]
This isn't a bug, but there's a typo here: "pagecollapsing".

> +per second to avoid DAMON_HUGEPAGE consuming too much CPU time for the collapse
> +operation. ::

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522145518.158910-1-gutierrez.asier@huawei-partners.com?part=4