Hi Hailong, On Thu, 21 Apr 2022 18:05:24 +0800 Hailong Tu <tuhailong@xxxxxxxxx> wrote: > The timer stays active even if the reclaim mechanism is never enabled. > It is unnecessary overhead can be completely avoided by using module_param_cb() for enabled flag. As commented to the previous version[1], let's wrap the line at 75 columns. checkpatch.pl is also complaining. [1] https://lore.kernel.org/linux-mm/20220421084806.72553-1-sj@xxxxxxxxxx/ > > Signed-off-by: Hailong Tu <tuhailong@xxxxxxxxx> > --- > mm/damon/reclaim.c | 26 ++++++++++++++++++++++++-- > 1 file changed, 24 insertions(+), 2 deletions(-) > > diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c > index e34c4d0c4d93..e573e3f50064 100644 > --- a/mm/damon/reclaim.c > +++ b/mm/damon/reclaim.c > @@ -28,7 +28,6 @@ > * this. > */ > static bool enabled __read_mostly; > -module_param(enabled, bool, 0600); > > /* > * Time threshold for cold memory regions identification in microseconds. > @@ -358,11 +357,34 @@ static void damon_reclaim_timer_fn(struct work_struct *work) > enabled = last_enabled; > } > > - schedule_delayed_work(&damon_reclaim_timer, > + if (enabled) > + schedule_delayed_work(&damon_reclaim_timer, > msecs_to_jiffies(ENABLE_CHECK_INTERVAL_MS)); > } > static DECLARE_DELAYED_WORK(damon_reclaim_timer, damon_reclaim_timer_fn); > > +static int enabled_store(const char *val, > + const struct kernel_param *kp) > +{ > + int rc = param_set_bool(val, kp); > + > + if (rc < 0) > + return rc; > + > + if (enabled) > + schedule_delayed_work(&damon_reclaim_timer, 0); > + > + return 0; > +} > + > +static const struct kernel_param_ops enabled_param_ops = { > + .set = enabled_store, > + .get = param_get_bool, > +}; > + > +module_param_cb(enabled, &enabled_param_ops, &enabled, 0600); > +MODULE_PARM_DESC(enabled, "Enable or disable DAMON_RECLAIM (default: disabled)"); I'd prefer 80 columns limit. Could you please put the string in the next line, like below? MODULE_PARM_DESC(enabled, "Enable or disable DAMON_RECLAIM (default: disabled)"); Thanks, SJ > + > static int damon_reclaim_after_aggregation(struct damon_ctx *c) > { > struct damos *s; > -- > 2.25.1 >
