[PATCH 0/3] mm/damon: Enhance damon and its samples

[PATCH 0/3] mm/damon: Enhance damon and its samples

[Honggyu Kim]

This includes random fixes of damon and its samples to make it safer and
consistent with other damon modules.

It includes the following changes.
- Fix unexpected divide by zero crash not allowing zero size region
- Keep "enabled" parameter in damon samples consistent with others
- Fix bugs for damon samples in case of start failures

Honggyu Kim (3):
  mm/damon: do not allow creating zero size region
  samples/damon: change enable parameters to enabled
  samples/damon: fix bugs for damon sample for start failures

 mm/damon/core.c       |  3 +++
 samples/damon/mtier.c | 22 +++++++++++++---------
 samples/damon/prcl.c  | 22 +++++++++++++---------
 samples/damon/wsse.c  | 22 +++++++++++++---------
 4 files changed, 42 insertions(+), 27 deletions(-)


base-commit: e04c78d86a9699d136910cfc0bdcf01087e3267e
-- 
2.34.1

[PATCH 1/3] mm/damon: do not allow creating zero size region

[Honggyu Kim]

Creating zero size region leads a divide by zero error inside
damon_get_intervals_score() as follows.

  static unsigned long damon_get_intervals_score(struct damon_ctx *c)
  {
      struct damon_target *t;
      struct damon_region *r;
      unsigned long sz_region, max_access_events = 0, access_events = 0;
      unsigned long target_access_events;
      unsigned long goal_bp = c->attrs.intervals_goal.access_bp;

      damon_for_each_target(t, c) {
          damon_for_each_region(r, t) {
              sz_region = damon_sz_region(r);
              max_access_events += sz_region * c->attrs.aggr_samples;
              access_events += sz_region * r->nr_accesses;
          }
      }
      target_access_events = max_access_events * goal_bp / 10000;
      return access_events * 10000 / target_access_events;  /* divide by zero! */
  }

This patch makes a NULL return for such cases when creating a region
inside damon_new_region().

Signed-off-by: Honggyu Kim <honggyu.kim@sk.com>
Cc: Yunjeong Mun <yunjeong.mun@sk.com>
---
 mm/damon/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index b217e0120e09..44740da337fd 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -126,8 +126,11 @@ struct damon_region *damon_new_region(unsigned long start, unsigned long end)
 	if (!region)
 		return NULL;
 
+	if (start == end)
+		return NULL;
 	region->ar.start = start;
 	region->ar.end = end;
+
 	region->nr_accesses = 0;
 	region->nr_accesses_bp = 0;
 	INIT_LIST_HEAD(&region->list);
-- 
2.34.1

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

[Honggyu Kim]

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.

Before:
  /sys/module/wsse/parameters/enable
  /sys/module/prcl/parameters/enable
  /sys/module/mtier/parameters/enable

After:
  /sys/module/wsse/parameters/enabled
  /sys/module/prcl/parameters/enabled
  /sys/module/mtier/parameters/enabled

There is no functional changes in this patch.

Signed-off-by: Honggyu Kim <honggyu.kim@sk.com>
Cc: Yunjeong Mun <yunjeong.mun@sk.com>
---
 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;
 	int err;
 
-	err = kstrtobool(val, &enable);
+	err = kstrtobool(val, &enabled);
 	if (err)
 		return err;
 
-	if (enable == enabled)
+	if (enabled == enable)
 		return 0;
 
-	if (enable)
+	if (enabled)
 		return damon_sample_mtier_start();
 	damon_sample_mtier_stop();
 	return 0;
diff --git a/samples/damon/prcl.c b/samples/damon/prcl.c
index 056b1b21a0fe..e8fe596704cc 100644
--- a/samples/damon/prcl.c
+++ b/samples/damon/prcl.c
@@ -17,14 +17,14 @@ module_param(target_pid, int, 0600);
 static int damon_sample_prcl_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_prcl_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_WSSE");
+static bool enabled __read_mostly;
+module_param_cb(enabled, &enabled_param_ops, &enabled, 0600);
+MODULE_PARM_DESC(enabled, "Enable or disable DAMON_SAMPLE_PRCL");
 
 static struct damon_ctx *ctx;
 static struct pid *target_pidp;
@@ -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;
 	int err;
 
-	err = kstrtobool(val, &enable);
+	err = kstrtobool(val, &enabled);
 	if (err)
 		return err;
 
-	if (enable == enabled)
+	if (enabled == enable)
 		return 0;
 
-	if (enable)
+	if (enabled)
 		return damon_sample_prcl_start();
 	damon_sample_prcl_stop();
 	return 0;
diff --git a/samples/damon/wsse.c b/samples/damon/wsse.c
index 11be25803274..5039d534fdb6 100644
--- a/samples/damon/wsse.c
+++ b/samples/damon/wsse.c
@@ -18,14 +18,14 @@ module_param(target_pid, int, 0600);
 static int damon_sample_wsse_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_wsse_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 or disable DAMON_SAMPLE_WSSE");
