Re: [PATCH 7/8] mm/damon/core: handle extreme memory state in get_node_memcg_used_bp()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Integer overflow in basis point calculation on 32-bit systems for large memory sizes.
- [High] Incorrect saturation logic for negative global memory utilization triggers false maximum reclaim.
--

commit 23b94672ac5d1c09d669ade5732261e82d129c96
Author: SJ Park <sj@kernel.org>

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

This commit handles corner cases where total memory is zero or
lruvec_page_state returns more used memory than total memory. It returns
100% or 0% for used and free memory queries respectively in these scenarios.

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

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index ad3657d356fbc..3060edf5e4fa6 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -2854,6 +2854,12 @@ static unsigned long damos_get_node_memcg_used_bp(
>  	mem_cgroup_put(memcg);
>  
>  	si_meminfo_node(&i, goal->nid);
> +	if (!i.totalram || i.totalram < used_pages) {
> +		if (goal->metric == DAMOS_QUOTA_NODE_MEMCG_USED_BP)
> +			return 10000;
> +		else	/* DAMOS_QUOTA_NODE_MEMCG_FREE_BP */
> +			return 0;
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but looking at similar logic in
damos_get_node_mem_bp():

    if (!i.totalram || i.totalram < i.freeram) {
        if (goal->metric == DAMOS_QUOTA_NODE_MEM_USED_BP)
            return 10000;
        else    /* DAMOS_QUOTA_NODE_MEM_FREE_BP */
            return 0;
    }

Can this regression trigger unwarranted memory reclaim pressure? If transient
per-CPU stats drift causes i.freeram to slightly exceed i.totalram, this
condition evaluates to true.

If free memory exceeds total memory, it implies used memory is less than 0.
Should the correct saturation be 0% used and 100% free instead? Returning
10000 (100%) for used memory tells DAMON that the empty node is completely
full, which might cause aggressive reclaim behavior on mostly empty nodes.

>  	if (goal->metric == DAMOS_QUOTA_NODE_MEMCG_USED_BP)
>  		numerator = used_pages;
>  	else	/* DAMOS_QUOTA_NODE_MEMCG_FREE_BP */

[ ... ]

[Severity: High]
This is a pre-existing issue, but does this basis point calculation overflow
on 32-bit systems for large memory sizes?

A little further down in damos_get_node_memcg_used_bp() (and similarly in
damos_get_node_mem_bp()):

    return mult_frac(numerator, 10000, i.totalram);

The mult_frac macro evaluates its internal remainder multiplication using
the type of the numerator. On 32-bit systems, the numerator is a 32-bit
unsigned long.

If the numerator is greater than 429,496 (which corresponds to 1.75 GB of
RAM with 4K pages), the multiplication by 10000 wraps around ULONG_MAX. For
instance, 429,497 * 10000 modulo 2^32 evaluates to 2704, which then divided
by total RAM evaluates to near 0%.

Could this regression lead to incorrect memory quota enforcement for memcgs
or nodes exceeding this threshold?

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