Re: [RFC v5] mm/damon: add synchronous commit for commit_inputs

Liew Rui Yan
2026-03-26T06:16:55Z

On Wed, 25 Mar 2026 07:29:20 -0700 SeongJae Park <sj@kernel.org> wrote:

> On Wed, 25 Mar 2026 09:39:39 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
>
> > Problem
> > =======
> > Writing invalid parameters to sysfs followed by 'commit_inputs=Y' fails
> > silently (no error returned to shell), because the validation happens
> > asynchronously in the kdamond.
> >
> > Solution
> > ========
> > To fix this, the commit_inputs_store() callback now uses damon_call() to
> > synchronously commit parameters in the kdamond thread's safe context.
> > This ensures that validation errors are returned immediately to
> > userspace, following the pattern used by DAMON_SYSFS.
> >
> > Changes
> > =======
> > 1. Added commit_inputs_store() and commit_inputs_fn() to commit
> > synchronously.
> > 2. Removed handle_commit_inputs().
> >
> > This change is motivated from another discussion [1].
> >
> > [1] https://lore.kernel.org/20260318153731.97470-1-aethernet65535@gmail.com
> >
> > Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com>
> > ---
> > Changes from RFC-v4:
> > - Rename the 'yes' variable in commit_inputs_store() to the more
> > understandable 'commit_inputs_request'.
> > - Return -EBUSY instead of -EINVAL when 'commit_inputs' is triggered
> > while kdamond is not running.
> > - Link to RFC-v4: https://lore.kernel.org/20260323021648.6590-1-aethernet65535@gmail.com
> >
> > Changes from RFC-v3:
> > - Added checks for 'ctx' and 'damon_is_running()' to prevent NULL
> > pointer dereference during early boot. (Found by Sashiko.dev)
> > - Removed handle_commit_inputs() and its associated polling logic as
> > they have become dead code after moving to the synchronous damon_call()
> > approach.
> > - Ensure the 'commit_inputs' is properly updated.
> > Link to RFC-v3: https://lore.kernel.org/20260322231522.32700-1-aethernet65535@gmail.com
> >
> > Changes from RFC-v2:
> > - Removed damon_validate_attrs(), now using damon_commit_ctx() for
> > synchronous validation in the kdamond context.
> > - Following DAMON_SYSFS pattern for synchronous commit via damon_call().
> > - Link to RFC-v2: https://lore.kernel.org/20260321140926.22163-1-aethernet65535@gmail.com
> >
> > Changes from RFC-v1:
> > - Remove question from commit message area.
> > - Added synchronous validation for DAMON_RECLAIM.
> > - Rename damon_valid_attrs() -> damon_validate_attrs().
> > - Exported a new function damon_validate_attrs() and declared it in
> > damon.h.
> > - Link to RFC-v1: https://lore.kernel.org/20260321002642.22712-1-aethernet65535@gmail.com
> >
> > mm/damon/lru_sort.c | 54 +++++++++++++++++++++++++++++++++++++++------
> > mm/damon/reclaim.c | 54 +++++++++++++++++++++++++++++++++++++++------
> > 2 files changed, 94 insertions(+), 14 deletions(-)
> >
> > diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
> > index 554559d72976..a2410f648b51 100644
> > --- a/mm/damon/lru_sort.c
> > +++ b/mm/damon/lru_sort.c
> > @@ -39,7 +39,6 @@ static bool enabled __read_mostly;
> > * the re-reading, DAMON_LRU_SORT will be disabled.
> > */
> > static bool commit_inputs __read_mostly;
> > -module_param(commit_inputs, bool, 0600);
> >
> > /*
> > * Desired active to [in]active memory ratio in bp (1/10,000).
> > @@ -349,18 +348,59 @@ static int damon_lru_sort_apply_parameters(void)
> > return err;
> > }
> >
> > -static int damon_lru_sort_handle_commit_inputs(void)
> > +static int damon_lru_sort_commit_inputs_fn(void *arg)
> > {
> > + return damon_lru_sort_apply_parameters();
> > +}
> > +
> > +static int damon_lru_sort_commit_inputs_store(const char *val,
> > + const struct kernel_param *kp)
> > +{
> > + bool commit_inputs_request;
> > int err;
> > + struct damon_call_control control = {
> > + .fn = damon_lru_sort_commit_inputs_fn,
> > + .data = ctx,
>
> You don't really use the data in the callback function. Why don't you just
> unset it?

You are right, thank you for pointing this out. I will remove the
unnecessary assignment in the next version.

> > + .repeat = false,
> > + };
> >
> > - if (!commit_inputs)
> > + err = kstrtobool(val, &commit_inputs_request);
> > + if (err)
> > + return err;
> > +
> > + if (!commit_inputs_request)
> > return 0;
> >
> > - err = damon_lru_sort_apply_parameters();
> > - commit_inputs = false;
> > - return err;
> > + /*
> > + * Skip damon_call() during early boot or when kdamond is
> > + * not running to avoid NULL pointer dereference.
> > + */
> > + if (!ctx)
> > + return -EBUSY;
> > +
> > + err = damon_call(ctx, &control);
> > + if (err) {
> > + /*
> > + * If kdamond is not running, damon_call() returns -EINVAL.
> > + * Remap it to -EBUSY to better reflect that the operation
> > + * failed due to the current state of the system.
> > + */
> > + if (err == -EINVAL && !damon_is_running(ctx))
> > + return -EBUSY;
>
> In my opinion, EINVAL is a better return value. It was failed not because
> something is busy but just because DAMON is not running. Committing something
> to DAMON while DAMON is not running seems invalid operation to me.

That makes sense. I will revert this to return -EINVAL when DAMON is not
running.

Best regards,
Rui Yan