Re: [PATCH v3 3/3] Docs/mm/damon/design: Document hugepage_mem_bp target metric

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Underflow in total used memory calculation causes division by a huge number, breaking the intended safeguard and leading to aggressive DAMOS actions.
- [Low] Returning INT_MAX from damos_hugepage_mem_bp() causes an integer overflow when computing the quota score on 32-bit architectures.
- [Low] The newly added 'hugepage_mem_bp' sysfs metric is missing from the automated sysfs selftests.
--

--- Patch [3]: [PATCH v3 3/3] Docs/mm/damon/design: Document hugepage_mem_bp target metric ---
commit 8b5f375e8a76a103dce1c4b0a9e133966c306366
Author: Asier Gutierrez <gutierrez.asier@huawei-partners.com>

Docs/mm/damon/design: Document hugepage_mem_bp target metric
    
Document hugepage_mem_bp metric exposed by sysfs.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index e0122376f43d4..e38f2ddebb341 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -3007,6 +3007,24 @@ static unsigned int damos_get_in_active_mem_bp(bool active_ratio)
>  	return mult_frac(inactive, 10000, total);
>  }
>  
> +static unsigned int damos_hugepage_mem_bp(void)
> +{
> +	unsigned long thp, total;
> +
> +	thp = global_node_page_state(NR_ANON_THPS) +
> +				global_node_page_state(NR_SHMEM_THPS) +
> +				global_node_page_state(NR_FILE_THPS);
> +	total = totalram_pages() - global_zone_page_state(NR_FREE_PAGES);

[Severity: High]
Does this calculation handle vmstat drift correctly when free pages temporarily
exceed total RAM?

Due to per-CPU negative deltas on mostly idle systems,
global_zone_page_state(NR_FREE_PAGES) can temporarily exceed totalram_pages().
If this happens, the unsigned subtraction will underflow to a massive value
instead of zero.

This would bypass the !total check and cause mult_frac() to divide by the
massive value, resulting in 0. Could this false 0% hugepage usage cause DAMOS
to aggressively apply actions when it should be pausing them?

> +
> +	/*
> +	 * vmstat may lag in idle systems and lead to free pages equaling to
> +	 * the total RAM in the system.
> +	 */
> +	if (!total)
> +		return INT_MAX;

[Severity: Low]
Can returning INT_MAX here cause an integer overflow in damos_quota_score()?

If total is exactly 0, returning INT_MAX means it will be multiplied by 10000
in damos_quota_score():

    score = mult_frac(goal->current_value, 10000, goal->target_value);

On 32-bit architectures, INT_MAX * 10000 will overflow. While the wrapped value
might remain large enough to suppress DAMOS actions in practice, it breaks the
10000 basis point scale. Should a smaller maximum value be returned here to
avoid the overflow?

> +	return mult_frac(thp, 10000, total);
> +}

[ ... ]

> 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: Low]
Is hugepage_mem_bp missing from the automated sysfs selftests?

The tools/testing/selftests/damon/sysfs.sh script explicitly tests the other
valid target_metric strings:

    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"
    ensure_write_fail "$fpath" "foo" "invalid input"

Without an entry for hugepage_mem_bp, the new sysfs interface won't be verified
by the kselftests CI framework.

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