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

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

Hi SeongJae,

> Forwarding Sashiko review for doing discussions via mails.
> 
> # review url: https://sashiko.dev/#/patchset/20260325013939.18167-1-aethernet65535@gmail.com
> # start of sashiko.dev inline review
> commit 60ccea4154b0c58741fae2323454a5a9496b67fa
> Author: Liew Rui Yan <aethernet65535@gmail.com>
> 
> mm/damon: add synchronous commit for commit_inputs
> 
> Writing invalid parameters to sysfs followed by commit_inputs=Y previously
> failed silently. This patch uses damon_call() to synchronously commit
> parameters in the kdamond thread's safe context, returning 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..a2410f648b51 100644
> > --- a/mm/damon/lru_sort.c
> > +++ b/mm/damon/lru_sort.c

Thank you for forwarding the review from Sashiko.dev!

> > +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,
> > +		.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);
> 
> Can this sequence lead to a system-wide deadlock on kernel_param_lock?
> 
> Because damon_lru_sort_commit_inputs_store() is a sysfs .set callback, it
> executes with the global kernel_param_lock held.
> 
> If kdamond_fn terminates (e.g., due to an invalid parameter like addr_unit=3
> causing a non-power-of-2 min_region_sz), it processes its exit sequence by
> calling kdamond_call(ctx, true) to flush pending requests. After flushing, it
> eventually sets ctx->kdamond = NULL at the very end of the thread.
> 
> If a user writes to commit_inputs exactly in the window after
> kdamond_call() finishes but before ctx->kdamond = NULL is set:
> 
> Thread 1 (kdamond_fn):
>     kdamond_call(ctx, true);
> 
> Thread 2 (sysfs write):
>     damon_call(ctx, &control);
>         if (damon_is_running(ctx))
>             wait_for_completion(&control.completion);
> 
> Thread 1 (kdamond_fn):
>     ctx->kdamond = NULL;
> 
> Thread 2 will sleep indefinitely in wait_for_completion(). Because Thread 2
> holds the global kernel_param_lock, this permanently freezes all module
> parameter sysfs interfaces system-wide.

I have verified this behavior with the following test case:

    # cd /sys/module/damon_lru_sort/parameters/
    # echo Y > enabled
    # ps aux | grep "[k]damond"
    root   70  0.0  0.0  0  0 ?  I  12:16  0:00 [kdamond.0]
    # echo 3 > addr_unit
    # echo Y > commit_inputs
    bash: echo: write error: Invalid argument
    # ps aux | grep "[k]damond"

    ... kdamond has exited unexpectedly

I will add a patch in v6 to validate 'addr_unit' in addr_unit_store() to
reject non-power-of-2 inputs immediately.

> > +	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;
> > +
> > +		return err;
> > +	}
> > +
> > +	return control.return_code;
> >  }
> 
> > diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
> > index 86da14778658..4cbd67e08d56 100644
> > --- a/mm/damon/reclaim.c
> > +++ b/mm/damon/reclaim.c
> 
> [ ... ]
> 
> > +	/*
> > +	 * 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);
> 
> Does this code share the same kernel_param_lock deadlock potential as
> damon_lru_sort_commit_inputs_store() described above?

As we discussed in the RFC-v4 thread [1], this is a false positive.
Since 'enabled=N' and 'commit_inputs=Y' are both serialized by the
global 'kernel_param_lock', and kthreads cannot be forcibly terminated
by userspace signals, the completion signal in damon_call() will always
be reached in a controlled manner.

[1] https://lore.kernel.org/20260323021648.6590-1-aethernet65535@gmail.com

Best regards,
Rui Yan