+static bool enabled __read_mostly;
+module_param_cb(enabled, &enabled_param_ops, &enabled, 0600);
+MODULE_PARM_DESC(enabled, "Enable or disable DAMON_SAMPLE_WSSE");
 
 static struct damon_ctx *ctx;
 static struct pid *target_pidp;
@@ -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;
 	int err;
 
-	err = kstrtobool(val, &enable);
+	err = kstrtobool(val, &enabled);
 	if (err)
 		return err;
 
-	if (enable == enabled)
+	if (enabled == enable)
 		return 0;
 
-	if (enable)
+	if (enabled)
 		return damon_sample_wsse_start();
 	damon_sample_wsse_stop();
 	return 0;
-- 
2.34.1

[PATCH 3/3] samples/damon: fix bugs for damon sample for start failures

[Honggyu Kim]

The damon_sample_{wsse,prcl,mtier}_start() can fail so we must reset the
"enabled" parameter to "false" again for proper rollback.

In such cases, setting Y to "enabled" then N triggers the following
crash because damon sample start failed but the "enabled" stays as Y.

  [ 2441.419649] damon_sample_prcl: start
  [ 2454.146817] damon_sample_prcl: stop
  [ 2454.146862] ------------[ cut here ]------------
  [ 2454.146865] kernel BUG at mm/slub.c:546!
  [ 2454.148183] Oops: invalid opcode: 0000 [#1] SMP NOPTI
  	...
  [ 2454.167555] Call Trace:
  [ 2454.167822]  <TASK>
  [ 2454.168061]  damon_destroy_ctx+0x78/0x140
  [ 2454.168454]  damon_sample_prcl_enable_store+0x8d/0xd0
  [ 2454.168932]  param_attr_store+0xa1/0x120
  [ 2454.169315]  module_attr_store+0x20/0x50
  [ 2454.169695]  sysfs_kf_write+0x72/0x90
  [ 2454.170065]  kernfs_fop_write_iter+0x150/0x1e0
  [ 2454.170491]  vfs_write+0x315/0x440
  [ 2454.170833]  ksys_write+0x69/0xf0
  [ 2454.171162]  __x64_sys_write+0x19/0x30
  [ 2454.171525]  x64_sys_call+0x18b2/0x2700
  [ 2454.171900]  do_syscall_64+0x7f/0x680
  [ 2454.172258]  ? exit_to_user_mode_loop+0xf6/0x180
  [ 2454.172694]  ? clear_bhb_loop+0x30/0x80
  [ 2454.173067]  ? clear_bhb_loop+0x30/0x80
  [ 2454.173439]  entry_SYSCALL_64_after_hwframe+0x76/0x7e

Since it's just a sample module, no need to add it to stable tree.

Signed-off-by: Honggyu Kim <honggyu.kim@sk.com>
Cc: Yunjeong Mun <yunjeong.mun@sk.com>
---
 samples/damon/mtier.c | 8 ++++++--
 samples/damon/prcl.c  | 8 ++++++--
 samples/damon/wsse.c  | 8 ++++++--
 3 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/samples/damon/mtier.c b/samples/damon/mtier.c
index cb273bf4b5c2..29ee5bf60e74 100644
--- a/samples/damon/mtier.c
+++ b/samples/damon/mtier.c
@@ -170,8 +170,12 @@ static int damon_sample_mtier_enable_store(
 	if (enabled == enable)
 		return 0;
 
-	if (enabled)
-		return damon_sample_mtier_start();
+	if (enabled) {
+		err = damon_sample_mtier_start();
+		if (err)
+			enabled = false;
+		return err;
+	}
 	damon_sample_mtier_stop();
 	return 0;
 }
diff --git a/samples/damon/prcl.c b/samples/damon/prcl.c
index e8fe596704cc..188b292a91c8 100644
--- a/samples/damon/prcl.c
+++ b/samples/damon/prcl.c
@@ -122,8 +122,12 @@ static int damon_sample_prcl_enable_store(
 	if (enabled == enable)
 		return 0;
 
-	if (enabled)
-		return damon_sample_prcl_start();
+	if (enabled) {
+		err = damon_sample_prcl_start();
+		if (err)
+			enabled = false;
+		return err;
+	}
 	damon_sample_prcl_stop();
 	return 0;
 }
diff --git a/samples/damon/wsse.c b/samples/damon/wsse.c
index 5039d534fdb6..53e847546d92 100644
--- a/samples/damon/wsse.c
+++ b/samples/damon/wsse.c
@@ -102,8 +102,12 @@ static int damon_sample_wsse_enable_store(
 	if (enabled == enable)
 		return 0;
 
-	if (enabled)
-		return damon_sample_wsse_start();
+	if (enabled) {
+		err = damon_sample_wsse_start();
+		if (err)
+			enabled = false;
+		return err;
+	}
 	damon_sample_wsse_stop();
 	return 0;
 }
-- 
2.34.1

Re: [PATCH 1/3] mm/damon: do not allow creating zero size region

[SeongJae Park]

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

> Creating zero size region leads a divide by zero error inside
> damon_get_intervals_score() as follows.
> 
>   static unsigned long damon_get_intervals_score(struct damon_ctx *c)
>   {
>       struct damon_target *t;
>       struct damon_region *r;
>       unsigned long sz_region, max_access_events = 0, access_events = 0;
>       unsigned long target_access_events;
>       unsigned long goal_bp = c->attrs.intervals_goal.access_bp;
> 
>       damon_for_each_target(t, c) {
>           damon_for_each_region(r, t) {
>               sz_region = damon_sz_region(r);
>               max_access_events += sz_region * c->attrs.aggr_samples;
>               access_events += sz_region * r->nr_accesses;
>           }
>       }
>       target_access_events = max_access_events * goal_bp / 10000;
>       return access_events * 10000 / target_access_events;  /* divide by zero! */
>   }

Thank you for finding this issue!  Coudl you please further share how zero size
region can be made, and if user-space can make the situation?

> 
> This patch makes a NULL return for such cases when creating a region
> inside damon_new_region().

I agree zero size region could look silly.  But I don't really think it should
be prohibited.  What about modifying damon_get_intervals_score() instead?
Maybe we can set target_access_events as 1 in this case.

> 
> Signed-off-by: Honggyu Kim <honggyu.kim@sk.com>
> Cc: Yunjeong Mun <yunjeong.mun@sk.com>
> ---
>  mm/damon/core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index b217e0120e09..44740da337fd 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -126,8 +126,11 @@ struct damon_region *damon_new_region(unsigned long start, unsigned long end)
>  	if (!region)
>  		return NULL;
>  
> +	if (start == end)
> +		return NULL;
>  	region->ar.start = start;
>  	region->ar.end = end;
> +

Above new line is unnecessary in my opinion.

>  	region->nr_accesses = 0;
>  	region->nr_accesses_bp = 0;
>  	INIT_LIST_HEAD(&region->list);
> -- 
> 2.34.1


Thanks,
SJ

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

[SeongJae Park]

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

Re: [PATCH 3/3] samples/damon: fix bugs for damon sample for start failures

[SeongJae Park]

Hi Honggyu,

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

> The damon_sample_{wsse,prcl,mtier}_start() can fail so we must reset the
> "enabled" parameter to "false" again for proper rollback.
> 
> In such cases, setting Y to "enabled" then N triggers the following
> crash because damon sample start failed but the "enabled" stays as Y.
> 
>   [ 2441.419649] damon_sample_prcl: start
>   [ 2454.146817] damon_sample_prcl: stop
>   [ 2454.146862] ------------[ cut here ]------------
>   [ 2454.146865] kernel BUG at mm/slub.c:546!
>   [ 2454.148183] Oops: invalid opcode: 0000 [#1] SMP NOPTI
>   	...
>   [ 2454.167555] Call Trace:
>   [ 2454.167822]  <TASK>
>   [ 2454.168061]  damon_destroy_ctx+0x78/0x140
>   [ 2454.168454]  damon_sample_prcl_enable_store+0x8d/0xd0
>   [ 2454.168932]  param_attr_store+0xa1/0x120
>   [ 2454.169315]  module_attr_store+0x20/0x50
>   [ 2454.169695]  sysfs_kf_write+0x72/0x90
>   [ 2454.170065]  kernfs_fop_write_iter+0x150/0x1e0
>   [ 2454.170491]  vfs_write+0x315/0x440
>   [ 2454.170833]  ksys_write+0x69/0xf0
>   [ 2454.171162]  __x64_sys_write+0x19/0x30
>   [ 2454.171525]  x64_sys_call+0x18b2/0x2700
>   [ 2454.171900]  do_syscall_64+0x7f/0x680
>   [ 2454.172258]  ? exit_to_user_mode_loop+0xf6/0x180
>   [ 2454.172694]  ? clear_bhb_loop+0x30/0x80
>   [ 2454.173067]  ? clear_bhb_loop+0x30/0x80
>   [ 2454.173439]  entry_SYSCALL_64_after_hwframe+0x76/0x7e

Thank you for finding and fixing this!

> 
> Since it's just a sample module, no need to add it to stable tree.

I agree this is not a real bug.  But, even a bug in DAMON unit test was
deserved to get a CVE id.  Hence I'd suggest adding at least Fixes: tag here,
and let stable kernels maintainers to decide whether to pick this up to stable
kernels or not.  I can help backporting if needed.

If you agree, I think we could add

Fixes: b757c6cfc696d ("samples/damon/wsse: start and stop DAMON as the user requests")

If we want to make stable kernel maintainers' life easier, we would better to
split this into three patches, for each module, and add appropriate Fixes, as
below.

For wsse,
Fixes: b757c6cfc696d ("samples/damon/wsse: start and stop DAMON as the user requests")

For prcl,

Fixes: 2aca254620a8 ("samples/damon: introduce a skeleton of a smaple DAMON module for proactive reclamation")

For mtier,

Fixes: 82a08bde3cf7 ("samples/damon: implement a DAMON module for memory tiering")

Could you please do so?

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

Regardless of your agreement on adding Fixes:

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


Thanks,
SJ

[...]

Re: [PATCH 1/3] mm/damon: do not allow creating zero size region

[Honggyu Kim]

Hi SeongJae,

Thanks for the kind review as always!

On 6/23/2025 1:04 AM, SeongJae Park wrote:
> On Sun, 22 Jun 2025 21:09:23 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:
> 
>> Creating zero size region leads a divide by zero error inside
>> damon_get_intervals_score() as follows.
>>
>>    static unsigned long damon_get_intervals_score(struct damon_ctx *c)
>>    {
>>        struct damon_target *t;
>>        struct damon_region *r;
>>        unsigned long sz_region, max_access_events = 0, access_events = 0;
>>        unsigned long target_access_events;
>>        unsigned long goal_bp = c->attrs.intervals_goal.access_bp;
>>
>>        damon_for_each_target(t, c) {
>>            damon_for_each_region(r, t) {
>>                sz_region = damon_sz_region(r);
>>                max_access_events += sz_region * c->attrs.aggr_samples;
>>                access_events += sz_region * r->nr_accesses;
>>            }
>>        }
>>        target_access_events = max_access_events * goal_bp / 10000;
>>        return access_events * 10000 / target_access_events;  /* divide by zero! */
>>    }
> 
> Thank you for finding this issue!  Coudl you please further share how zero size
> region can be made, and if user-space can make the situation?

The initial values of node*_start_addr and node*_end_addr inside
/sys/module/mtier/parameters/ are all zeros so I saw this problem easily by
setting Y to "enabled".

>>
>> This patch makes a NULL return for such cases when creating a region
>> inside damon_new_region().
> 
> I agree zero size region could look silly.  But I don't really think it should
> be prohibited.  What about modifying damon_get_intervals_score() instead?
> Maybe we can set target_access_events as 1 in this case.

Hmm... I don't get what you mean by setting "target_access_events" to 1 for such
cases.  Could you please explain more?

> 
>>
>> Signed-off-by: Honggyu Kim <honggyu.kim@sk.com>
>> Cc: Yunjeong Mun <yunjeong.mun@sk.com>
>> ---
>>   mm/damon/core.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/mm/damon/core.c b/mm/damon/core.c
>> index b217e0120e09..44740da337fd 100644
>> --- a/mm/damon/core.c
>> +++ b/mm/damon/core.c
>> @@ -126,8 +126,11 @@ struct damon_region *damon_new_region(unsigned long start, unsigned long end)
>>        if (!region)
>>                return NULL;
>>
>> +     if (start == end)
>> +             return NULL;
If you're okay, then I'd like to modify this as follows again.

+     if (start >= end)
+             return NULL;

What do you think about this?

>>        region->ar.start = start;
>>        region->ar.end = end;
>> +
> 
> Above new line is unnecessary in my opinion.

Sure. will remove it in the next version.

Thanks,
Honggyu

> 
>>        region->nr_accesses = 0;
>>        region->nr_accesses_bp = 0;
>>        INIT_LIST_HEAD(&region->list);
>> --
>> 2.34.1
> 
> 
> Thanks,
> SJ

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

[Honggyu Kim]

On 6/23/2025 1:14 AM, SeongJae Park wrote:
> 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>

Thanks for the review!

> 
>> ---
>>   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

[... snip ...]

>> @@ -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.

I also thought about comparing "enabled" and "enable" isn't good, but just
didn't fix it to reduce changes.  As you're pointing that issue as well, I will
rename it in the next version.

Thanks,
Honggyu

> 
> [...]
>> 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
> 

Re: [PATCH 3/3] samples/damon: fix bugs for damon sample for start failures

[Honggyu Kim]

Hi SeongJae,

On 6/23/2025 1:29 AM, SeongJae Park wrote:
> Hi Honggyu,
> 
> On Sun, 22 Jun 2025 21:09:25 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:
> 
>> The damon_sample_{wsse,prcl,mtier}_start() can fail so we must reset the
>> "enabled" parameter to "false" again for proper rollback.
>>
>> In such cases, setting Y to "enabled" then N triggers the following
>> crash because damon sample start failed but the "enabled" stays as Y.
>>
>>    [ 2441.419649] damon_sample_prcl: start
>>    [ 2454.146817] damon_sample_prcl: stop
>>    [ 2454.146862] ------------[ cut here ]------------
>>    [ 2454.146865] kernel BUG at mm/slub.c:546!
>>    [ 2454.148183] Oops: invalid opcode: 0000 [#1] SMP NOPTI
>>        ...
>>    [ 2454.167555] Call Trace:
>>    [ 2454.167822]  <TASK>
>>    [ 2454.168061]  damon_destroy_ctx+0x78/0x140
>>    [ 2454.168454]  damon_sample_prcl_enable_store+0x8d/0xd0
>>    [ 2454.168932]  param_attr_store+0xa1/0x120
>>    [ 2454.169315]  module_attr_store+0x20/0x50
>>    [ 2454.169695]  sysfs_kf_write+0x72/0x90
>>    [ 2454.170065]  kernfs_fop_write_iter+0x150/0x1e0
>>    [ 2454.170491]  vfs_write+0x315/0x440
>>    [ 2454.170833]  ksys_write+0x69/0xf0
>>    [ 2454.171162]  __x64_sys_write+0x19/0x30
>>    [ 2454.171525]  x64_sys_call+0x18b2/0x2700
>>    [ 2454.171900]  do_syscall_64+0x7f/0x680
>>    [ 2454.172258]  ? exit_to_user_mode_loop+0xf6/0x180
>>    [ 2454.172694]  ? clear_bhb_loop+0x30/0x80
>>    [ 2454.173067]  ? clear_bhb_loop+0x30/0x80
>>    [ 2454.173439]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> 
> Thank you for finding and fixing this!
> 
>>
>> Since it's just a sample module, no need to add it to stable tree.
> 
> I agree this is not a real bug.  But, even a bug in DAMON unit test was
> deserved to get a CVE id.  Hence I'd suggest adding at least Fixes: tag here,
> and let stable kernels maintainers to decide whether to pick this up to stable
> kernels or not.  I can help backporting if needed.
> 
> If you agree, I think we could add
> 
> Fixes: b757c6cfc696d ("samples/damon/wsse: start and stop DAMON as the user requests")
> 
> If we want to make stable kernel maintainers' life easier, we would better to
> split this into three patches, for each module, and add appropriate Fixes, as
> below.

Spliting this into three patches is okay.

> 
> For wsse,
> Fixes: b757c6cfc696d ("samples/damon/wsse: start and stop DAMON as the user requests")
> 
> For prcl,
> 
> Fixes: 2aca254620a8 ("samples/damon: introduce a skeleton of a smaple DAMON module for proactive reclamation")
> 
> For mtier,
> 
> Fixes: 82a08bde3cf7 ("samples/damon: implement a DAMON module for memory tiering")
> 
> Could you please do so?

If we do, then this patchset has to come before "enabled" renaming patch.

In addition, I remember mixing stable and non-stable patches together in the
same patchset isn't welcomed by Andrew or other stable tree maintainers.  So I
might be better to split those three fix patches into a separate patchset.

The only thing that bothers me in that case is writing cover letter for simple 2
or 3 patches as they are relatively simple.

But it isn't that hard so can manage those in the next spin.

> 
>>
>> Signed-off-by: Honggyu Kim <honggyu.kim@sk.com>
>> Cc: Yunjeong Mun <yunjeong.mun@sk.com>
> 
> Regardless of your agreement on adding Fixes:
> 
> Reviewed-by: SeongJae Park <sj@kernel.org>
> 
> 
> Thanks,
> SJ

Thanks,
Honggyu

> 
> [...]

Re: [PATCH 1/3] mm/damon: do not allow creating zero size region

[SeongJae Park]

On Mon, 23 Jun 2025 11:58:53 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:

> Hi SeongJae,
> 
> Thanks for the kind review as always!

My pleasure :)

> 
> On 6/23/2025 1:04 AM, SeongJae Park wrote:
> > On Sun, 22 Jun 2025 21:09:23 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:
> > 
> >> Creating zero size region leads a divide by zero error inside
> >> damon_get_intervals_score() as follows.
> >>
> >>    static unsigned long damon_get_intervals_score(struct damon_ctx *c)
> >>    {
> >>        struct damon_target *t;
> >>        struct damon_region *r;
> >>        unsigned long sz_region, max_access_events = 0, access_events = 0;
> >>        unsigned long target_access_events;
> >>        unsigned long goal_bp = c->attrs.intervals_goal.access_bp;
> >>
> >>        damon_for_each_target(t, c) {
> >>            damon_for_each_region(r, t) {
> >>                sz_region = damon_sz_region(r);
> >>                max_access_events += sz_region * c->attrs.aggr_samples;
> >>                access_events += sz_region * r->nr_accesses;
> >>            }
> >>        }
> >>        target_access_events = max_access_events * goal_bp / 10000;
> >>        return access_events * 10000 / target_access_events;  /* divide by zero! */
> >>    }
> > 
> > Thank you for finding this issue!  Coudl you please further share how zero size
> > region can be made, and if user-space can make the situation?
> 
> The initial values of node*_start_addr and node*_end_addr inside
> /sys/module/mtier/parameters/ are all zeros so I saw this problem easily by
> setting Y to "enabled".

Thank you for clarifying!  Please add this description on the commit message if
you send a next version of this patch, with Fixes: tag.

> 
> >>
> >> This patch makes a NULL return for such cases when creating a region
> >> inside damon_new_region().
> > 
> > I agree zero size region could look silly.  But I don't really think it should
> > be prohibited.  What about modifying damon_get_intervals_score() instead?
> > Maybe we can set target_access_events as 1 in this case.
> 
> Hmm... I don't get what you mean by setting "target_access_events" to 1 for such
> cases.  Could you please explain more?

I mean, something like below.

@@ -1449,6 +1449,7 @@ static unsigned long damon_get_intervals_score(struct damon_ctx *c)
                }
        }
        target_access_events = max_access_events * goal_bp / 10000;
