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

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 > totalr=
am incorrectly forces maximum scheme aggressiveness instead of reporting 10=
0% free memory.
- [Medium] The commit message contradicts the implementation, and the uncon=
ditional return of 10000 for zero totalram breaks metric semantics.

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

--- Patch [7]: [RFC PATCH v1.1 7/9] mm/damon/core: handle extreme memory st=
ate in damon_get_node_mem_bp() ---
commit 05d27d4d51952669ff23ec9285ccb83a273556ad
Author: SJ Park <[email protected]>

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/[email protected]

> 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 ret=
urns
10000.=20

Does this unconditional return contradict the intent documented in the comm=
it
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(
>  	}
> =20
>  	si_meminfo_node(&i, goal->nid);
> -	if (goal->metric =3D=3D 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=3D1,
freeram=3D0 would yield)?

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

> +	if (goal->metric =3D=3D DAMOS_QUOTA_NODE_MEM_USED_BP) {
>  		numerator =3D 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 =3D 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.=20

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 fr=
ee
memory goal, causing it to wastefully and drastically increase the reclamat=
ion
budget (e.g., pageout/demotion) on a node that is already completely free?

Should this return 10000 (100% free) instead?

>  		numerator =3D 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 =3D=3D 0, does the s=
ame
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 =3D=3D DAMOS_QUOTA_SOME_MEM_USED_BP)
        return mult_frac(active, 10000, total);
    return mult_frac(inactive, 10000, total);
}

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260802162631.9030=
[email protected]?part=3D7