Re: [PATCH 1/2] mm/damon/reclaim: add autotune_monitoring_intervals parameter

[SeongJae Park <sj@kernel.org>]

On Fri, 01 May 2026 01:30:43 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Validation check on `damon_reclaim_mon_attrs.aggr_interval` prematurely rejects configurations when autotuning is enabled, and exposes a TOCTOU divide-by-zero panic.

No blocker for this patch is found.  Plese read below for more details.

> --
> 
> commit 34f815f9177f234892c1b9608e13d2fc0957697e
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/reclaim: add autotune_monitoring_intervals parameter
> 
> This commit adds a module parameter for utilizing DAMON's auto-tuning
> feature with suggested default setups. When enabled, this feature
> overrides manually entered monitoring intervals to capture a meaningful
> amount of access events per snapshot.
> 
> > diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
> > index a60ee800d63e9..7126d47fb8b2f 100644
> > --- a/mm/damon/reclaim.c
> > +++ b/mm/damon/reclaim.c
> 
> [ ... ]
> 
> > @@ -201,12 +215,21 @@ static int damon_reclaim_apply_parameters(void)
> >  		goto out;
> >  	}
> >  
> > -	err = damon_set_attrs(param_ctx, &damon_reclaim_mon_attrs);
> > +	attrs = damon_reclaim_mon_attrs;
> > +	if (autotune_monitoring_intervals) {
> > +		attrs.sample_interval = 5000;
> > +		attrs.aggr_interval = 100000;
> > +		attrs.intervals_goal.access_bp = 40;
> > +		attrs.intervals_goal.aggrs = 3;
> > +		attrs.intervals_goal.min_sample_us = 5000;
> > +		attrs.intervals_goal.max_sample_us = 10 * 1000 * 1000;
> > +	}
> 
> In the existing code just above this diff, there is a validation check:
> 
>     if (!damon_reclaim_mon_attrs.aggr_interval) {
>         err = -EINVAL;
>         goto out;
>     }
> 
> If autotune_monitoring_intervals is enabled, could this check prematurely
> reject the configuration if damon_reclaim_mon_attrs.aggr_interval is 0?
> The auto-tuning override happens right below the check.

Yes, and this is a behavior that consistent to that of DAMON_LRU_SORT.  I
didn't really intentionally designed this behavior but I find no problem at
this.  Zero aggregation interval is invalid.  Just keeping it as invalid even
if autotune is enabled sounds natural to me.

> 
> Additionally, because this check evaluates the global module parameter
> directly instead of a local copy, could a concurrent sysfs write cause a
> divide-by-zero panic? 
> 
> If a user sets the global interval to 0 right after this check passes:
> 
>     if (!damon_reclaim_mon_attrs.aggr_interval) {
>         // passes
>     }
>     
>     // concurrent sysfs write sets damon_reclaim_mon_attrs.aggr_interval = 0
>     
>     attrs = damon_reclaim_mon_attrs;
> 
> If autotune_monitoring_intervals is false, attrs.aggr_interval becomes 0
> and is passed to damon_reclaim_new_scheme(), which performs:
> 
>     .min_age_region = min_age / aggr_interval,
> 
> Would moving this check to evaluate the local attrs.aggr_interval after
> the auto-tuning block prevent both of these scenarios?

No.

First of all, this patch is doing nothing for the code.

And damon_reclaim_apply_parameters() does not race against itself, since it
works synchronous to the parameters update, and the parameter updates are
protected by kernel_param_lock.


Thanks,
SJ

[...]