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

[SeongJae Park <sj@kernel.org>]

On Mon, 27 Apr 2026 00:00:11 +0000 sashiko-bot@kernel.org wrote:

> Sashiko AI review found 1 potential issue(s):
> - [High] Writing to the `commit_inputs` sysfs parameter before the DAMON worker thread (`kdamond`) is started causes the writing task to hang indefinitely in an unkillable D-state.

I think this is wrong.

> --
> 
> commit 43a70e5ab295c21c0b94eddd32d886c9d0eba3fa
> Author: Liew Rui Yan <aethernet65535@gmail.com>
> 
> mm/damon: add synchronous commit for commit_inputs
> 
> This commit changes the commit_inputs_store() callback to use damon_call()
> to synchronously commit parameters in the kdamond thread's safe context.
> This ensures validation errors are returned 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..641af42cc2d1 100644
> > --- a/mm/damon/lru_sort.c
> > +++ b/mm/damon/lru_sort.c
> 
> [ ... ]
> 
> > @@ -349,18 +348,51 @@ 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)
> > +	if (!val) {
> > +		commit_inputs_request = true;
> > +	} else {
> > +		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;
> > +
> > +	err = damon_call(ctx, &control);
> 
> If the module has not yet been enabled (e.g., enabled=N) and the kdamond
> thread is not running, does this call to damon_call() block indefinitely?
> 
> Since ctx is allocated during initialization, the ctx pointer is not NULL.
> damon_call() then queues the control request and calls wait_for_completion().
> 
> Without the background thread running to process the queue and signal
> completion, it seems the sysfs writer task is placed into an unkillable
> D-state forever.

No.  ctx is initialized to NULL because it is static.  Once it is set,
kdamond_fn() is started.  If user stops kdamond by writing N to enabled,
ctx->call_controls_obsolete is set.  If user further writes Y to commit_inputs
for invoking damon_call(), it will return -ECANCELED due to
ctx->call_controls_obsolete.

> 
> The damon_call() documentation indicates it should only be called after
> damon_start() has succeeded, otherwise it could fall into an indefinite wait.

Yes, and that's excluded here since the module is not calling damon_call() for
NULL ctx.

> 
> > +
> > +	return err ? err : control.return_code;
> >  }
> 
> [ ... ]
> 
> > diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
> > index 86da14778658..4fc4a54b5e54 100644
> > --- a/mm/damon/reclaim.c
> > +++ b/mm/damon/reclaim.c
> 
> [ ... ]
> 
> > @@ -255,18 +254,51 @@ 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)
> > +	if (!val) {
> > +		commit_inputs_request = true;
> > +	} else {
> > +		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);
> 
> Could this exhibit the same hang as in damon_lru_sort.c? If the worker
> thread has not been started, does writing to commit_inputs block forever?

Same to the above reply, no.

> 
> > +
> > +	return err ? err : control.return_code;
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260426231619.107231-1-sj@kernel.org?part=3

Sent using hkml (https://github.com/sjp38/hackermail)