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

[RFC v4] 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-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 | 49 +++++++++++++++++++++++++++++++++++++++------
 mm/damon/reclaim.c  | 49 +++++++++++++++++++++++++++++++++++++++------
 2 files changed, 86 insertions(+), 12 deletions(-)

diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
index 554559d72976..37b3c897e822 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,56 @@ 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 yes;
 	int err;
+	struct damon_call_control control = {
+		.fn = damon_lru_sort_commit_inputs_fn,
+		.data = ctx,
+		.repeat = false,
+	};
 
-	if (!commit_inputs)
+	err = kstrtobool(val, &yes);
+	if (err)
+		return err;
+
+	if (commit_inputs == yes)
 		return 0;
 
-	err = damon_lru_sort_apply_parameters();
+	if (!yes) {
+		commit_inputs = false;
+		return 0;
+	}
+
+	commit_inputs = yes;
+
+	/*
+	 * Skip damon_call() during early boot or when kdamond is idle
+	 * to avoid NULL pointer dereference or unexpected -EINVAL.
+	 */
+	if (!ctx || !damon_is_running(ctx))
+		return 0;
+
+	err = damon_call(ctx, &control);
 	commit_inputs = false;
-	return err;
+
+	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 +411,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..441d5d9f9eab 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,56 @@ 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 yes;
 	int err;
+	struct damon_call_control control = {
+		.fn = damon_reclaim_commit_inputs_fn,
+		.data = ctx,
+		.repeat = false,
+	};
 
-	if (!commit_inputs)
+	err = kstrtobool(val, &yes);
+	if (err)
+		return err;
+
+	if (commit_inputs == yes)
 		return 0;
 
-	err = damon_reclaim_apply_parameters();
+	if (!yes) {
+		commit_inputs = false;
+		return 0;
+	}
+
+	commit_inputs = yes;
+
+	/*
+	 * Skip damon_call() during early boot or when kdamond is idle
+	 * to avoid NULL pointer dereference or unexpected -EINVAL.
+	 */
+	if (!ctx || !damon_is_running(ctx))
+		return 0;
+
+	err = damon_call(ctx, &control);
 	commit_inputs = false;
-	return err;
+
+	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 +313,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: [RFC v4] mm/damon: add synchronous commit for commit_inputs

[Liew Rui Yan]

Hi SeongJae,

Thank you for reviewing this patch. I'd like to address the review from
Sashiko.dev [1] and ask for your guidance on two concerns.

> > -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 yes;
> >  	int err;
> > +	struct damon_call_control control = {
> > +		.fn = damon_lru_sort_commit_inputs_fn,
> > +		.data = ctx,
> > +		.repeat = false,
> > +	};
> >  
> > -	if (!commit_inputs)
> > +	err = kstrtobool(val, &yes);
> > +	if (err)
> > +		return err;
> > +
> > +	if (commit_inputs == yes)
> >  		return 0;
> >  
> > -	err = damon_lru_sort_apply_parameters();
> > +	if (!yes) {
> > +		commit_inputs = false;
> > +		return 0;
> > +	}
> > +
> > +	commit_inputs = yes;
> > +
> > +	/*
> > +	 * Skip damon_call() during early boot or when kdamond is idle
> > +	 * to avoid NULL pointer dereference or unexpected -EINVAL.
> > +	 */
> > +	if (!ctx || !damon_is_running(ctx))
> > +		return 0;
> If kdamond is not running, this returns 0 but leaves commit_inputs set to
> true. When kdamond later starts, will subsequent writes of 'Y' to
> commit_inputs hit the earlier check "if (commit_inputs == yes) return 0;" and
> silently ignore parameter updates until it is manually reset to 'N'?

My current thought:
When '!ctx || !damon_is_running(ctx)', we should return -EBUSY instead
of 0, and keep 'commit_inputs' unchanged. This way, userspace
immediately knows the operation cannot proceed, and there is no risk of
stale state.

But, this means that users can no longer write 'commit_inputs=Y' before
'enabled=Y'.

