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

No, that's not a problem for me, but I think it might change some user
behavior, although I don't think anyone relies on that behavior. If
you're okay with it, then that's great! :>

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

I also believe that this is true.

> > >
> > > 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 have read the implementation of this damon_call_handle_inactive_ctx().
It does seem to avoid that issue.

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

It makes perfect sense. I believe there shouldn't be any problems, since
both 'enabled' and 'commit_inputs' are protected by param_lock.

Best regards,
Rui Yan