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

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

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.

But, this means that users can no longer write 'commit_inputs=Y' before
'enabled=Y'.

> > +
> > +	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?
>
> 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.

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.

[1] https://sashiko.dev/#/patchset/20260323021648.6590-1-aethernet65535%40gmail.com

Best regards,
Rui Yan