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

[Liew Rui Yan <aethernet65535@gmail.com>]

Forwarding Sashiko review for doing discussions via mails.

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

mm/damon: add synchronous commit for commit_inputs

This commit modifies the commit_inputs module parameter callback to
synchronously apply parameters using damon_call() instead of relying on
asynchronous validation in the kdamond thread.

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
--- a/mm/damon/lru_sort.c
+++ b/mm/damon/lru_sort.c
[ ... ]
> @@ -349,18 +348,51 @@ static int damon_lru_sort_apply_parameters(void)
[ ... ]
> +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)
> +	if (!val) {
> +		commit_inputs_request = true;
> +	} else {
> +		err = kstrtobool(val, &commit_inputs_request);
> +		if (err)
> +			return err;
> +	}
>  
> -	err = damon_lru_sort_apply_parameters();
> -	commit_inputs = false;
> -	return err;
> +	if (!commit_inputs_request)
>  		return 0;
>  
> +	/*
> +	* Skip damon_call() if ctx is not initialized to avoid
> +	* NULL pointer dereference.
> +	*/
> +	if (!ctx)
> +		return -EINVAL;

Does this break module loading if commit_inputs=Y is provided as a boot
parameter or via a modprobe configuration?

During module initialization, parse_args() executes module parameter
callbacks before the module's init function runs.

Since ctx is initialized in the module's init function, won't ctx be NULL
when this callback fires, causing an -EINVAL return that aborts the
entire module load?

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

Could this synchronous call break backward compatibility for userspace
scripts?

Previously, writing Y to commit_inputs while the monitor was disabled
(enabled=N) would silently succeed and apply the parameters on the next
start.

If the kdamond thread is not running, won't damon_call() detect
!damon_is_running(ctx) and return -EINVAL, causing writes to fail when
the monitor is off?

Since this is a module parameter callback, the global param_lock mutex is
held during its execution. Does calling damon_call() here cause system-wide
latency spikes?

damon_call() blocks on wait_for_completion() waiting for the kdamond thread,
which only checks its call list once per sample interval.

Will this stall all other tasks attempting to read or write built-in module
parameters for up to the duration of the DAMON sampling interval?

Is there a race condition here with the kdamond thread termination that
could result in a permanent deadlock?

When kdamond_fn() exits, it flushes pending controls and then sets
ctx->kdamond to NULL.

If a concurrent sysfs write triggers damon_call() after the controls are
flushed but before ctx->kdamond is cleared, damon_is_running(ctx) will
return true.

Will damon_call() block indefinitely on wait_for_completion() because the
thread is exiting and will never signal it, leaving the sysfs writer
permanently stuck in D state while holding the system-wide param_lock?

> +
> +	return err ? err : control.return_code;
>  }

[ ... ]
diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
--- a/mm/damon/reclaim.c
+++ b/mm/damon/reclaim.c
[ ... ]
> +static int damon_reclaim_commit_inputs_store(const char *val,
> +					    const struct kernel_param *kp)
> +{
[ ... ]
> +	/*
> +	* 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;
>  }

Are these same regressions present here in damon_reclaim_commit_inputs_store()
as well?


# end of sashiko.dev inline review
# review url: https://sashiko.dev/#/patchset/20260329075415.36775-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 \
#             20260329075415.36775-1-aethernet65535@gmail.com
#
# [1] https://github.com/sjp38/hackermail