Re: (Sashiko) Re: [RFC v2] mm/damon: add synchronous validation for commit_inputs

[SeongJae Park <sj@kernel.org>]

On Sat, 21 Mar 2026 10:06:22 -0700 SeongJae Park <sj@kernel.org> wrote:

> Forwarding Sashiko review for doing the discussion on the mailing list.
> 
> # review url: https://sashiko.dev/#/patchset/20260321140926.22163-1-aethernet65535@gmail.com
> # start of sashiko.dev inline review
> commit 05e8b12e4fe302bfaf2b30ee9c0d38b036fc1c5e
> Author: Liew Rui Yan <aethernet65535@gmail.com>
> Subject: mm/damon: add synchronous validation for commit_inputs
> 
> This commit adds synchronous validation to the sysfs commit_inputs parameter
> for the DAMON lru_sort and reclaim modules. It aims to return immediate
> feedback to userspace if the provided monitoring attributes are invalid,
> rather than failing silently in the background.
> 
> Link: https://lore.kernel.org/20260318153731.97470-1-aethernet65535@gmail.com
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index db6c67e52d2b..22136486e745 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -751,6 +751,27 @@ static bool damon_valid_intervals_goal(struct damon_attrs *attrs)
> >  	return true;
> >  }
> >  
> > +/**
> > + * damon_validate_attrs() - Check if the given DAMON attributes are valid.
> > + * @attrs:		attributes to be checked
> > + *
> > + * Return: 0 if valid, negative error code otherwise.
> > + */
> > +int damon_validate_attrs(struct damon_attrs *attrs)
> > +{
> > +	if (!damon_valid_intervals_goal(attrs))
> > +		return -EINVAL;
> > +	if (attrs->min_nr_regions < 3)
> > +		return -EINVAL;
> > +	if (attrs->min_nr_regions > attrs->max_nr_regions)
> > +		return -EINVAL;
> > +	if (attrs->sample_interval > attrs->aggr_interval)
> > +		return -EINVAL;
> 
> Does this validation fully prevent silent asynchronous failures?
> 
> If a user writes 0 to sample_interval or aggr_interval, the check 
> attrs->sample_interval > attrs->aggr_interval will pass (since 0 > 0 is
> false), returning success to the shell.
> 
> Since the daemon's actual application logic requires non-zero intervals,
> will this still fail silently in the background for zero values?

Zero sample_interval and aggr_interval are valid DAMON parameters.  DAMON logic
is written to allow that.  If not, that's a bug or wrong design.  Please let me
know if you find that.  So this should be fine.

> 
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(damon_validate_attrs);
> 
> Is this EXPORT_SYMBOL() necessary? 
> 
> Since both DAMON_RECLAIM and DAMON_LRU_SORT are built into the kernel and
> cannot be compiled as loadable modules, does this need to be exported to the
> global symbol table? None of the other DAMON core functions in this file
> appear to be exported for this reason.

I don't think it is necessary.  I also suggested to just not expose this
function to in-kernel DAMON core API callers in the high level review.

> 
> [ ... ]
> > diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
> > index 554559d72976..883860721d3b 100644
> > --- a/mm/damon/lru_sort.c
> > +++ b/mm/damon/lru_sort.c
> [ ... ]
> > @@ -361,6 +360,39 @@ static int damon_lru_sort_handle_commit_inputs(void)
> >  	return err;
> >  }
> >  
> > +static int damon_lru_sort_commit_inputs_store(const char *val,
> > +		const struct kernel_param *kp)
> > +{
> > +	struct damon_attrs attrs;
> > +	bool yes;
> > +	int err;
> > +
> > +	err = kstrtobool(val, &yes);
> > +	if (err)
> > +		return err;
> > +
> > +	if (commit_inputs == yes)
> > +		return 0;
> > +
> > +	if (!yes)
> > +		return 0;
> 
> If a user writes 'N' to the sysfs parameter, yes evaluates to false and this
> returns success without updating commit_inputs.
> 
> If commit_inputs was previously 'Y' (waiting for kdamond to process it),
> does this prevent administrators from aborting a pending parameter commit?
> 
> > +
> > +	attrs = damon_lru_sort_mon_attrs;
> > +	err = damon_validate_attrs(&attrs);
> 
> Will this synchronous validation reject valid configurations if
> autotune_monitoring_intervals is enabled?
> 
> The actual parameter application logic in damon_lru_sort_apply_parameters()
> overwrites sample_interval and aggr_interval with hardcoded defaults when
> autotuning is on. 
> 
> Because this validation happens before those overrides are applied, will a
> user who leaves sample_interval > aggr_interval receive a false -EINVAL error
> even though the configuration would be safely overridden?

I haven't think thoroughly about this.  But I think synchronously doing not
only validation but also commit, as I suggested on the high level review will
solve these complex problems.

> 
> > +	if (err)
> > +		return err;
> > +
> > +	commit_inputs = yes;
> > +	return err;
> > +}
> [ ... ]
> > diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
> > index 86da14778658..6ea44b02cc76 100644
> > --- a/mm/damon/reclaim.c
> > +++ b/mm/damon/reclaim.c
> [ ... ]
> > @@ -267,6 +266,39 @@ static int damon_reclaim_handle_commit_inputs(void)
> >  	return err;
> >  }
> >  
> > +static int damon_reclaim_commit_inputs_store(const char *val,
> > +		const struct kernel_param *kp)
> > +{
> > +	struct damon_attrs attrs;
> > +	bool yes;
> > +	int err;
> > +
> > +	err = kstrtobool(val, &yes);
> > +	if (err)
> > +		return err;
> > +
> > +	if (commit_inputs == yes)
> > +		return 0;
> > +
> > +	if (!yes)
> > +		return 0;
> 
> Similar to the lru_sort module, does returning 0 here without clearing
> commit_inputs prevent users from aborting a pending commit by writing 'N'?

Again, I believe doing both validation and commit synchronously will eliminate
these complexity.

> 
> > +
> > +	attrs = damon_reclaim_mon_attrs;
> > +	err = damon_validate_attrs(&attrs);
> > +	if (err)
> > +		return err;
> > +
> > +	commit_inputs = yes;
> > +	return err;
> > +}
> [ ... ]
> 
> 
> # end of sashiko.dev inline review
> # review url: https://sashiko.dev/#/patchset/20260321140926.22163-1-aethernet65535@gmail.com

FYI,

hkml [1] generated a draft of this mail using below command:

    hkml patch sashiko_dev --for_forwarding 20260321140926.22163-1-aethernet65535@gmail.com

[1] https://github.com/sjp38/hackermail


Thanks,
SJ