Re: (sashiko review) [PATCH] mm/damon: add synchronous commit for commit_inputs

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

> > +	/*
> > +	* Skip damon_call() if ctx is not initialized to avoid
> > +	* NULL pointer dereference.
> > +	*/
> > +	if (!ctx)
> > +		return -EINVAL;
> 
> During kernel boot, start_kernel() uses parse_args() to evaluate the kernel
> command line. If damon_lru_sort.commit_inputs=Y is passed, parse_args()
> invokes this callback before the module init function runs, so ctx is NULL.
> 
> Since returning an error from parse_args() during boot causes it to skip
> parsing remaining arguments, does this regression break the system boot
> process by dropping after_dashes arguments intended for the init process?

This is a deliberate design choice. The 'commit_inputs' parameter is
intended to function as a synchronous trigger (like a button) while the
kdamond is running.

Regarding the concerns about setting 'commit_inputs=Y' when 'enabled=N'
(or during boot), I believe this is non-issue in practice. I plan to
provide an update to the admin-guide after this patch is merged to
clarify the proper usage sequence for users.

> > +
> > +	err = damon_call(ctx, &control);
> 
> Since this function acts as a module parameter .set callback, it is invoked
> by param_attr_store() with the global param_lock held.
> 
> Could this cause a system-wide deadlock of module operations? If a sysfs
> write occurs exactly during kdamond thread termination (when kdamond_fn()
> cancels pending calls but before ctx->kdamond is set to NULL under
> kdamond_lock), damon_is_running() can falsely return true. This would leave
> the sysfs thread waiting forever on wait_for_completion() while holding
> param_lock.

You are correct regarding the potential deadlock. But this is a known
issue, and SeongJae Park is already working on a fix [1].

> Additionally, prior to this patch, user scripts could configure parameters
> and write 'Y' to commit_inputs even if the monitor was disabled, and they
> would be applied when started.
> 
> Because damon_call() immediately returns -EINVAL if the kdamond thread is
> not currently running, will this regression break existing userspace
> configuration scripts that trigger a commit before enabling the monitor?

I understand your concern about backward compatibility. But providing a
'commit_inputs' request while the kdamond is not running, is technically
redundant. Since parameters are applied automatically at the next
'enabled=Y', the new -EINVAL response serves as clearer feedback for
userspace, ensuring that synchronous commits are only invoked when the
kdamond is actually running.

> > +
> > +	return err ? err : control.return_code;
> >  }
> >  
> > +static const struct kernel_param_ops commit_inputs_param_ops = {
> 
> The previous module_param(..., bool) macro used param_ops_bool, which
> includes the KERNEL_PARAM_OPS_FL_NOARG flag. This allows parameters to be
> provided as a standalone flag without an explicit value.
> 
> By omitting this flag here, does providing the parameter without an explicit
> value now produce a parsing error?

To be honest, I was not aware of the implications of omitting the NOARG
flag. It was not a deliberate choice on my part.

SeongJae, what are your thoughts on this? SHould we stick with the
explicit 'commit_inputs=Y' requirement for the new synchronous behavior,
or should we restore the NOARG compatibility?

> > +	.set = damon_lru_sort_commit_inputs_store,
> > +	.get = param_get_bool,
> > +};
> 
> The commit_inputs_store function parses the input into a local variable
> commit_inputs_request but never writes to the global commit_inputs variable.
> 
> Because param_get_bool reads the unmodified global commit_inputs variable,
> will sysfs reads of commit_inputs unconditionally return 'N'?

This is actually a design choice we discussed [2]. Since 'commit_inputs'
is now a synchronous trigger (or a "button") rather than a persistent
state, it doesn't necessarily need to store the 'Y' value.

> [ ... ]
> 
> (Note: The same regressions appear to exist in mm/damon/reclaim.c)

Thank you for your review. :>

[1] https://lore.kernel.org/20260327233319.3528-1-sj@kernel.org (Lastest
but may not the last fix)
[2] https://lore.kernel.org/20260323150544.81042-1-sj@kernel.org

Best regards,
Rui Yan