Re: [RFC PATCH v1.1 7/9] mm/damon/core: handle extreme memory state in damon_get_node_mem_bp()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Returning 0 for DAMOS_QUOTA_NODE_MEM_FREE_BP when freeram > totalram incorrectly forces maximum scheme aggressiveness instead of reporting 100% free memory.
- [Medium] The commit message contradicts the implementation, and the unconditional return of 10000 for zero totalram breaks metric semantics.

Pre-existing issues:
- [High] Unhandled integer underflow in MEM_USED_BP and MEMCG_FREE_BP calculations when counters drift.
- [High] The patch fixes a potential divide-by-zero in damos_get_node_mem_bp() but misses identical unvalidated divisions in related functions.
--

--- Patch [7]: [RFC PATCH v1.1 7/9] mm/damon/core: handle extreme memory state in damon_get_node_mem_bp() ---
commit 05d27d4d51952669ff23ec9285ccb83a273556ad
Author: SJ Park <sj@kernel.org>

mm/damon/core: handle extreme memory state in damon_get_node_mem_bp()

In an extreme and unlikely situation, si_meminfo_node() might let the
caller show zero total ram. That could cause a divide by zero in
damon_get_node_mem_bp(). Fix it by setting the totalram one byte in the
case.

Link: https://lore.kernel.org/20260328133216.9697-1-sj@kernel.org

> In an extreme and unlikely situation, si_meminfo_node() might let the
> caller show zero total ram.  That could cause a divide by zero in
> damon_get_node_mem_bp().  Fix it by setting the totalram one byte in the
> case.

[Severity: Medium]
The commit message states "Fix it by setting the totalram one byte in the
case."

However, the implementation actually bypasses the metric type check and returns
10000. 

Does this unconditional return contradict the intent documented in the commit
message?

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 3bdbf4fbf7147..e3f3ee75a3d33 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -2816,10 +2816,16 @@ static __kernel_ulong_t damos_get_node_mem_bp(
>  	}
>  
>  	si_meminfo_node(&i, goal->nid);
> -	if (goal->metric == DAMOS_QUOTA_NODE_MEM_USED_BP)
> +	if (!i.totalram)
> +		return 10000;

[Severity: Medium]
If the metric is DAMOS_QUOTA_NODE_MEM_FREE_BP, shouldn't it report 0% free
rather than 100% free if the memory is absent (which is what totalram=1,
freeram=0 would yield)?

Does returning 10000 here break the semantic constraint that used + free = 100%?

> +	if (goal->metric == DAMOS_QUOTA_NODE_MEM_USED_BP) {
>  		numerator = i.totalram - i.freeram;

[Severity: High]
This is a pre-existing issue, but if counter drift results in
freeram > totalram, does this subtraction silently underflow, producing a
massive unsigned value?

If this is passed to mult_frac(), could it generate a huge metric value,
leading the auto-tuner to mistakenly act as if the memory is massively
overused?

An identical unhandled underflow also appears to exist in
damos_get_node_memcg_used_bp() when evaluating DAMOS_QUOTA_NODE_MEMCG_FREE_BP
if used_pages exceeds totalram:

mm/damon/core.c:damos_get_node_memcg_used_bp() {
    ...
    numerator = i.totalram - used_pages;
    ...
}

> -	else	/* DAMOS_QUOTA_NODE_MEM_FREE_BP */
> +	} else {
> +		/* DAMOS_QUOTA_NODE_MEM_FREE_BP */
> +		if (i.totalram < i.freeram)
> +			return 0;

[Severity: High]
When counter drift causes freeram > totalram, the node effectively has
~100% free memory. 

By returning 0 (0% free memory) in this scenario, doesn't the DAMON quota
auto-tuning feedback loop interpret this as severely under-achieving the free
memory goal, causing it to wastefully and drastically increase the reclamation
budget (e.g., pageout/demotion) on a node that is already completely free?

Should this return 10000 (100% free) instead?

>  		numerator = i.freeram;
> +	}
>  	return mult_frac(numerator, 10000, i.totalram);
>  }

[Severity: High]
This is a pre-existing issue, but while this patch addresses a potential
divide-by-zero when si_meminfo_node() reports totalram == 0, does the same
unvalidated division exist in related functions?

For example, damos_get_node_memcg_used_bp() divides by i.totalram without a
zero-check:

mm/damon/core.c:damos_get_node_memcg_used_bp() {
    ...
    si_meminfo_node(&i, goal->nid);
    ...
    return mult_frac(numerator, 10000, i.totalram);
}

Similarly, damos_get_in_active_mem_bp() divides by total (active + inactive)
via mult_frac() without checking if total is zero:

mm/damon/core.c:damos_get_in_active_mem_bp() {
    ...
    if (goal->metric == DAMOS_QUOTA_SOME_MEM_USED_BP)
        return mult_frac(active, 10000, total);
    return mult_frac(inactive, 10000, total);
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260802162631.90304-1-sj@kernel.org?part=7