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

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

> > Problem
> > =======
> > Writing invalid parameters to sysfs followed by 'commit_inputs=Y' fails
> > silently (no error returned to shell), because the validation happens
> > asynchronously in the kdamond.
> > 
> > Solution
> > ========
> > To fix this, the commit_inputs_store() callback now uses damon_call() to
> > synchronously commit parameters in the kdamond thread's safe context.
> > This ensures that validation errors are returned immediately to
> > userspace, following the pattern used by DAMON_SYSFS.
> > 
> > Changes
> > =======
> > 1. Added commit_inputs_store() and commit_inputs_fn() to commit
> >    synchronously.
> > 2. Removed handle_commit_inputs().
> > 
> > This change is motivated from another discussion [1].
> > 
> > [1] https://lore.kernel.org/20260318153731.97470-1-aethernet65535@gmail.com
> > 
> > Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com>
> > ---
> > Changes from RFC-v3:
> > - Added checks for 'ctx' and 'damon_is_running()' to prevent NULL
> >   pointer dereference during early boot. (Found by Sashiko.dev)
> 
> I'd prefer archiving sashiko question on the mailing list so that others can
> also read it without have to visit the web site.  Please consider doing so.

Sure, that was indeed my oversight. I didn't consider that others might
not want to open other websites (or that the link might become invalid
someday). Next time I will try to archive shashiko question on the
maling list. Thanks for reminding me.

> FYI, because such sharing is not very comfortable for now, I developed new hkml
> features [1] for helping such sashiko review sharing, and I'm using the
> feature.  Please fee free to use it if you think it can help you, too.
> 
> [1] https://github.com/sjp38/hackermail/blob/master/USAGE.md#sashikodev

I will try this tool.

> > - Removed handle_commit_inputs() and its associated polling logic as
> >   they have become dead code after moving to the synchronous damon_call()
> >   approach.
> > - Ensure the 'commit_inputs' is properly updated.
> > Link to RFC-v3: https://lore.kernel.org/20260322231522.32700-1-aethernet65535@gmail.com
> > 
> > Changes from RFC-v2:
> > - Removed damon_validate_attrs(), now using damon_commit_ctx() for
> >   synchronous validation in the kdamond context.
> > - Following DAMON_SYSFS pattern for synchronous commit via damon_call().
> > - Link to RFC-v2: https://lore.kernel.org/20260321140926.22163-1-aethernet65535@gmail.com
> 
> Thank you for adding the detailed revision changes.
> 
> But, please consider waiting about one day before posting a new version, so
> that we have enough time to discuss on the previous version.  If you find
> something you want to change on the next version, you can comment that to the
> current version of the patch and give time for others to comment about the next
> revision plan if they have any opinion.

Understood. I will post the new version only after careful
consideration. And when I have a new ideas (of next version), I will
clearly comment on them.

> > 
> > Changes from RFC-v1:
> > - Remove question from commit message area.
> > - Added synchronous validation for DAMON_RECLAIM.
> > - Rename damon_valid_attrs() -> damon_validate_attrs().
> > - Exported a new function damon_validate_attrs() and declared it in
> >   damon.h.
> > - Link to RFC-v1: https://lore.kernel.org/20260321002642.22712-1-aethernet65535@gmail.com
> > 
> >  mm/damon/lru_sort.c | 49 +++++++++++++++++++++++++++++++++++++++------
> >  mm/damon/reclaim.c  | 49 +++++++++++++++++++++++++++++++++++++++------
> >  2 files changed, 86 insertions(+), 12 deletions(-)
> > 
> > diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
> > index 554559d72976..37b3c897e822 100644
> > --- a/mm/damon/lru_sort.c
> > +++ b/mm/damon/lru_sort.c
> > @@ -39,7 +39,6 @@ static bool enabled __read_mostly;
> >   * the re-reading, DAMON_LRU_SORT will be disabled.
> >   */
> >  static bool commit_inputs __read_mostly;
> > -module_param(commit_inputs, bool, 0600);
> >  
> >  /*
> >   * Desired active to [in]active memory ratio in bp (1/10,000).
> > @@ -349,18 +348,56 @@ 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 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;
> 
> I was not very sure what 'yes' means.  How about renaming it, say,
> 'commit_inputs_request' ?

Indeed, that makes sense, I've now changed the variable name to
'commit_inputs_request'.

> > +
> > +	if (commit_inputs == yes)
> >  		return 0;
> >  
> > -	err = damon_lru_sort_apply_parameters();
> > +	if (!yes) {
> > +		commit_inputs = false;
> > +		return 0;
> > +	}
> 
> I'd prefer doing !yes check before 'commit_inputs == yes' check.  That
> eliminates false request case earlier, make my brain cleaner.

Since 'commit_inputs' always is N, I will remove 'commit_inputs == yes'
and keep only '!yes'.

> > +
> > +	commit_inputs = yes;
> 
> We will anyway set this 'false' after damon_call().  Before returning this
> function, users cannot read commit_inputs parameter since this callback is
> protected by param_lock.  I think we don't need to change commit_inputs value
> at all?

I think so too!

> > +
> > +	/*
> > +	 * 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;
> 
> damon_call() handles !damon_is_running() case.  So I think you should check
> only !ctx.
> 
> Also, this exposes commit_inputs true.  Next 'Y' write to commit_inputs will
> make no effect?  Again, it seems we shouldn't change commit_inputs at all.

You're absolutely right; I realized this during my code review as well.
Changing the value of 'commit_inputs' is indeed unnecessary.

Best regards,
Rui Yan