[PATCH] mm/damon: add synchronous commit for commit_inputs

[PATCH] mm/damon: add synchronous commit for commit_inputs

[Liew Rui Yan]

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-v6:
- Removed unnecessary assignment (repeat) in commit_inputs_store().
- Change the return value; if an error occurs, return the error code.
- Removed the RFC tag.
- Link to RFC-v6: https://lore.kernel.org/20260327062558.66392-1-aethernet65535@gmail.com

Changes from RFC-v5:
- Removed unnecessary assignment (data) in commit_inputs_store().
- Return -EINVAL instead of -EBUSY when 'commit_inputs' is triggered
  while kdamond is not running.
- Link to RFC-v5: https://lore.kernel.org/20260325013939.18167-1-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 | 41 ++++++++++++++++++++++++++++++++++-------
 mm/damon/reclaim.c  | 41 ++++++++++++++++++++++++++++++++++-------
 2 files changed, 68 insertions(+), 14 deletions(-)

diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
index 554559d72976..3af6854ba3da 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,46 @@ 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,
+	};
+
+	err = kstrtobool(val, &commit_inputs_request);
+	if (err)
+		return err;
 
-	if (!commit_inputs)
+	if (!commit_inputs_request)
 		return 0;
 
-	err = damon_lru_sort_apply_parameters();
-	commit_inputs = false;
-	return err;
+	/*
+	 * Skip damon_call() if ctx is not initialized to avoid
+	 * NULL pointer dereference.
+	 */
+	if (!ctx)
+		return -EINVAL;
+
+	err = damon_call(ctx, &control);
+
+	return err ? err : control.return_code;
 }
 
+static const struct kernel_param_ops commit_inputs_param_ops = {
+	.set = damon_lru_sort_commit_inputs_store,
+	.get = param_get_bool,
+};
+
+module_param_cb(commit_inputs, &commit_inputs_param_ops, &commit_inputs, 0600);
+
 static int damon_lru_sort_damon_call_fn(void *arg)
 {
 	struct damon_ctx *c = arg;
@@ -374,7 +401,7 @@ static int damon_lru_sort_damon_call_fn(void *arg)
 			damon_lru_sort_cold_stat = s->stat;
 	}
 
-	return damon_lru_sort_handle_commit_inputs();
+	return 0;
 }
 
 static struct damon_call_control call_control = {
diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
index 86da14778658..d8398161f091 100644
--- a/mm/damon/reclaim.c
+++ b/mm/damon/reclaim.c
@@ -39,7 +39,6 @@ static bool enabled __read_mostly;
  * re-reading, DAMON_RECLAIM will be disabled.
  */
 static bool commit_inputs __read_mostly;
-module_param(commit_inputs, bool, 0600);
 
 /*
  * Time threshold for cold memory regions identification in microseconds.
@@ -255,18 +254,46 @@ static int damon_reclaim_apply_parameters(void)
 	return err;
 }
 
-static int damon_reclaim_handle_commit_inputs(void)
+static int damon_reclaim_commit_inputs_fn(void *arg)
 {
+	return damon_reclaim_apply_parameters();
+}
+
+static int damon_reclaim_commit_inputs_store(const char *val,
+					     const struct kernel_param *kp)
+{
+	bool commit_inputs_request;
 	int err;
+	struct damon_call_control control = {
+		.fn = damon_reclaim_commit_inputs_fn,
+	};
 
-	if (!commit_inputs)
+	err = kstrtobool(val, &commit_inputs_request);
+	if (err)
+		return err;
+
+	if (!commit_inputs_request)
 		return 0;
 
-	err = damon_reclaim_apply_parameters();
-	commit_inputs = false;
-	return err;
+	/*
+	 * Skip damon_call() if ctx is not initialized to avoid
+	 * NULL pointer dereference.
+	 */
+	if (!ctx)
+		return -EINVAL;
+
+	err = damon_call(ctx, &control);
+
+	return err ? err : control.return_code;
 }
 