> > +
> > +	err = damon_call(ctx, &control);
> The module parameter set operations are protected by the global kernel
> param_lock. If the kdamond thread is currently deactivated by watermarks and
> sleeping for the watermark check interval, could this damon_call() block on
> wait_for_completion() and hold the global param_lock for an unbounded
> duration?
>
> Also, could there be a race condition here if the kdamond thread is stopping?
>
> If kdamond_fn() flushes pending calls and only afterward sets ctx->kdamond to
> NULL, could damon_call() insert its control into the list precisely in this
> window? This might cause it to perceive the thread as active and block on the
> completion forever, deadlocking the param_lock.

I admit I don't have an elegant solution yet. Here are my current
thought:

Add a timeout to wait_for_completion() (e.g.,
wait_for_completion_timeout()), using 'damos_watermarks.interval' as the
upper bound. This prevents indefinite blocking of 'param_lock', though
it still holds the global lock for up to several seconds.

[1] https://sashiko.dev/#/patchset/20260323021648.6590-1-aethernet65535%40gmail.com

Best regards,
Rui Yan

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

[Liew Rui Yan]

Hi SeongJae,

Follow-up on my previous email regarding the locking concerns.

> I admit I don't have an elegant solution yet. Here are my current
> thought:
> 
> Add a timeout to wait_for_completion() (e.g.,
> wait_for_completion_timeout()), using 'damos_watermarks.interval' as the
> upper bound. This prevents indefinite blocking of 'param_lock', though
> it still holds the global lock for up to several seconds.

After further analysis, I realized that since both 'commit_inputs' and
'enabled' module parameters are protected by the global 'param_lock',
stopping kdamond gracefully (via enabled=N) is serialized with
'commit_inputs' writes. Therefore, a classic deadlock scenario should
not occur in normal operation.

However, I'm considering edge cases where kdamond might terminate
unexpectedly. In such cases, commit_inputs_store() could hold
'param_lock' indefinitely, blocking other module parameter updates
system-wide.

To mitigate this risk defensively, would you accept adding a timeout to
wait_for_completion()? This ensures 'param_lock' is eventually released
even if kdamond fails unexpectedly.

Please let me know your preference. :>

Best regards,
Rui Yan

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

[SeongJae Park]

On Mon, 23 Mar 2026 10:16:48 +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-v3:
> - Added checks for 'ctx' and 'damon_is_running()' to prevent NULL
>   pointer dereference during early boot. (Found by Sashiko.dev)

I'd prefer archiving sashiko question on the mailing list so that others can
also read it without have to visit the web site.  Please consider doing so.

FYI, because such sharing is not very comfortable for now, I developed new hkml
features [1] for helping such sashiko review sharing, and I'm using the
feature.  Please fee free to use it if you think it can help you, too.

[1] https://github.com/sjp38/hackermail/blob/master/USAGE.md#sashikodev

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

Thank you for adding the detailed revision changes.

But, please consider waiting about one day before posting a new version, so
that we have enough time to discuss on the previous version.  If you find
something you want to change on the next version, you can comment that to the
current version of the patch and give time for others to comment about the next
revision plan if they have any opinion.

> 
> 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 | 49 +++++++++++++++++++++++++++++++++++++++------
>  mm/damon/reclaim.c  | 49 +++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 86 insertions(+), 12 deletions(-)
> 
> diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
> index 554559d72976..37b3c897e822 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,56 @@ 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 yes;
>  	int err;
> +	struct damon_call_control control = {
> +		.fn = damon_lru_sort_commit_inputs_fn,
> +		.data = ctx,
> +		.repeat = false,
> +	};
>  
> -	if (!commit_inputs)
> +	err = kstrtobool(val, &yes);
> +	if (err)
> +		return err;

I was not very sure what 'yes' means.  How about renaming it, say,
'commit_inputs_request' ?

> +
> +	if (commit_inputs == yes)
>  		return 0;
>  
> -	err = damon_lru_sort_apply_parameters();
> +	if (!yes) {
> +		commit_inputs = false;
> +		return 0;
> +	}