+       target_access_events = target_access_events ? : 1;
        return access_events * 10000 / target_access_events;
 }

> 
> > 
> >>
> >> Signed-off-by: Honggyu Kim <honggyu.kim@sk.com>
> >> Cc: Yunjeong Mun <yunjeong.mun@sk.com>
> >> ---
> >>   mm/damon/core.c | 3 +++
> >>   1 file changed, 3 insertions(+)
> >>
> >> diff --git a/mm/damon/core.c b/mm/damon/core.c
> >> index b217e0120e09..44740da337fd 100644
> >> --- a/mm/damon/core.c
> >> +++ b/mm/damon/core.c
> >> @@ -126,8 +126,11 @@ struct damon_region *damon_new_region(unsigned long start, unsigned long end)
> >>        if (!region)
> >>                return NULL;
> >>
> >> +     if (start == end)
> >> +             return NULL;
> If you're okay, then I'd like to modify this as follows again.
> 
> +     if (start >= end)
> +             return NULL;
> 
> What do you think about this?

I still prefer fixing the found bug on the spot.  I don't think having zero or
negative size regions is really somewhat we always prohibit.


Thanks,
SJ

[...]

Re: [PATCH 3/3] samples/damon: fix bugs for damon sample for start failures

[SeongJae Park]

On Mon, 23 Jun 2025 12:16:32 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:

