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

On Sun, 02 Aug 2026 16:35:44 +0000 [email protected] wrote:

> 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 <[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 returns
> 10000. 
> 
> Does this unconditional return contradict the intent documented in the commit
> message?

Good catch, I will update 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%?

I made mistakes at revisioning.  I will fix this.

> 
> > +	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;
>     ...
> }

The intention of this revision was handling this, but I made a complete
mistake.  I will fix.

> 
> > -	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?

That depends on the perspective.  I think 0% makes sense.

> 
> >  		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);
> }

Later patches in this series fix those.

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


Thanks,
SJ