I'd prefer doing !yes check before 'commit_inputs == yes' check.  That
eliminates false request case earlier, make my brain cleaner.

> +
> +	commit_inputs = yes;

We will anyway set this 'false' after damon_call().  Before returning this
function, users cannot read commit_inputs parameter since this callback is
protected by param_lock.  I think we don't need to change commit_inputs value
at all?

> +
> +	/*
> +	 * Skip damon_call() during early boot or when kdamond is idle
> +	 * to avoid NULL pointer dereference or unexpected -EINVAL.
> +	 */
> +	if (!ctx || !damon_is_running(ctx))
> +		return 0;

damon_call() handles !damon_is_running() case.  So I think you should check
only !ctx.

Also, this exposes commit_inputs true.  Next 'Y' write to commit_inputs will
make no effect?  Again, it seems we shouldn't change commit_inputs at all.

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

Again, seems this is not really necessary.

> -	return err;
> +
> +	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 +411,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..441d5d9f9eab 100644
> --- a/mm/damon/reclaim.c
> +++ b/mm/damon/reclaim.c

Similar comments should be applied to DAMON_RECLAIM part changes, too.


Thanks,
SJ

[...]

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

[SeongJae Park]

On Mon, 23 Mar 2026 15:27:24 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> Hi SeongJae,
> 
> Thank you for reviewing this patch. I'd like to address the review from
> Sashiko.dev [1] and ask for your guidance on two concerns.
> 
> > > -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 yes;
> > >  	int err;
> > > +	struct damon_call_control control = {
> > > +		.fn = damon_lru_sort_commit_inputs_fn,
> > > +		.data = ctx,
> > > +		.repeat = false,
> > > +	};
> > >  
> > > -	if (!commit_inputs)
> > > +	err = kstrtobool(val, &yes);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	if (commit_inputs == yes)
> > >  		return 0;
> > >  
> > > -	err = damon_lru_sort_apply_parameters();
> > > +	if (!yes) {
> > > +		commit_inputs = false;
> > > +		return 0;
> > > +	}
> > > +
> > > +	commit_inputs = yes;
> > > +
> > > +	/*
> > > +	 * Skip damon_call() during early boot or when kdamond is idle
> > > +	 * to avoid NULL pointer dereference or unexpected -EINVAL.
> > > +	 */
> > > +	if (!ctx || !damon_is_running(ctx))
> > > +		return 0;
> > If kdamond is not running, this returns 0 but leaves commit_inputs set to
> > true. When kdamond later starts, will subsequent writes of 'Y' to
> > commit_inputs hit the earlier check "if (commit_inputs == yes) return 0;" and
> > silently ignore parameter updates until it is manually reset to 'N'?
> 
> My current thought:
> When '!ctx || !damon_is_running(ctx)', we should return -EBUSY instead
> of 0, and keep 'commit_inputs' unchanged. This way, userspace
> immediately knows the operation cannot proceed, and there is no risk of
> stale state.

Yes, I think we should unset commit_inputs in the case as you and sashiko also
mentioned.  Maybe it is better to just keep its value always 'false', though.
Please read my reply to the patch for more details about why I think so.

> 
> But, this means that users can no longer write 'commit_inputs=Y' before
> 'enabled=Y'.

Is this a problem?

> 
> > > +
> > > +	err = damon_call(ctx, &control);
> > The module parameter set operations are protected by the global kernel
> > param_lock. If the kdamond thread is currently deactivated by watermarks and
> > sleeping for the watermark check interval, could this damon_call() block on
> > wait_for_completion() and hold the global param_lock for an unbounded
> > duration?

It is bounded to user-set 'wmarks_interval'.  So I guess that's not a real
issue?

> >
> > Also, could there be a race condition here if the kdamond thread is stopping?
> >
> > If kdamond_fn() flushes pending calls and only afterward sets ctx->kdamond to
> > NULL, could damon_call() insert its control into the list precisely in this
> > window? This might cause it to perceive the thread as active and block on the
> > completion forever, deadlocking the param_lock.