> Hi SeongJae,
> 
> On 6/23/2025 1:29 AM, SeongJae Park wrote:
> > Hi Honggyu,
> > 
> > On Sun, 22 Jun 2025 21:09:25 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:
[...]
> > If we want to make stable kernel maintainers' life easier, we would better to
> > split this into three patches, for each module, and add appropriate Fixes, as
> > below.
> 
> Spliting this into three patches is okay.
[...]
> > Could you please do so?
> 
> If we do, then this patchset has to come before "enabled" renaming patch.
> 
> In addition, I remember mixing stable and non-stable patches together in the
> same patchset isn't welcomed by Andrew or other stable tree maintainers.  So I
> might be better to split those three fix patches into a separate patchset.

I didn't think they would really bother such mixed patch series, but, yes,
please do so.  Being nice to others is cool :)

> 
> The only thing that bothers me in that case is writing cover letter for simple 2
> or 3 patches as they are relatively simple.

I think you could also add the divide-by-zero fix (first patch of this series)
to the series.

> 
> But it isn't that hard so can manage those in the next spin.

Thank you, looking forward to the next revisions!


Thanks,
SJ

[...]

Re: [PATCH 1/3] mm/damon: do not allow creating zero size region

[Honggyu Kim]

Hi SeongJae,

Sorry for the late response.

