Re: [PATCH 2/3] samples/damon: change enable parameters to enabled

[SeongJae Park <sj@kernel.org>]

Hi Honggyu,

On Sun, 22 Jun 2025 21:09:24 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:

> The damon_{lru_sort,reclaim,stat} kernel modules use "enabled"
> parameter knobs as follows.
> 
>   /sys/module/damon_lru_sort/parameters/enabled
>   /sys/module/damon_reclaim/parameters/enabled
>   /sys/module/damon_stat/parameters/enabled
> 
> However, other sample modules of damon use "enable" parameter knobs so
> it'd be better to rename them from "enable" to "enabled" to keep the
> consistency with other damon modules.

I think this is a good change, thank you!

[...]
> 
> Signed-off-by: Honggyu Kim <honggyu.kim@sk.com>
> Cc: Yunjeong Mun <yunjeong.mun@sk.com>

I have simple comments about variable names below.  If you address it, please
feel free to add below.

Reviewed-by: SeongJae Park <sj@kernel.org>

> ---
>  samples/damon/mtier.c | 16 ++++++++--------
>  samples/damon/prcl.c  | 16 ++++++++--------
>  samples/damon/wsse.c  | 16 ++++++++--------
>  3 files changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/samples/damon/mtier.c b/samples/damon/mtier.c
> index f3220d6e6739..cb273bf4b5c2 100644
> --- a/samples/damon/mtier.c
> +++ b/samples/damon/mtier.c
> @@ -33,14 +33,14 @@ module_param(node0_mem_free_bp, ulong, 0600);
>  static int damon_sample_mtier_enable_store(
>  		const char *val, const struct kernel_param *kp);
>  
> -static const struct kernel_param_ops enable_param_ops = {
> +static const struct kernel_param_ops enabled_param_ops = {
>  	.set = damon_sample_mtier_enable_store,
>  	.get = param_get_bool,
>  };
>  
> -static bool enable __read_mostly;
> -module_param_cb(enable, &enable_param_ops, &enable, 0600);
> -MODULE_PARM_DESC(enable, "Enable of disable DAMON_SAMPLE_MTIER");
> +static bool enabled __read_mostly;
> +module_param_cb(enabled, &enabled_param_ops, &enabled, 0600);
> +MODULE_PARM_DESC(enabled, "Enable or disable DAMON_SAMPLE_MTIER");
>  
>  static struct damon_ctx *ctxs[2];
>  
> @@ -160,17 +160,17 @@ static void damon_sample_mtier_stop(void)
>  static int damon_sample_mtier_enable_store(
>  		const char *val, const struct kernel_param *kp)
>  {
> -	bool enabled = enable;
> +	bool enable = enabled;

The local variable is to cache the old state of the parameter.  Hence I think
'enable' is not a very good name here.  I'd prefer naming the local variable
and similar ones on other sample modules as 'is_enabled' or somewhat else that
better represents its usage.

[...]
> diff --git a/samples/damon/prcl.c b/samples/damon/prcl.c
[...]
> @@ -112,17 +112,17 @@ static void damon_sample_prcl_stop(void)
>  static int damon_sample_prcl_enable_store(
>  		const char *val, const struct kernel_param *kp)
>  {
> -	bool enabled = enable;
> +	bool enable = enabled;

Ditto.

[...]
> diff --git a/samples/damon/wsse.c b/samples/damon/wsse.c
[...]
> @@ -92,17 +92,17 @@ static void damon_sample_wsse_stop(void)
>  static int damon_sample_wsse_enable_store(
>  		const char *val, const struct kernel_param *kp)
>  {
> -	bool enabled = enable;
> +	bool enable = enabled;

Here, too.

[...]


Thanks,
SJ