damon_call() handles such cases using damon_call_handle_inactive_ctx().  I hope
that could work for this, too.  Liew, could you please double check?

> 
> I admit I don't have an elegant solution yet. Here are my current
> thought:
> 
> Add a timeout to wait_for_completion() (e.g.,
> wait_for_completion_timeout()), using 'damos_watermarks.interval' as the
> upper bound. This prevents indefinite blocking of 'param_lock', though
> it still holds the global lock for up to several seconds.

Because it is already kind of timed out for user-set 'wmarks_interval', I don't
think we need an additional timeout.  Please let me know if I'm missing
something.


Thanks,
SJ

[...]

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

[SeongJae Park]

On Mon, 23 Mar 2026 22:19:55 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> Hi SeongJae,
> 
> Follow-up on my previous email regarding the locking concerns.
> 
> > I admit I don't have an elegant solution yet. Here are my current
> > thought:
> > 
> > Add a timeout to wait_for_completion() (e.g.,
> > wait_for_completion_timeout()), using 'damos_watermarks.interval' as the
> > upper bound. This prevents indefinite blocking of 'param_lock', though
> > it still holds the global lock for up to several seconds.
> 
> After further analysis, I realized that since both 'commit_inputs' and
> 'enabled' module parameters are protected by the global 'param_lock',
> stopping kdamond gracefully (via enabled=N) is serialized with
> 'commit_inputs' writes. Therefore, a classic deadlock scenario should
> not occur in normal operation.
> 
> However, I'm considering edge cases where kdamond might terminate
> unexpectedly. In such cases, commit_inputs_store() could hold
> 'param_lock' indefinitely, blocking other module parameter updates
> system-wide.
> 
> To mitigate this risk defensively, would you accept adding a timeout to
> wait_for_completion()? This ensures 'param_lock' is eventually released
> even if kdamond fails unexpectedly.
> 
> Please let me know your preference. :>

As I mentioned on the reply to the original mail of this mail, I think such
infinite wait will not happen because damon_call() is aware of the kdamond
stop, and therefore the timeout is not needed.  Please double check and let me
know if I'm missing something, though, as a reply to my reply.


Thanks,
SJ

[...]

Re: [RFC v4] 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-v3:
> > - Added checks for 'ctx' and 'damon_is_running()' to prevent NULL
> >   pointer dereference during early boot. (Found by Sashiko.dev)
> 
> I'd prefer archiving sashiko question on the mailing list so that others can
> also read it without have to visit the web site.  Please consider doing so.

Sure, that was indeed my oversight. I didn't consider that others might
not want to open other websites (or that the link might become invalid
someday). Next time I will try to archive shashiko question on the
maling list. Thanks for reminding me.

> FYI, because such sharing is not very comfortable for now, I developed new hkml
> features [1] for helping such sashiko review sharing, and I'm using the
> feature.  Please fee free to use it if you think it can help you, too.
> 
> [1] https://github.com/sjp38/hackermail/blob/master/USAGE.md#sashikodev

I will try this tool.

> > - 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
> 
> Thank you for adding the detailed revision changes.
> 
> But, please consider waiting about one day before posting a new version, so
> that we have enough time to discuss on the previous version.  If you find
> something you want to change on the next version, you can comment that to the
> current version of the patch and give time for others to comment about the next
> revision plan if they have any opinion.

Understood. I will post the new version only after careful
consideration. And when I have a new ideas (of next version), I will
clearly comment on them.

> > 
> > 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 | 49 +++++++++++++++++++++++++++++++++++++++------
> >  mm/damon/reclaim.c  | 49 +++++++++++++++++++++++++++++++++++++++------
> >  2 files changed, 86 insertions(+), 12 deletions(-)
> > 
> > diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
> > index 554559d72976..37b3c897e822 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,56 @@ 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 yes;
> >  	int err;
> > +	struct damon_call_control control = {
> > +		.fn = damon_lru_sort_commit_inputs_fn,
> > +		.data = ctx,
> > +		.repeat = false,
> > +	};
> >  
> > -	if (!commit_inputs)
> > +	err = kstrtobool(val, &yes);
> > +	if (err)
> > +		return err;
> 
> I was not very sure what 'yes' means.  How about renaming it, say,
> 'commit_inputs_request' ?