On 6/24/2025 3:03 AM, SeongJae Park wrote:
> On Mon, 23 Jun 2025 11:58:53 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:
> 
>> Hi SeongJae,
>>
>> Thanks for the kind review as always!
> 
> My pleasure :)
> 
>>
>> On 6/23/2025 1:04 AM, SeongJae Park wrote:
>>> On Sun, 22 Jun 2025 21:09:23 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:
>>>
>>>> Creating zero size region leads a divide by zero error inside
>>>> damon_get_intervals_score() as follows.
>>>>
>>>>     static unsigned long damon_get_intervals_score(struct damon_ctx *c)
>>>>     {
>>>>         struct damon_target *t;
>>>>         struct damon_region *r;
>>>>         unsigned long sz_region, max_access_events = 0, access_events = 0;
>>>>         unsigned long target_access_events;
>>>>         unsigned long goal_bp = c->attrs.intervals_goal.access_bp;
>>>>
>>>>         damon_for_each_target(t, c) {
>>>>             damon_for_each_region(r, t) {
>>>>                 sz_region = damon_sz_region(r);
>>>>                 max_access_events += sz_region * c->attrs.aggr_samples;
>>>>                 access_events += sz_region * r->nr_accesses;
>>>>             }
>>>>         }
>>>>         target_access_events = max_access_events * goal_bp / 10000;
>>>>         return access_events * 10000 / target_access_events;  /* divide by zero! */
>>>>     }
>>>
>>> Thank you for finding this issue!  Coudl you please further share how zero size
>>> region can be made, and if user-space can make the situation?
>>
>> The initial values of node*_start_addr and node*_end_addr inside
>> /sys/module/mtier/parameters/ are all zeros so I saw this problem easily by
>> setting Y to "enabled".
> 
> Thank you for clarifying!  Please add this description on the commit message if
> you send a next version of this patch, with Fixes: tag.

