Re: [PATCH v3 1/3] mm/damon: Introduce DAMOS_QUOTA_HUGEPAGE auto tuning

On Mon, 20 Jul 2026 12:16:57 +0000 [email protected] wrote:

> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> 
> New issues:
> - [High] Unsigned underflow in `total` bypasses the division-by-zero guard, causing `damos_hugepage_mem_bp` to return 0 instead of the intended safeguard `INT_MAX`.
> - [Medium] The new `DAMOS_QUOTA_HUGEPAGE_MEM_BP` metric is missing from the sysfs parser mapping array, rendering the feature completely inaccessible to userspace.
> - [Medium] 32-bit integer overflow in `mult_frac()` yields drastically incorrect ratios when the number of huge pages exceeds ~1.7GB.
> 
> Pre-existing issues:
> - [High] Preexisting division by zero in `damos_get_in_active_mem_bp` when LRU lists are empty.
> --
> 
> --- Patch [1]: [PATCH v3 1/3] mm/damon: Introduce DAMOS_QUOTA_HUGEPAGE auto tuning ---
> commit 8d24f093924a5075fd67c8f7808eada2166af60d
> Author: Asier Gutierrez <[email protected]>
> 
> mm/damon: Introduce DAMOS_QUOTA_HUGEPAGE auto tuning
> 
> Introduce DAMOS_QUOTA_HUGEPAGE_MEM_BP auto tuning and add a new DAMOS
> quota goal metric to measure huge page consumption. Add a guard to avoid
> division by 0 when vmstat lags.
> 
> Link: https://lore.kernel.org/all/[email protected]/
> 
> > diff --git a/include/linux/damon.h b/include/linux/damon.h
> > --- a/include/linux/damon.h
> > +++ b/include/linux/damon.h
> [ ... ]
> > @@ -167,6 +168,7 @@ enum damos_quota_goal_metric {
> >  	DAMOS_QUOTA_ACTIVE_MEM_BP,
> >  	DAMOS_QUOTA_INACTIVE_MEM_BP,
> >  	DAMOS_QUOTA_NODE_ELIGIBLE_MEM_BP,
> > +	DAMOS_QUOTA_HUGEPAGE_MEM_BP,
> 
> [Severity: Medium]
> Is this feature accessible from userspace?
> 
> If a user writes hugepage_mem_bp to the target_metric sysfs file,
> target_metric_store() iterates over the damos_sysfs_qgoal_metric_names
> array to find a match.
> 
> Because DAMOS_QUOTA_HUGEPAGE_MEM_BP is missing from that array in
> mm/damon/sysfs-schemes.c, will the kernel always reject the user's input
> with -EINVAL?

No issue.  The next patch will make it accessible.

> 
> >  	NR_DAMOS_QUOTA_GOAL_METRICS,
> >  };
> >  
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > --- 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);
> 
> [Severity: High]
> This is a pre-existing issue, but can this divide by zero?
> 
> In damos_get_in_active_mem_bp(), total is used as the divisor without a zero
> check. If all memory is forced onto the unevictable list (e.g. via
> mlockall()), or if the system is completely idle with only kernel allocations,
> active + inactive can be zero.
> 
> If DAMOS_QUOTA_ACTIVE_MEM_BP is used in such a state, would mult_frac()
> trigger a hardware divide-by-zero exception and panic the system?

No blocker of this patch.

I will work on this.

> 
> >  }
> >  
> > +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);
> > +
> > +	/*
> > +	 * 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: High]
> Can total underflow here?
> 
> Because global_zone_page_state(NR_FREE_PAGES) relies on vmstat, which uses
> per-CPU caching, the global NR_FREE_PAGES counter can temporarily exceed
> totalram_pages() if some CPUs have flushed frees while others have pending
> allocations.
> 
> If this happens, total (which is unsigned long) underflows to a value near
> ULONG_MAX. Could this bypass the !total guard and pass a massive integer
> to mult_frac(), which then incorrectly returns 0?

Maybe better to address correctly.  But the user impact is too trivial to block
this patch.  No blocker in my opinion.

> 
> > +	return mult_frac(thp, 10000, total);
> 
> [Severity: Medium]
> Will this overflow on 32-bit architectures?
> 
> mult_frac() evaluates the intermediate multiplication using the types of its
> arguments. Since thp is an unsigned long, evaluating thp * 10000 uses 32-bit
> arithmetic on 32-bit systems like ARM LPAE.
> 
> If the system has more than 429,496 base pages of THP (approximately 1.7GB),
> will this silently wrap around and yield drastically incorrect quota goal
> ratios?

No blocker.  Known low priority issue.

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


Thanks,
SJ