Indeed, that makes sense, I've now changed the variable name to
'commit_inputs_request'.

> > +
> > +	if (commit_inputs == yes)
> >  		return 0;
> >  
> > -	err = damon_lru_sort_apply_parameters();
> > +	if (!yes) {
> > +		commit_inputs = false;
> > +		return 0;
> > +	}
> 
> I'd prefer doing !yes check before 'commit_inputs == yes' check.  That
> eliminates false request case earlier, make my brain cleaner.

Since 'commit_inputs' always is N, I will remove 'commit_inputs == yes'
and keep only '!yes'.

> > +
> > +	commit_inputs = yes;
> 
> We will anyway set this 'false' after damon_call().  Before returning this
> function, users cannot read commit_inputs parameter since this callback is
> protected by param_lock.  I think we don't need to change commit_inputs value
> at all?

I think so too!

> > +
> > +	/*
> > +	 * Skip damon_call() during early boot or when kdamond is idle
> > +	 * to avoid NULL pointer dereference or unexpected -EINVAL.
> > +	 */
> > +	if (!ctx || !damon_is_running(ctx))
> > +		return 0;
> 
> damon_call() handles !damon_is_running() case.  So I think you should check
> only !ctx.
> 
> Also, this exposes commit_inputs true.  Next 'Y' write to commit_inputs will
> make no effect?  Again, it seems we shouldn't change commit_inputs at all.

You're absolutely right; I realized this during my code review as well.
Changing the value of 'commit_inputs' is indeed unnecessary.

Best regards,
Rui Yan

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

[Liew Rui Yan]

> > Hi SeongJae,
> > 
> > Thank you for reviewing this patch. I'd like to address the review from
> > Sashiko.dev [1] and ask for your guidance on two concerns.
> > 
> > > > -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 yes;
> > > >  	int err;
> > > > +	struct damon_call_control control = {
> > > > +		.fn = damon_lru_sort_commit_inputs_fn,
> > > > +		.data = ctx,
> > > > +		.repeat = false,
> > > > +	};
> > > >  
> > > > -	if (!commit_inputs)
> > > > +	err = kstrtobool(val, &yes);
> > > > +	if (err)
> > > > +		return err;
> > > > +
> > > > +	if (commit_inputs == yes)
> > > >  		return 0;
> > > >  
> > > > -	err = damon_lru_sort_apply_parameters();
> > > > +	if (!yes) {
> > > > +		commit_inputs = false;
> > > > +		return 0;
> > > > +	}
> > > > +
> > > > +	commit_inputs = yes;
> > > > +
> > > > +	/*
> > > > +	 * Skip damon_call() during early boot or when kdamond is idle
> > > > +	 * to avoid NULL pointer dereference or unexpected -EINVAL.
> > > > +	 */
> > > > +	if (!ctx || !damon_is_running(ctx))
> > > > +		return 0;
> > > If kdamond is not running, this returns 0 but leaves commit_inputs set to
> > > true. When kdamond later starts, will subsequent writes of 'Y' to
> > > commit_inputs hit the earlier check "if (commit_inputs == yes) return 0;" and
> > > silently ignore parameter updates until it is manually reset to 'N'?
> > 
> > My current thought:
> > When '!ctx || !damon_is_running(ctx)', we should return -EBUSY instead
> > of 0, and keep 'commit_inputs' unchanged. This way, userspace
> > immediately knows the operation cannot proceed, and there is no risk of
> > stale state.
> 
> Yes, I think we should unset commit_inputs in the case as you and sashiko also
> mentioned.  Maybe it is better to just keep its value always 'false', though.
> Please read my reply to the patch for more details about why I think so.
> 
> > 
> > But, this means that users can no longer write 'commit_inputs=Y' before
> > 'enabled=Y'.
> 
> Is this a problem?