OK. I will add "Fixes: tag.

>>>>
>>>> This patch makes a NULL return for such cases when creating a region
>>>> inside damon_new_region().
>>>
>>> I agree zero size region could look silly.  But I don't really think it should
>>> be prohibited.  What about modifying damon_get_intervals_score() instead?
>>> Maybe we can set target_access_events as 1 in this case.
>>
>> Hmm... I don't get what you mean by setting "target_access_events" to 1 for such
>> cases.  Could you please explain more?
> 
> I mean, something like below.
> 
> @@ -1449,6 +1449,7 @@ static unsigned long damon_get_intervals_score(struct damon_ctx *c)
>                  }
>          }
>          target_access_events = max_access_events * goal_bp / 10000;
> +       target_access_events = target_access_events ? : 1;
>          return access_events * 10000 / target_access_events;
>   }

I actually didn't mean the code, but just wondered if setting 
"target_access_events" to 1 makes sense in this context.

I now think that it doesn't make any difference because applying DAMOS actions
to zero size regions as it's just no-ops.  So I can take your change.

>>>
>>>>
>>>> Signed-off-by: Honggyu Kim <honggyu.kim@sk.com>
>>>> Cc: Yunjeong Mun <yunjeong.mun@sk.com>
>>>> ---
>>>>    mm/damon/core.c | 3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/mm/damon/core.c b/mm/damon/core.c
>>>> index b217e0120e09..44740da337fd 100644
>>>> --- a/mm/damon/core.c
>>>> +++ b/mm/damon/core.c
>>>> @@ -126,8 +126,11 @@ struct damon_region *damon_new_region(unsigned long start, unsigned long end)
>>>>         if (!region)
>>>>                 return NULL;
>>>>
>>>> +     if (start == end)
>>>> +             return NULL;
>> If you're okay, then I'd like to modify this as follows again.
>>
>> +     if (start >= end)
>> +             return NULL;
>>
>> What do you think about this?
> 
> I still prefer fixing the found bug on the spot.  I don't think having zero or
> negative size regions is really somewhat we always prohibit.