+static const struct kernel_param_ops commit_inputs_param_ops = {
+	.set = damon_reclaim_commit_inputs_store,
+	.get = param_get_bool,
+};
+
+module_param_cb(commit_inputs, &commit_inputs_param_ops, &commit_inputs, 0600);
+
 static int damon_reclaim_damon_call_fn(void *arg)
 {
 	struct damon_ctx *c = arg;
@@ -276,7 +303,7 @@ static int damon_reclaim_damon_call_fn(void *arg)
 	damon_for_each_scheme(s, c)
 		damon_reclaim_stat = s->stat;
 
-	return damon_reclaim_handle_commit_inputs();
+	return 0;
 }
 
 static struct damon_call_control call_control = {
-- 
2.53.0

Re: (sashiko review) [PATCH] mm/damon: add synchronous commit for commit_inputs

[Liew Rui Yan]

Forwarding Sashiko review for doing discussions via mails.

# review url: https://sashiko.dev/#/patchset/20260328084524.5451-1-aethernet65535@gmail.com
# start of sashiko.dev inline review
commit 36139818bf6b2abe505ab824b951e904796bcd67
Author: Liew Rui Yan <aethernet65535@gmail.com>

mm/damon: add synchronous commit for commit_inputs

This patch modifies the commit_inputs sysfs parameter to synchronously apply
parameters using damon_call(). It adds custom module parameter callbacks to
return validation errors immediately to userspace.

Link: https://lore.kernel.org/20260318153731.97470-1-aethernet65535@gmail.com

> diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
> index 554559d72976..3af6854ba3da 100644
> --- a/mm/damon/lru_sort.c
> +++ b/mm/damon/lru_sort.c

[ ... ]

> @@ -349,18 +348,46 @@ 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,
> +	};
>  
> -	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() if ctx is not initialized to avoid
> +	* NULL pointer dereference.
> +	*/
> +	if (!ctx)
> +		return -EINVAL;

During kernel boot, start_kernel() uses parse_args() to evaluate the kernel
command line. If damon_lru_sort.commit_inputs=Y is passed, parse_args()
invokes this callback before the module init function runs, so ctx is NULL.

Since returning an error from parse_args() during boot causes it to skip
parsing remaining arguments, does this regression break the system boot
process by dropping after_dashes arguments intended for the init process?

> +
> +	err = damon_call(ctx, &control);

Since this function acts as a module parameter .set callback, it is invoked
by param_attr_store() with the global param_lock held.

Could this cause a system-wide deadlock of module operations? If a sysfs
write occurs exactly during kdamond thread termination (when kdamond_fn()
cancels pending calls but before ctx->kdamond is set to NULL under
kdamond_lock), damon_is_running() can falsely return true. This would leave
the sysfs thread waiting forever on wait_for_completion() while holding
param_lock.

Additionally, prior to this patch, user scripts could configure parameters
and write 'Y' to commit_inputs even if the monitor was disabled, and they
would be applied when started.

Because damon_call() immediately returns -EINVAL if the kdamond thread is
not currently running, will this regression break existing userspace
configuration scripts that trigger a commit before enabling the monitor?

> +
> +	return err ? err : control.return_code;
>  }
>  
> +static const struct kernel_param_ops commit_inputs_param_ops = {

The previous module_param(..., bool) macro used param_ops_bool, which
includes the KERNEL_PARAM_OPS_FL_NOARG flag. This allows parameters to be
provided as a standalone flag without an explicit value.

By omitting this flag here, does providing the parameter without an explicit
value now produce a parsing error?

> +	.set = damon_lru_sort_commit_inputs_store,
> +	.get = param_get_bool,
> +};

The commit_inputs_store function parses the input into a local variable
commit_inputs_request but never writes to the global commit_inputs variable.

Because param_get_bool reads the unmodified global commit_inputs variable,
will sysfs reads of commit_inputs unconditionally return 'N'?

[ ... ]

(Note: The same regressions appear to exist in mm/damon/reclaim.c)


# end of sashiko.dev inline review
# review url: https://sashiko.dev/#/patchset/20260328084524.5451-1-aethernet65535@gmail.com
#
# hkml [1] generated a draft of this mail.  It can be regenerated
# using below command:
#
#     hkml patch sashiko_dev --for_forwarding \
#             20260328084524.5451-1-aethernet65535@gmail.com
#
# [1] https://github.com/sjp38/hackermail

