[PATCH V4] mm/damon/reclaim: Fix the timer always stays active

[PATCH V4] mm/damon/reclaim: Fix the timer always stays active

[Hailong Tu]

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.

Signed-off-by: Hailong Tu <tuhailong@gmail.com>
---
 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)");
+
 static int damon_reclaim_after_aggregation(struct damon_ctx *c)
 {
 	struct damos *s;
-- 
2.25.1

Re: [PATCH V4] mm/damon/reclaim: Fix the timer always stays active

[SeongJae Park]

Hi Hailong,


On Thu, 21 Apr 2022 18:05:24 +0800 Hailong Tu <tuhailong@gmail.com> 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@kernel.org/

> 
> Signed-off-by: Hailong Tu <tuhailong@gmail.com>
> ---
>  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
> 

答复: [PATCH V4] mm/damon/reclaim: Fix the timer always stays active

[涂海龙(Hailong)]

Hi, SJ
  OK, I will modify it according to your comments

  But I did checkpatch, there was no obvious style problem.
./scripts/checkpatch.pl --no-tree -f mm/damon/reclaim.c
total: 0 errors, 0 warnings, 425 lines checked
mm/damon/reclaim.c has no obvious style problems and is ready for submission.

Yours sincerely
Hailong

-----邮件原件-----
发件人: SeongJae Park <sj@kernel.org>
发送时间: 2022年4月21日 20:41
收件人: Hailong Tu <tuhailong@gmail.com>
抄送: akpm@linux-foundation.org; sj@kernel.org; torvalds@linux-foundation.org; gregkh@google.com; surenb@google.com; linux-mm@kvack.org; linux-kernel@vger.kernel.org; 涂海龙(Hailong) <tuhailong@oppo.com>; 李春鹏(punk li) <lichunpeng@oppo.com>; 邱长平(Aaron Qiu) <aaron.qiu@oppo.com>; 范国泽(Gordon) <fanguoze@oppo.com>; 王德锁(DeSuo) <wangdesuo@oppo.com>; 李志华(Terry) <lizhihua@oppo.com>
主题: Re: [PATCH V4] mm/damon/reclaim: Fix the timer always stays active

Hi Hailong,


On Thu, 21 Apr 2022 18:05:24 +0800 Hailong Tu <tuhailong@gmail.com> 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@kernel.org/

>
> Signed-off-by: Hailong Tu <tuhailong@gmail.com>
> ---
>  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
>
________________________________
OPPO

本电子邮件及其附件含有OPPO公司的保密信息，仅限于邮件指明的收件人使用（包含个人及群组）。禁止任何人在未经授权的情况下以任何形式使用。如果您错收了本邮件，请立即以电子邮件通知发件人并删除本邮件及其附件。

This e-mail and its attachments contain confidential information from OPPO, which is intended only for the person or entity whose address is listed above. Any use of the information contained herein in any way (including, but not limited to, total or partial disclosure, reproduction, or dissemination) by persons other than the intended recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by phone or email immediately and delete it!