I can split "target_access_events" change patch from this regardless of this
with "Fixes" tag.

But I don't get why you think zero size region is acceptable.  Do you see any 
benefits or have special reasons allowing zero size regions?

Thanks,
Honggyu

> 
> 
> Thanks,
> SJ
> 
> [...]
> 

Re: [PATCH 3/3] samples/damon: fix bugs for damon sample for start failures

[Honggyu Kim]

Hi SeongJae,

On 6/24/2025 3:11 AM, SeongJae Park wrote:
> On Mon, 23 Jun 2025 12:16:32 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:
> 
>> Hi SeongJae,
>>
>> On 6/23/2025 1:29 AM, SeongJae Park wrote:
>>> Hi Honggyu,
>>>
>>> On Sun, 22 Jun 2025 21:09:25 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:
> [...]
>>> If we want to make stable kernel maintainers' life easier, we would better to
>>> split this into three patches, for each module, and add appropriate Fixes, as
>>> below.
>>
>> Spliting this into three patches is okay.
> [...]
>>> Could you please do so?
>>
>> If we do, then this patchset has to come before "enabled" renaming patch.
>>
>> In addition, I remember mixing stable and non-stable patches together in the
>> same patchset isn't welcomed by Andrew or other stable tree maintainers.  So I
>> might be better to split those three fix patches into a separate patchset.
> 
> I didn't think they would really bother such mixed patch series, but, yes,
> please do so.  Being nice to others is cool :)

Sure.  I will split "Fixes" changes in a separate patchset.

> 
>>
>> The only thing that bothers me in that case is writing cover letter for simple 2
>> or 3 patches as they are relatively simple.
> 
> I think you could also add the divide-by-zero fix (first patch of this series)
> to the series.

Sure.

>>
>> But it isn't that hard so can manage those in the next spin.
> 
> Thank you, looking forward to the next revisions!

Thank you too.  I will send it within a few days.

Honggyu

> 
> 
> Thanks,
> SJ
> 
> [...]

Re: [PATCH 1/3] mm/damon: do not allow creating zero size region

[SeongJae Park]

On Thu, 26 Jun 2025 07:24:14 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:

> Hi SeongJae,
> 
> Sorry for the late response.