Re: (sashiko review) [PATCH] mm/damon: add synchronous commit for commit_inputs

[Liew Rui Yan]

> > +	/*
> > +	* Skip damon_call() if ctx is not initialized to avoid
> > +	* NULL pointer dereference.
> > +	*/
> > +	if (!ctx)
> > +		return -EINVAL;
> 
> During kernel boot, start_kernel() uses parse_args() to evaluate the kernel
> command line. If damon_lru_sort.commit_inputs=Y is passed, parse_args()
> invokes this callback before the module init function runs, so ctx is NULL.
> 
> Since returning an error from parse_args() during boot causes it to skip
> parsing remaining arguments, does this regression break the system boot
> process by dropping after_dashes arguments intended for the init process?

This is a deliberate design choice. The 'commit_inputs' parameter is
intended to function as a synchronous trigger (like a button) while the
kdamond is running.

Regarding the concerns about setting 'commit_inputs=Y' when 'enabled=N'
(or during boot), I believe this is non-issue in practice. I plan to
provide an update to the admin-guide after this patch is merged to
clarify the proper usage sequence for users.

> > +
> > +	err = damon_call(ctx, &control);
> 
> Since this function acts as a module parameter .set callback, it is invoked
> by param_attr_store() with the global param_lock held.
> 
> Could this cause a system-wide deadlock of module operations? If a sysfs
> write occurs exactly during kdamond thread termination (when kdamond_fn()
> cancels pending calls but before ctx->kdamond is set to NULL under
> kdamond_lock), damon_is_running() can falsely return true. This would leave
> the sysfs thread waiting forever on wait_for_completion() while holding
> param_lock.

You are correct regarding the potential deadlock. But this is a known
issue, and SeongJae Park is already working on a fix [1].

> Additionally, prior to this patch, user scripts could configure parameters
> and write 'Y' to commit_inputs even if the monitor was disabled, and they
> would be applied when started.
> 
> Because damon_call() immediately returns -EINVAL if the kdamond thread is
> not currently running, will this regression break existing userspace
> configuration scripts that trigger a commit before enabling the monitor?

I understand your concern about backward compatibility. But providing a
'commit_inputs' request while the kdamond is not running, is technically
redundant. Since parameters are applied automatically at the next
'enabled=Y', the new -EINVAL response serves as clearer feedback for
userspace, ensuring that synchronous commits are only invoked when the
kdamond is actually running.

> > +
> > +	return err ? err : control.return_code;
> >  }
> >  
> > +static const struct kernel_param_ops commit_inputs_param_ops = {
> 
> The previous module_param(..., bool) macro used param_ops_bool, which
> includes the KERNEL_PARAM_OPS_FL_NOARG flag. This allows parameters to be
> provided as a standalone flag without an explicit value.
> 
> By omitting this flag here, does providing the parameter without an explicit
> value now produce a parsing error?

To be honest, I was not aware of the implications of omitting the NOARG
flag. It was not a deliberate choice on my part.

SeongJae, what are your thoughts on this? SHould we stick with the
explicit 'commit_inputs=Y' requirement for the new synchronous behavior,
or should we restore the NOARG compatibility?

> > +	.set = damon_lru_sort_commit_inputs_store,
> > +	.get = param_get_bool,
> > +};
> 
> The commit_inputs_store function parses the input into a local variable
> commit_inputs_request but never writes to the global commit_inputs variable.
> 
> Because param_get_bool reads the unmodified global commit_inputs variable,
> will sysfs reads of commit_inputs unconditionally return 'N'?

This is actually a design choice we discussed [2]. Since 'commit_inputs'
is now a synchronous trigger (or a "button") rather than a persistent
state, it doesn't necessarily need to store the 'Y' value.

> [ ... ]
> 
> (Note: The same regressions appear to exist in mm/damon/reclaim.c)

Thank you for your review. :>

[1] https://lore.kernel.org/20260327233319.3528-1-sj@kernel.org (Lastest
but may not the last fix)
[2] https://lore.kernel.org/20260323150544.81042-1-sj@kernel.org

Best regards,
Rui Yan

Re: (sashiko review) [PATCH] mm/damon: add synchronous commit for commit_inputs

[SeongJae Park]

