Re: (sashiko review) [PATCH] 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/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