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

[SeongJae Park <sj@kernel.org>]

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

[...]