No, that's not a problem for me, but I think it might change some user
behavior, although I don't think anyone relies on that behavior. If
you're okay with it, then that's great! :>

> > 
> > > > +
> > > > +	err = damon_call(ctx, &control);
> > > The module parameter set operations are protected by the global kernel
> > > param_lock. If the kdamond thread is currently deactivated by watermarks and
> > > sleeping for the watermark check interval, could this damon_call() block on
> > > wait_for_completion() and hold the global param_lock for an unbounded
> > > duration?
> 
> It is bounded to user-set 'wmarks_interval'.  So I guess that's not a real
> issue?

I also believe that this is true.

> > >
> > > Also, could there be a race condition here if the kdamond thread is stopping?
> > >
> > > If kdamond_fn() flushes pending calls and only afterward sets ctx->kdamond to
> > > NULL, could damon_call() insert its control into the list precisely in this
> > > window? This might cause it to perceive the thread as active and block on the
> > > completion forever, deadlocking the param_lock.
> 
> damon_call() handles such cases using damon_call_handle_inactive_ctx().  I hope
> that could work for this, too.  Liew, could you please double check?

I have read the implementation of this damon_call_handle_inactive_ctx().
It does seem to avoid that issue.

> > I admit I don't have an elegant solution yet. Here are my current
> > thought:
> > 
> > Add a timeout to wait_for_completion() (e.g.,
> > wait_for_completion_timeout()), using 'damos_watermarks.interval' as the
> > upper bound. This prevents indefinite blocking of 'param_lock', though
> > it still holds the global lock for up to several seconds.
> 
> Because it is already kind of timed out for user-set 'wmarks_interval', I don't
> think we need an additional timeout.  Please let me know if I'm missing
> something.

It makes perfect sense. I believe there shouldn't be any problems, since
both 'enabled' and 'commit_inputs' are protected by param_lock.

Best regards,
Rui Yan

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

[Liew Rui Yan]

> > Hi SeongJae,
> > 
> > Follow-up on my previous email regarding the locking concerns.
> > 
> > > I admit I don't have an elegant solution yet. Here are my current
> > > thought:
> > > 
> > > Add a timeout to wait_for_completion() (e.g.,
> > > wait_for_completion_timeout()), using 'damos_watermarks.interval' as the
> > > upper bound. This prevents indefinite blocking of 'param_lock', though
> > > it still holds the global lock for up to several seconds.
> > 
> > After further analysis, I realized that since both 'commit_inputs' and
> > 'enabled' module parameters are protected by the global 'param_lock',
> > stopping kdamond gracefully (via enabled=N) is serialized with
> > 'commit_inputs' writes. Therefore, a classic deadlock scenario should
> > not occur in normal operation.
> > 
> > However, I'm considering edge cases where kdamond might terminate
> > unexpectedly. In such cases, commit_inputs_store() could hold
> > 'param_lock' indefinitely, blocking other module parameter updates
> > system-wide.
> > 
> > To mitigate this risk defensively, would you accept adding a timeout to
> > wait_for_completion()? This ensures 'param_lock' is eventually released
> > even if kdamond fails unexpectedly.
> > 
> > Please let me know your preference. :>
> 
> As I mentioned on the reply to the original mail of this mail, I think such
> infinite wait will not happen because damon_call() is aware of the kdamond
> stop, and therefore the timeout is not needed.  Please double check and let me
> know if I'm missing something, though, as a reply to my reply.

Yes, you are absolutely right. My concern stemmed from a misconception
that kdamond could be forcibly terminated by userspace signals like
'kill -9'.

I just verified this experimentally in virtme-ng, even 'sudo kill -9
<kdamond_pid>' fails to terminate the kdamond thread. This confirms that
kdamond, asa kthread, must exit gracefully via kthread_stop(), ensuring
the completion will always be signaled.

Thank you for guiding me to verify this. I agree that no additional
timeout is needed.

Best regards,
Rui Yan