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

[SeongJae Park <sj@kernel.org>]

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?

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

[ ... ]
> 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?

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

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