On Sat, 28 Mar 2026 18:44:50 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
[...]
> > > +static const struct kernel_param_ops commit_inputs_param_ops = {
> > 
> > The previous module_param(..., bool) macro used param_ops_bool, which
> > includes the KERNEL_PARAM_OPS_FL_NOARG flag. This allows parameters to be
> > provided as a standalone flag without an explicit value.
> > 
> > By omitting this flag here, does providing the parameter without an explicit
> > value now produce a parsing error?
> 
> To be honest, I was not aware of the implications of omitting the NOARG
> flag. It was not a deliberate choice on my part.
> 
> SeongJae, what are your thoughts on this? SHould we stick with the
> explicit 'commit_inputs=Y' requirement for the new synchronous behavior,
> or should we restore the NOARG compatibility?

Should be a no big deal.  But it would better to keep the compatibility unless
it makes things too difficult.  And it seems simple to do.  Could you please do
that?

> 
> > > +	.set = damon_lru_sort_commit_inputs_store,
> > > +	.get = param_get_bool,
> > > +};
> > 
> > The commit_inputs_store function parses the input into a local variable
> > commit_inputs_request but never writes to the global commit_inputs variable.
> > 
> > Because param_get_bool reads the unmodified global commit_inputs variable,
> > will sysfs reads of commit_inputs unconditionally return 'N'?
> 
> This is actually a design choice we discussed [2]. Since 'commit_inputs'
> is now a synchronous trigger (or a "button") rather than a persistent
> state, it doesn't necessarily need to store the 'Y' value.

I agree to all Liew's opinions.

> 
> > [ ... ]
> > 
> > (Note: The same regressions appear to exist in mm/damon/reclaim.c)
> 
> Thank you for your review. :>

Thank you for reviewing the review.

> 
> [1] https://lore.kernel.org/20260327233319.3528-1-sj@kernel.org (Lastest
> but may not the last fix)
> [2] https://lore.kernel.org/20260323150544.81042-1-sj@kernel.org


Thanks,
SJ

[...]

Re: (sashiko review) [PATCH] mm/damon: add synchronous commit for commit_inputs

[Liew Rui Yan]

On Sat, 28 Mar 2026 07:09:06 -0700 SeongJae Park <sj@kernel.org> wrote:

> On Sat, 28 Mar 2026 18:44:50 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
> >
> > [...]
> > > The previous module_param(..., bool) macro used param_ops_bool, which
> > > includes the KERNEL_PARAM_OPS_FL_NOARG flag. This allows parameters to be
> > > provided as a standalone flag without an explicit value.
> > > 
> > > By omitting this flag here, does providing the parameter without an explicit
> > > value now produce a parsing error?
> > 
> > To be honest, I was not aware of the implications of omitting the NOARG
> > flag. It was not a deliberate choice on my part.
> > 
> > SeongJae, what are your thoughts on this? SHould we stick with the
> > explicit 'commit_inputs=Y' requirement for the new synchronous behavior,
> > or should we restore the NOARG compatibility?
> 
> Should be a no big deal.  But it would better to keep the compatibility unless
> it makes things too difficult.  And it seems simple to do.  Could you please do
> that?

Sure, I will restore the NOARG compatibility in the next version.

> > 
> > > > +	.set = damon_lru_sort_commit_inputs_store,
> > > > +	.get = param_get_bool,
> > > > +};
> > > 
> > > The commit_inputs_store function parses the input into a local variable
> > > commit_inputs_request but never writes to the global commit_inputs variable.
> > > 
> > > Because param_get_bool reads the unmodified global commit_inputs variable,
> > > will sysfs reads of commit_inputs unconditionally return 'N'?
> > 
> > This is actually a design choice we discussed [2]. Since 'commit_inputs'
> > is now a synchronous trigger (or a "button") rather than a persistent
> > state, it doesn't necessarily need to store the 'Y' value.
> 
> I agree to all Liew's opinions.

Great! Glad we are aligned on the design.

> [...]
> > [1] https://lore.kernel.org/20260327233319.3528-1-sj@kernel.org (Lastest
> > but may not the last fix)
> > [2] https://lore.kernel.org/20260323150544.81042-1-sj@kernel.org
> [...]

Best regards,
Rui Yan