Re: [RFC PATCH v2 1/3] mm/damon/reclaim: detect and use fresh enabled and kdamond_pid values

[SeongJae Park <sj@kernel.org>]

On Sat, 18 Apr 2026 04:57:15 +0000 sashiko-bot@kernel.org wrote:

> Sashiko AI review found 1 potential issue(s):
> - [Medium] Command-line override for the 'enabled' parameter is broken: passing 'enabled=N' after 'enabled=Y' fails to disable the module.
> --
> 
> commit 6f46639cc34c6ddcbecc0aba915988dff8f651de
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/reclaim: detect and use fresh enabled and kdamond_pid values
> 
> This commit updates DAMON_RECLAIM to dynamically query the running status
> of kdamond instead of relying on cached parameter values. This ensures
> that when kdamond stops unexpectedly, the enabled and kdamond_pid parameters
> reflect the correct state.
> 
> > diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
> > index 86da147786583..46660c8d4d422 100644
> > --- a/mm/damon/reclaim.c
> > +++ b/mm/damon/reclaim.c
> 
> [ ... ]
> 
> > @@ -332,10 +316,16 @@ module_param_cb(addr_unit, &addr_unit_param_ops, &addr_unit, 0600);
> >  MODULE_PARM_DESC(addr_unit,
> >  	"Scale factor for DAMON_RECLAIM to ops address conversion (default: 1)");
> >  
> > +static bool damon_reclaim_enabled(void)
> > +{
> > +	if (!ctx)
> > +		return false;
> > +	return damon_is_running(ctx);
> > +}
> > +
> >  static int damon_reclaim_enabled_store(const char *val,
> >  		const struct kernel_param *kp)
> >  {
> > -	bool is_enabled = enabled;
> >  	bool enable;
> >  	int err;
> >  
> > @@ -343,7 +333,7 @@ static int damon_reclaim_enabled_store(const char *val,
> >  	if (err)
> >  		return err;
> >  
> > -	if (is_enabled == enable)
> > +	if (damon_reclaim_enabled() == enable)
> >  		return 0;
> 
> Does changing this check break kernel command-line overrides during boot?
> 
> During early boot when command-line parameters are parsed, ctx is not yet
> allocated, so damon_reclaim_enabled() unconditionally returns false.
> 
> If a system has damon_reclaim.enabled=Y in the boot parameters and an
> administrator appends damon_reclaim.enabled=N to override it, the first
> evaluation correctly sets the internal enabled flag to true.
> 
> However, when the second parameter (N) is processed, enable is false.
> Since damon_reclaim_enabled() also returns false, the condition matches
> and the function returns 0 immediately. The internal enabled variable is
> never updated to false, causing the module to start unexpectedly when
> damon_reclaim_init() runs.
> 
> Should this keep checking the internal enabled variable during early boot?

I don't think such override is common, but I also think there is no harm at
making the old behavior be kept.  I will address this like below, in the next
revision.

'''
--- a/mm/damon/reclaim.c
+++ b/mm/damon/reclaim.c
@@ -381,27 +381,20 @@ static bool damon_reclaim_enabled(void)
 static int damon_reclaim_enabled_store(const char *val,
                const struct kernel_param *kp)
 {
-       bool enable;
        int err;

-       err = kstrtobool(val, &enable);
+       err = kstrtobool(val, &enabled);
        if (err)
                return err;

-       if (damon_reclaim_enabled() == enable)
+       if (damon_reclaim_enabled() == enabled)
                return 0;

        /* Called before init function.  The function will handle this. */
        if (!damon_initialized())
-               goto set_param_out;
-
-       err = damon_reclaim_turn(enable);
-       if (err)
-               return err;
+               return 0;

-set_param_out:
-       enabled = enable;
-       return err;
+       return damon_reclaim_turn(enabled);
 }

 static int damon_reclaim_enabled_load(char *buffer,
'''


Thanks,
SJ

[...]