No worry!

> On 6/24/2025 3:03 AM, SeongJae Park wrote:
> > On Mon, 23 Jun 2025 11:58:53 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:
[...]
> > I mean, something like below.
> > 
> > @@ -1449,6 +1449,7 @@ static unsigned long damon_get_intervals_score(struct damon_ctx *c)
> >                  }
> >          }
> >          target_access_events = max_access_events * goal_bp / 10000;
> > +       target_access_events = target_access_events ? : 1;
> >          return access_events * 10000 / target_access_events;
> >   }
> 
> I actually didn't mean the code, but just wondered if setting 
> "target_access_events" to 1 makes sense in this context.
> 
> I now think that it doesn't make any difference because applying DAMOS actions
> to zero size regions as it's just no-ops.  So I can take your change.

Thank you for clarifying, looking forward to your fix! :)

[...]
> > I still prefer fixing the found bug on the spot.  I don't think having zero or
> > negative size regions is really somewhat we always prohibit.
> 
> I can split "target_access_events" change patch from this regardless of this
> with "Fixes" tag.
> 
> But I don't get why you think zero size region is acceptable.  Do you see any 
> benefits or have special reasons allowing zero size regions?

In short, I don't anticipate special benefits of allowing zero size region.
But that's smae to this change.

I even have small concern about this change.

This change might make people assume any damon_region would have non-zero
positive size.  I think that's wrong assumption.  Any DAMON core and API caller
code can set the start and the end addresses with arbitrary values.

I think making the assumption true could be beneficial since it will help
writing code with less corner cases.  But to make the assumption true, we
should first check if any existing code is violating it, and if any existing
code that written with current assumption (region size can be zero) can be
broken.  After that, we should also prevent future code violating it.

And if the assumption becomes truth, we get one more rule.  I, at least, prefer
having as less rules as possible.  My taste may be weird, but this is my humble
feeling and opinion.


Thanks,
SJ

[...]

Re: [PATCH 3/3] samples/damon: fix bugs for damon sample for start failures

[SeongJae Park]

On Thu, 26 Jun 2025 07:27:18 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:
[...] 
> Thank you too.  I will send it within a few days.

Thank you, looking forward to the next version!


Thanks,
SJ

[...]

Re: [PATCH 1/3] mm/damon: do not allow creating zero size region

[Honggyu Kim]

Hi SeongJae,

On 6/27/2025 12:27 AM, SeongJae Park wrote:
> On Thu, 26 Jun 2025 07:24:14 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:
> 
>> Hi SeongJae,
>>
>> Sorry for the late response.
> 
> No worry!
> 
>> On 6/24/2025 3:03 AM, SeongJae Park wrote:
>>> On Mon, 23 Jun 2025 11:58:53 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:
> [...]
>>> I mean, something like below.
>>>
>>> @@ -1449,6 +1449,7 @@ static unsigned long damon_get_intervals_score(struct damon_ctx *c)
>>>                   }
>>>           }
>>>           target_access_events = max_access_events * goal_bp / 10000;
>>> +       target_access_events = target_access_events ? : 1;
>>>           return access_events * 10000 / target_access_events;
>>>    }
>>
>> I actually didn't mean the code, but just wondered if setting
>> "target_access_events" to 1 makes sense in this context.
>>
>> I now think that it doesn't make any difference because applying DAMOS actions
>> to zero size regions as it's just no-ops.  So I can take your change.
> 
> Thank you for clarifying, looking forward to your fix! :)

Will send it within a few days.  Thanks for your patience.

> 
> [...]
>>> I still prefer fixing the found bug on the spot.  I don't think having zero or
>>> negative size regions is really somewhat we always prohibit.
>>
>> I can split "target_access_events" change patch from this regardless of this
>> with "Fixes" tag.
>>
>> But I don't get why you think zero size region is acceptable.  Do you see any
>> benefits or have special reasons allowing zero size regions?
> 
> In short, I don't anticipate special benefits of allowing zero size region.
> But that's smae to this change.
> 
> I even have small concern about this change.
> 
> This change might make people assume any damon_region would have non-zero
> positive size.  I think that's wrong assumption.  Any DAMON core and API caller
> code can set the start and the end addresses with arbitrary values.
> 
> I think making the assumption true could be beneficial since it will help
> writing code with less corner cases.  But to make the assumption true, we
> should first check if any existing code is violating it, and if any existing
> code that written with current assumption (region size can be zero) can be
> broken.  After that, we should also prevent future code violating it.
> 
> And if the assumption becomes truth, we get one more rule.  I, at least, prefer
> having as less rules as possible.  My taste may be weird, but this is my humble
> feeling and opinion.

I see.  You don't want to make breaking changes even in this case, then I can
leave it as is.  Thanks for the explanation!

Thanks,
Honggyu

> 
> 
> Thanks,
> SJ
> 
> [...]
>