Re: [PATCH v3 2/3] mm/damon/sysfs: support hugepage_mem_bp quota goal metric

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Integer underflow when computing 'total' in `damos_hugepage_mem_bp` causes DAMON to become maximally aggressive on highly idle systems.
- [Medium] The new `hugepage_mem_bp` sysfs metric is missing from the DAMON sysfs selftest script.
- [Low] Missing documentation for the `hugepage_mem_bp` metric in the DAMON design guide.
--

--- Patch [2]: [PATCH v3 2/3] mm/damon/sysfs: support hugepage_mem_bp quota goal metric ---
commit 304fd61bee6da0356569e9c8297451d15a43e10f
Author: Asier Gutierrez <[email protected]>

mm/damon/sysfs: support hugepage_mem_bp quota goal metric

This commit exposes the new DAMOS_QUOTA_HUGEPAGE_MEM_BP autotune policy
metric through the DAMON sysfs interface. It adds "hugepage_mem_bp" to
the sysfs-schemes interface.

> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> index 32f495a96b17a..d9b81d7b5910e 100644
> --- a/mm/damon/sysfs-schemes.c
> +++ b/mm/damon/sysfs-schemes.c
> @@ -1269,6 +1269,10 @@ struct damos_sysfs_qgoal_metric_name damos_sysfs_qgoal_metric_names[] = {
>  		.metric = DAMOS_QUOTA_NODE_ELIGIBLE_MEM_BP,
>  		.name = "node_eligible_mem_bp",
>  	},
> +	{
> +		.metric = DAMOS_QUOTA_HUGEPAGE_MEM_BP,
> +		.name = "hugepage_mem_bp",
> +	},
>  };

[Severity: High]
While this patch exposes the new metric, is there a potential integer
underflow in the underlying implementation of damos_hugepage_mem_bp() in
mm/damon/core.c?

Due to per-CPU vmstat counter drift, pending positive deltas from freed
memory can cause the global NR_FREE_PAGES count to temporarily exceed the
fixed totalram_pages(). In damos_hugepage_mem_bp(), total is computed as:

    total = totalram_pages() - global_zone_page_state(NR_FREE_PAGES);

On a highly idle system, this could cause the unsigned subtraction to
underflow, resulting in a value near ULONG_MAX. The subsequent !total check
would fail, and mult_frac(thp, 10000, total) would evaluate to 0 due to the
enormous denominator.

Would this score of 0 lead damon_feed_loop_next_input() to perceive a maximal
divergence from the goal, forcing the scheme to maximize its quota
aggressiveness instead of minimizing it?

[Severity: Medium]
Should tools/testing/selftests/damon/sysfs.sh be updated to include the new
metric?

The test_goal() function validates writes to target_metric for all existing
metrics:

    ensure_write_succ "$fpath" "active_mem_bp" "valid input"
    ensure_write_succ "$fpath" "inactive_mem_bp" "valid input"
    ensure_write_succ "$fpath" "node_eligible_mem_bp" "valid input"

Could hugepage_mem_bp be added to this sequence to ensure the sysfs interface
is regression tested?

[Severity: Low]
Does this new metric need to be documented in
Documentation/mm/damon/design.rst?

The documentation lists all available target_metric values, such as
inactive_mem_bp and node_eligible_mem_bp, but it appears hugepage_mem_bp
is missing.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2