[RFC PATCH 0/2] samples/damon: improve expression of target region in mtier

[RFC PATCH 0/2] samples/damon: improve expression of target region in mtier

[Yunjeong Mun]

`mtier` module is a static module for migrating pages using
damos_action: DAMOS_MIGRATE_HOT and DAMOS_MIGRATE_COLD. Currently,
this module is built on the assumption that the system has two NUMA
nodes, where node0 is the fast-tier and node1 is slow-tier. This
patchset aims to make the expression of the migration target region
more user-friendly by : 1) allowing users to specify just the node id,
instead of physical address, and 2) adding two new knobs.

Patch 1 removes four knobs, `node#_start_addr` and `node#_end_addr`, which
previously required users to input the physical address of the region.
It introduces logic to conver NUMA node ids to physical address.

Patch 2 introduces two new knobs `migrate_hot` and `migrate_cold`, which
specify the source and destination node ids in a comma-separated format.

Yunjeong Mun (2):
  samples/damon: convert node id to physical address
  samples/damon: add `migrate_hot` and `migrate_cold` knobs

 samples/damon/mtier.c | 108 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 93 insertions(+), 15 deletions(-)


base-commit: db16fe88cdf83a1e7fdf75de282025b6ad61d08f
-- 
2.34.1

[RFC PATCH 1/2] samples/damon: convert node id to physical address

[Yunjeong Mun]

This patch removes the `node#_start_addr` and `node#_end_addr` knobs,
and introduces logic for converting numa node id to physical address.
It only checks whether a numa node is online and calculates the
start and end addresses of the node. It does not verify whether each
memory block within the numa node is `usable` or part of `System RAM`,
as performed by `damo` [1],[2].

[1]
https://github.com/damonitor/damo/blob/v2.8.5/src/damo_pa_layout.py#L72-L90
[2]
https://github.com/damonitor/damo/blob/v2.8.5/src/damo_pa_layout.py#L92-L10

Suggested-by: Honggyu Kim <honggyu.kim@sk.com>
Signed-off-by: Yunjeong Mun <yunjeong.mun@sk.com>
---
 samples/damon/mtier.c | 44 ++++++++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 15 deletions(-)

diff --git a/samples/damon/mtier.c b/samples/damon/mtier.c
index f3220d6e6739..ba6938a89c21 100644
--- a/samples/damon/mtier.c
+++ b/samples/damon/mtier.c
@@ -12,18 +12,6 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 
-static unsigned long node0_start_addr __read_mostly;
-module_param(node0_start_addr, ulong, 0600);
-
-static unsigned long node0_end_addr __read_mostly;
-module_param(node0_end_addr, ulong, 0600);
-
-static unsigned long node1_start_addr __read_mostly;
-module_param(node1_start_addr, ulong, 0600);
-
-static unsigned long node1_end_addr __read_mostly;
-module_param(node1_end_addr, ulong, 0600);
-
 static unsigned long node0_mem_used_bp __read_mostly = 9970;
 module_param(node0_mem_used_bp, ulong, 0600);
 
@@ -44,6 +32,28 @@ MODULE_PARM_DESC(enable, "Enable of disable DAMON_SAMPLE_MTIER");
 
 static struct damon_ctx *ctxs[2];
 
+struct region_range {
+	phys_addr_t start;
+	phys_addr_t end;
+};
+
+static int numa_info_init(int target_node, struct region_range *range) {
+
+	if (!node_online(target_node)) {
+		pr_err("NUMA node %d is not online\n", target_node);
+		return -EINVAL;
+	}
+
+	/* TODO: Do we need to support more accurate region range?  */
+	unsigned long start_pfn = node_start_pfn(target_node);
+	unsigned long end_pfn   = node_end_pfn(target_node);
+
+	range->start = (phys_addr_t)start_pfn << PAGE_SHIFT;
+	range->end  = (phys_addr_t)end_pfn << PAGE_SHIFT;
+
+	return 0;
+}
+
 static struct damon_ctx *damon_sample_mtier_build_ctx(bool promote)
 {
 	struct damon_ctx *ctx;
@@ -53,6 +63,7 @@ static struct damon_ctx *damon_sample_mtier_build_ctx(bool promote)
 	struct damos *scheme;
 	struct damos_quota_goal *quota_goal;
 	struct damos_filter *filter;
+	struct region_range addr;
 
 	ctx = damon_new_ctx();
 	if (!ctx)
@@ -82,9 +93,12 @@ static struct damon_ctx *damon_sample_mtier_build_ctx(bool promote)
 	if (!target)
 		goto free_out;
 	damon_add_target(ctx, target);
-	region = damon_new_region(
-			promote ? node1_start_addr : node0_start_addr,
-			promote ? node1_end_addr : node0_end_addr);
+
+	int ret = promote ? numa_info_init(1, &addr) : numa_info_init(0, &addr);
+	if (ret)
+		goto free_out;
+
+	region = damon_new_region(addr.start, addr.end);
 	if (!region)
 		goto free_out;
 	damon_add_region(region, target);
-- 
2.34.1

[RFC PATCH 2/2] samples/damon: add `migrate_hot` and `migrate_cold` knobs

[Yunjeong Mun]

This patch introduces two new konbs for promotion/demotion:
`migrate_hot` and `migrate_cold`. It receives node ids for migration in
a comma-separated format as `<src,dst>`. The usage is as follows:

    # demote pages from nid 0 to nid 1
    $ echo 0,1 > /sys/module/mtier/parameters/migrate_cold
    # promote pages from nid 1 to nid 0
    $ echo 1,0 > /sys/module/mtier/parameters/migrate_hot

Susggested-by: Honggyu Kim <honggyu.kim@sk.com>
Signed-off-by: Yunjeong Mun <yunjeong.mun@sk.com>
---
 samples/damon/mtier.c | 68 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 66 insertions(+), 2 deletions(-)

diff --git a/samples/damon/mtier.c b/samples/damon/mtier.c
index ba6938a89c21..55d2edb84d7e 100644
--- a/samples/damon/mtier.c
+++ b/samples/damon/mtier.c
@@ -11,6 +11,7 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/string.h>
 
 static unsigned long node0_mem_used_bp __read_mostly = 9970;
 module_param(node0_mem_used_bp, ulong, 0600);
@@ -18,6 +19,32 @@ module_param(node0_mem_used_bp, ulong, 0600);
 static unsigned long node0_mem_free_bp __read_mostly = 50;
 module_param(node0_mem_free_bp, ulong, 0600);
 
+static int get_migrate_hot(
+		char *val, const struct kernel_param *kp);
+
+static const struct kernel_param_ops migrate_hot_ops = {
+	.set = param_set_charp,
+	.get = get_migrate_hot,
+};
+
+static char *migrate_hot __read_mostly = "";
+module_param_cb(migrate_hot, &migrate_hot_ops, &migrate_hot, 0600);
+MODULE_PARM_DESC(migrate_hot,
+	"Specify source and destination node id as a comma-seperated pair");
+
+static int get_migrate_cold(
+		char *val, const struct kernel_param *kp);
+
+static const struct kernel_param_ops migrate_cold_ops = {
+	.set = param_set_charp,
+	.get = get_migrate_cold,
+};
+
+static char *migrate_cold __read_mostly = "";
+module_param_cb(migrate_cold, &migrate_cold_ops, &migrate_cold, 0600);
+MODULE_PARM_DESC(migrate_cold,
+	"Specify source and destination node id as a comma-seperated pair");
+
 static int damon_sample_mtier_enable_store(
 		const char *val, const struct kernel_param *kp);
 
@@ -37,6 +64,30 @@ struct region_range {
 	phys_addr_t end;
 };
 
+static int parse_migrate_node(int *src, int *dst, bool promote) {
+	char *comma;
+	char buf[32];
+
+	if (promote)
+		strscpy(buf, migrate_hot, sizeof(buf));
+	else
+		strscpy(buf, migrate_cold, sizeof(buf));
+
+	comma = strchr(buf, ',');
+	if (!comma)
+		return -EINVAL;
+
+	*comma = '\0';
+	comma++;
+
+	if (kstrtoint(buf, 0, src))
+		return -EINVAL;
+	if (kstrtoint(comma, 0, dst))
+		return -EINVAL;
+
+	return 0;
+}
+
 static int numa_info_init(int target_node, struct region_range *range) {
 
 	if (!node_online(target_node)) {
@@ -64,6 +115,7 @@ static struct damon_ctx *damon_sample_mtier_build_ctx(bool promote)
 	struct damos_quota_goal *quota_goal;
 	struct damos_filter *filter;
 	struct region_range addr;
+	int src, dst;
 
 	ctx = damon_new_ctx();
 	if (!ctx)
@@ -94,8 +146,10 @@ static struct damon_ctx *damon_sample_mtier_build_ctx(bool promote)
 		goto free_out;
 	damon_add_target(ctx, target);
 
-	int ret = promote ? numa_info_init(1, &addr) : numa_info_init(0, &addr);
-	if (ret)
+	if (parse_migrate_node(&src, &dst, promote))
+		goto free_out;
+
+	if (numa_info_init(src, &addr))
 		goto free_out;
 
 	region = damon_new_region(addr.start, addr.end);
@@ -171,6 +225,16 @@ static void damon_sample_mtier_stop(void)
 	damon_destroy_ctx(ctxs[1]);
 }
 
+static int get_migrate_hot(char *buf, const struct kernel_param *kp)
+{
+       return scnprintf(buf, PAGE_SIZE, "%s", migrate_hot);
+}
+
+static int get_migrate_cold(char *buf, const struct kernel_param *kp)
+{
+       return scnprintf(buf, PAGE_SIZE, "%s", migrate_cold);
+}
+
 static int damon_sample_mtier_enable_store(
 		const char *val, const struct kernel_param *kp)
 {
-- 
2.34.1

Re: [RFC PATCH 0/2] samples/damon: improve expression of target region in mtier

[SeongJae Park]

Hello Yunjeong,


On Tue,  1 Jul 2025 17:54:15 +0900 Yunjeong Mun <yunjeong.mun@sk.com> wrote:

> `mtier` module is a static module for migrating pages using
> damos_action: DAMOS_MIGRATE_HOT and DAMOS_MIGRATE_COLD. Currently,
> this module is built on the assumption that the system has two NUMA
> nodes, where node0 is the fast-tier and node1 is slow-tier. This
> patchset aims to make the expression of the migration target region
> more user-friendly by :

Thank you for your continued patches!  Let me clarify my humble opinions about
what is the goal of mtier, and what improvements it needs first, though.

mtier is not intended to be used as a solution for real world usages but a
sample module for helping developers understand how they can utilize DAMON by
calling its API functions.  If someone is looking for a real world DAMON-based
memory tiering solution, I think it should be under mm/damon/ directory like
DAMON_RECLAIM, not under samples/damon/ directory.

I hence think we should more focus on making mtier provides good example usages
of DAMON API functions, more than its user-space usages.  We could of course
improve its user-space interface if it helps experimental run of it for better
understanding of the code and DAMON API usages.  But in my opinion, it should
avoid a case that it result in adding more complicated code that makes reading
of the example difficult.

Please let me know if you have different opinions and I'm missing something.

From that perspective, I think the assumption of the fast node0 and the slow
node1 is not a particular problem but helps keeping the sample code easier to
read.  Of course it has rooms to improve, like having more documentation or
better names and cleaner code.

> 1) allowing users to specify just the node id,
> instead of physical address, and 2) adding two new knobs.
> 
> Patch 1 removes four knobs, `node#_start_addr` and `node#_end_addr`, which
> previously required users to input the physical address of the region.
> It introduces logic to conver NUMA node ids to physical address.

I think this is good change.  But even though mtier is a sample module, I'd
like to keep the old usages not broken if possible.  I'll leave more comments
on it as a reply to the patch.

> 
> Patch 2 introduces two new knobs `migrate_hot` and `migrate_cold`, which
> specify the source and destination node ids in a comma-separated format.

As mentioned abovely, I think the assumption of the fast node0 and the slow
node1 is particularly bad, so I'm sorry but I'm not really exciting with this
change.  I'll leave more comments as a reply to the patch.

> 
> Yunjeong Mun (2):
>   samples/damon: convert node id to physical address
>   samples/damon: add `migrate_hot` and `migrate_cold` knobs
> 
>  samples/damon/mtier.c | 108 ++++++++++++++++++++++++++++++++++++------
>  1 file changed, 93 insertions(+), 15 deletions(-)
> 
> 
> base-commit: db16fe88cdf83a1e7fdf75de282025b6ad61d08f
> -- 
> 2.34.1


Thanks,
SJ

Re: [RFC PATCH 1/2] samples/damon: convert node id to physical address

[SeongJae Park]

Hi Yunjeong,

On Tue,  1 Jul 2025 17:54:16 +0900 Yunjeong Mun <yunjeong.mun@sk.com> wrote:

> This patch removes the `node#_start_addr` and `node#_end_addr` knobs,
> and introduces logic for converting numa node id to physical address.
> It only checks whether a numa node is online and calculates the
> start and end addresses of the node. It does not verify whether each
> memory block within the numa node is `usable` or part of `System RAM`,
> as performed by `damo` [1],[2].

This is just a sample module, but I'd like to avoid making unnecessary
user-breaking changes.  How about keeping the existing knobs but adding yet
another knob for the automatic detection, say, 'detect_node_addresses'?

> 
> [1]
> https://github.com/damonitor/damo/blob/v2.8.5/src/damo_pa_layout.py#L72-L90
> [2]
> https://github.com/damonitor/damo/blob/v2.8.5/src/damo_pa_layout.py#L92-L10
> 
> Suggested-by: Honggyu Kim <honggyu.kim@sk.com>
> Signed-off-by: Yunjeong Mun <yunjeong.mun@sk.com>
> ---
>  samples/damon/mtier.c | 44 ++++++++++++++++++++++++++++---------------
>  1 file changed, 29 insertions(+), 15 deletions(-)
> 
> diff --git a/samples/damon/mtier.c b/samples/damon/mtier.c
> index f3220d6e6739..ba6938a89c21 100644
> --- a/samples/damon/mtier.c
> +++ b/samples/damon/mtier.c
> @@ -12,18 +12,6 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  
> -static unsigned long node0_start_addr __read_mostly;
> -module_param(node0_start_addr, ulong, 0600);
> -
> -static unsigned long node0_end_addr __read_mostly;
> -module_param(node0_end_addr, ulong, 0600);
> -
> -static unsigned long node1_start_addr __read_mostly;
> -module_param(node1_start_addr, ulong, 0600);
> -
> -static unsigned long node1_end_addr __read_mostly;
> -module_param(node1_end_addr, ulong, 0600);
> -
>  static unsigned long node0_mem_used_bp __read_mostly = 9970;
>  module_param(node0_mem_used_bp, ulong, 0600);
>  
> @@ -44,6 +32,28 @@ MODULE_PARM_DESC(enable, "Enable of disable DAMON_SAMPLE_MTIER");
>  
>  static struct damon_ctx *ctxs[2];
>  
> +struct region_range {
> +	phys_addr_t start;
> +	phys_addr_t end;
> +};
> +
> +static int numa_info_init(int target_node, struct region_range *range) {
> +

checkpatch.pl complaints as below.

ERROR: open brace '{' following function definitions go on the next line
#82: FILE: samples/damon/mtier.c:40:
+static int numa_info_init(int target_node, struct region_range *range) {

> +	if (!node_online(target_node)) {
> +		pr_err("NUMA node %d is not online\n", target_node);
> +		return -EINVAL;
> +	}
> +
> +	/* TODO: Do we need to support more accurate region range?  */
> +	unsigned long start_pfn = node_start_pfn(target_node);
> +	unsigned long end_pfn   = node_end_pfn(target_node);
> +
> +	range->start = (phys_addr_t)start_pfn << PAGE_SHIFT;
> +	range->end  = (phys_addr_t)end_pfn << PAGE_SHIFT;

Let's use PHYS_PFN() instead.

> +
> +	return 0;
> +}
> +
>  static struct damon_ctx *damon_sample_mtier_build_ctx(bool promote)
>  {
>  	struct damon_ctx *ctx;
> @@ -53,6 +63,7 @@ static struct damon_ctx *damon_sample_mtier_build_ctx(bool promote)
>  	struct damos *scheme;
>  	struct damos_quota_goal *quota_goal;
>  	struct damos_filter *filter;
> +	struct region_range addr;
>  
>  	ctx = damon_new_ctx();
>  	if (!ctx)
> @@ -82,9 +93,12 @@ static struct damon_ctx *damon_sample_mtier_build_ctx(bool promote)
>  	if (!target)
>  		goto free_out;
>  	damon_add_target(ctx, target);
> -	region = damon_new_region(
> -			promote ? node1_start_addr : node0_start_addr,
> -			promote ? node1_end_addr : node0_end_addr);
> +
> +	int ret = promote ? numa_info_init(1, &addr) : numa_info_init(0, &addr);
> +	if (ret)
> +		goto free_out;

Yet another checkpatch.pl complain.

WARNING: Missing a blank line after declarations
#119: FILE: samples/damon/mtier.c:98:
+       int ret = promote ? numa_info_init(1, &addr) : numa_info_init(0, &addr);
+       if (ret)

> +
> +	region = damon_new_region(addr.start, addr.end);
>  	if (!region)
>  		goto free_out;
>  	damon_add_region(region, target);
> -- 
> 2.34.1


Thanks,
SJ

Re: [RFC PATCH 2/2] samples/damon: add `migrate_hot` and `migrate_cold` knobs

[SeongJae Park]

Hello Yunjeong,

On Tue,  1 Jul 2025 17:54:17 +0900 Yunjeong Mun <yunjeong.mun@sk.com> wrote:

> This patch introduces two new konbs for promotion/demotion:
> `migrate_hot` and `migrate_cold`. It receives node ids for migration in
> a comma-separated format as `<src,dst>`. The usage is as follows:
> 
>     # demote pages from nid 0 to nid 1
>     $ echo 0,1 > /sys/module/mtier/parameters/migrate_cold
>     # promote pages from nid 1 to nid 0
>     $ echo 1,0 > /sys/module/mtier/parameters/migrate_hot

I understand you want to support some setups such as having a fast node of id 1
and a slow node of id 0.

Because mtier is a sample code, I think it's primary goal is to help developers
know how they can use DAMON API functions for writing kernel code that is
required for a situation similar to the sample.  Hence the sample should be
clean and simple enough to be understood.

The assumption of the two nodes (the fast node 0 and the slow node 1) is
arguably intended for making the code simple, in my opinion.  We could of
course make this kind of changes if it helps more experiments for understanding
the code, as sample code is not only for reading but also running.

> 
> Susggested-by: Honggyu Kim <honggyu.kim@sk.com>

checkpatch.pl found a typo: s/Susggest/Suggest/

> Signed-off-by: Yunjeong Mun <yunjeong.mun@sk.com>
> ---
>  samples/damon/mtier.c | 68 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 66 insertions(+), 2 deletions(-)

But, I feel like this change is rather making code too longer and complicated.
Hence I would suggest dropping this patch if you agree.  Please let me know if
you have different opinions, or I'm missing something.


Thanks,
SJ

[...]

Re: [RFC PATCH 1/2] samples/damon: convert node id to physical address

[Yunjeong Mun]

Hi Seongjae, thanks for your review :)

On Tue,  1 Jul 2025 16:54:07 -0700 SeongJae Park <sj@kernel.org> wrote:
> Hi Yunjeong,
> 
> On Tue,  1 Jul 2025 17:54:16 +0900 Yunjeong Mun <yunjeong.mun@sk.com> wrote:
> 
> > This patch removes the `node#_start_addr` and `node#_end_addr` knobs,
> > and introduces logic for converting numa node id to physical address.
> > It only checks whether a numa node is online and calculates the
> > start and end addresses of the node. It does not verify whether each
> > memory block within the numa node is `usable` or part of `System RAM`,
> > as performed by `damo` [1],[2].
> 
> This is just a sample module, but I'd like to avoid making unnecessary
> user-breaking changes.  How about keeping the existing knobs but adding yet
> another knob for the automatic detection, say, 'detect_node_addresses'?
> 

I agree. From my understanding, 'detect_node_addresses' can be set to 
either 'Y' or 'N'. If it is set to 'Y', mtier converts node0 and node1
to their physical addresses internally. If it is set to 'N', it uses
the existing knobs. Is that correct? 

> > 
> > [1]
> > https://github.com/damonitor/damo/blob/v2.8.5/src/damo_pa_layout.py#L72-L90
> > [2]
> > https://github.com/damonitor/damo/blob/v2.8.5/src/damo_pa_layout.py#L92-L10
> > 
> > Suggested-by: Honggyu Kim <honggyu.kim@sk.com>
> > Signed-off-by: Yunjeong Mun <yunjeong.mun@sk.com>
> > ---
> >  samples/damon/mtier.c | 44 ++++++++++++++++++++++++++++---------------
> >  1 file changed, 29 insertions(+), 15 deletions(-)
> > 
> > diff --git a/samples/damon/mtier.c b/samples/damon/mtier.c
> > index f3220d6e6739..ba6938a89c21 100644
> > --- a/samples/damon/mtier.c
> > +++ b/samples/damon/mtier.c
> > @@ -12,18 +12,6 @@
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  
> > -static unsigned long node0_start_addr __read_mostly;
> > -module_param(node0_start_addr, ulong, 0600);
> > -
> > -static unsigned long node0_end_addr __read_mostly;
> > -module_param(node0_end_addr, ulong, 0600);
> > -
> > -static unsigned long node1_start_addr __read_mostly;
> > -module_param(node1_start_addr, ulong, 0600);
> > -
> > -static unsigned long node1_end_addr __read_mostly;
> > -module_param(node1_end_addr, ulong, 0600);
> > -
> >  static unsigned long node0_mem_used_bp __read_mostly = 9970;
> >  module_param(node0_mem_used_bp, ulong, 0600);
> >  
> > @@ -44,6 +32,28 @@ MODULE_PARM_DESC(enable, "Enable of disable DAMON_SAMPLE_MTIER");
> >  
> >  static struct damon_ctx *ctxs[2];
> >  
> > +struct region_range {
> > +	phys_addr_t start;
> > +	phys_addr_t end;
> > +};
> > +
> > +static int numa_info_init(int target_node, struct region_range *range) {
> > +
> 
> checkpatch.pl complaints as below.
> 
> ERROR: open brace '{' following function definitions go on the next line
> #82: FILE: samples/damon/mtier.c:40:
> +static int numa_info_init(int target_node, struct region_range *range) {
> 
> > +	if (!node_online(target_node)) {
> > +		pr_err("NUMA node %d is not online\n", target_node);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* TODO: Do we need to support more accurate region range?  */
> > +	unsigned long start_pfn = node_start_pfn(target_node);
> > +	unsigned long end_pfn   = node_end_pfn(target_node);
> > +
> > +	range->start = (phys_addr_t)start_pfn << PAGE_SHIFT;
> > +	range->end  = (phys_addr_t)end_pfn << PAGE_SHIFT;
> 

I'll fix them in the next version.

> Let's use PHYS_PFN() instead.

Okay, I'm going to change it with PFN_PHYS()

> > +
> > +	return 0;
> > +}
> > +
> >  static struct damon_ctx *damon_sample_mtier_build_ctx(bool promote)
> >  {
> >  	struct damon_ctx *ctx;
> > @@ -53,6 +63,7 @@ static struct damon_ctx *damon_sample_mtier_build_ctx(bool promote)
> >  	struct damos *scheme;
> >  	struct damos_quota_goal *quota_goal;
> >  	struct damos_filter *filter;
> > +	struct region_range addr;
> >  
> >  	ctx = damon_new_ctx();
> >  	if (!ctx)
> > @@ -82,9 +93,12 @@ static struct damon_ctx *damon_sample_mtier_build_ctx(bool promote)
> >  	if (!target)
> >  		goto free_out;
> >  	damon_add_target(ctx, target);
> > -	region = damon_new_region(
> > -			promote ? node1_start_addr : node0_start_addr,
> > -			promote ? node1_end_addr : node0_end_addr);
> > +
> > +	int ret = promote ? numa_info_init(1, &addr) : numa_info_init(0, &addr);
> > +	if (ret)
> > +		goto free_out;
> 
> Yet another checkpatch.pl complain.
> 
> WARNING: Missing a blank line after declarations
> #119: FILE: samples/damon/mtier.c:98:
> +       int ret = promote ? numa_info_init(1, &addr) : numa_info_init(0, &addr);
> +       if (ret)
> 
> > +
> > +	region = damon_new_region(addr.start, addr.end);
> >  	if (!region)
> >  		goto free_out;
> >  	damon_add_region(region, target);
> > -- 
> > 2.34.1
> 
> 

Thanks for your detailed reviw! Next time, I will make sure to check 
checkpatch.pl:)

Best Regards,
Yunjeong Mun

> Thanks,
> SJ

Re: [RFC PATCH 2/2] samples/damon: add `migrate_hot` and `migrate_cold` knobs

[Yunjeong Mun]

Hello SeongJae,

On Tue,  1 Jul 2025 17:08:25 -0700 SeongJae Park <sj@kernel.org> wrote:
> Hello Yunjeong,
> 
> On Tue,  1 Jul 2025 17:54:17 +0900 Yunjeong Mun <yunjeong.mun@sk.com> wrote:
> 
> > This patch introduces two new konbs for promotion/demotion:
> > `migrate_hot` and `migrate_cold`. It receives node ids for migration in
> > a comma-separated format as `<src,dst>`. The usage is as follows:
> > 
> >     # demote pages from nid 0 to nid 1
> >     $ echo 0,1 > /sys/module/mtier/parameters/migrate_cold
> >     # promote pages from nid 1 to nid 0
> >     $ echo 1,0 > /sys/module/mtier/parameters/migrate_hot
> 
> I understand you want to support some setups such as having a fast node of id 1
> and a slow node of id 0.
> 
> Because mtier is a sample code, I think it's primary goal is to help developers
> know how they can use DAMON API functions for writing kernel code that is
> required for a situation similar to the sample.  Hence the sample should be
> clean and simple enough to be understood.

Thanks for explanation. I understand the intention of mtier module.
As you mentioned, mtier is helpful for leraning how to configure and 
run DAMON.

> 
> The assumption of the two nodes (the fast node 0 and the slow node 1) is
> arguably intended for making the code simple, in my opinion.  We could of
> course make this kind of changes if it helps more experiments for understanding
> the code, as sample code is not only for reading but also running.
> 

I also think that fast node 0 and the slow node 1 is intuitive and easy to
understand the code.

> > 
> > Susggested-by: Honggyu Kim <honggyu.kim@sk.com>
> 
> checkpatch.pl found a typo: s/Susggest/Suggest/
> 
> > Signed-off-by: Yunjeong Mun <yunjeong.mun@sk.com>
> > ---
> >  samples/damon/mtier.c | 68 +++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 66 insertions(+), 2 deletions(-)
> 
> But, I feel like this change is rather making code too longer and complicated.
> Hence I would suggest dropping this patch if you agree.  Please let me know if
> you have different opinions, or I'm missing something.
> 

Ok, I will drop this patch in next version RFC. As you mentiond, mtier
module should be simple and intuitive enough. I will consider this
kind of tiering module to mm/damon. I'll send you RFC v2 soon.

Best Regards,
Yunjeong Mun

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

Re: [RFC PATCH 1/2] samples/damon: convert node id to physical address

[SeongJae Park]

On Thu,  3 Jul 2025 14:18:33 +0900 Yunjeong Mun <yunjeong.mun@sk.com> wrote:

> Hi Seongjae, thanks for your review :)

My pleasure! :)

> 
> On Tue,  1 Jul 2025 16:54:07 -0700 SeongJae Park <sj@kernel.org> wrote:
> > Hi Yunjeong,
> > 
> > On Tue,  1 Jul 2025 17:54:16 +0900 Yunjeong Mun <yunjeong.mun@sk.com> wrote:
> > 
> > > This patch removes the `node#_start_addr` and `node#_end_addr` knobs,
> > > and introduces logic for converting numa node id to physical address.
> > > It only checks whether a numa node is online and calculates the
> > > start and end addresses of the node. It does not verify whether each
> > > memory block within the numa node is `usable` or part of `System RAM`,
> > > as performed by `damo` [1],[2].
> > 
> > This is just a sample module, but I'd like to avoid making unnecessary
> > user-breaking changes.  How about keeping the existing knobs but adding yet
> > another knob for the automatic detection, say, 'detect_node_addresses'?
> > 
> 
> I agree. From my understanding, 'detect_node_addresses' can be set to 
> either 'Y' or 'N'. If it is set to 'Y', mtier converts node0 and node1
> to their physical addresses internally. If it is set to 'N', it uses
> the existing knobs. Is that correct? 

Yes, you're correctly understanding my suggestion.

[...]
> Thanks for your detailed reviw! Next time, I will make sure to check 
> checkpatch.pl:)

Looking forward to your next patches!  Fyi, I use 'hkml patch format', which
automatically runs[1] checkpatch.pl and misc checks for common mistakes.

[1] https://github.com/sjp38/hackermail/blob/master/USAGE.md#checker-run


Thanks,
SJ

[...]

Re: [RFC PATCH 2/2] samples/damon: add `migrate_hot` and `migrate_cold` knobs

[SeongJae Park]

On Thu,  3 Jul 2025 14:18:34 +0900 Yunjeong Mun <yunjeong.mun@sk.com> wrote:

> Hello SeongJae,
> 
> On Tue,  1 Jul 2025 17:08:25 -0700 SeongJae Park <sj@kernel.org> wrote:
> > Hello Yunjeong,
> > 
> > On Tue,  1 Jul 2025 17:54:17 +0900 Yunjeong Mun <yunjeong.mun@sk.com> wrote:
[...]
> > Because mtier is a sample code, I think it's primary goal is to help developers
> > know how they can use DAMON API functions for writing kernel code that is
> > required for a situation similar to the sample.  Hence the sample should be
> > clean and simple enough to be understood.
> 
> Thanks for explanation. I understand the intention of mtier module.
> As you mentioned, mtier is helpful for leraning how to configure and 
> run DAMON.

Glad to hear this sample is working for the intended goal!

[...]
> > But, I feel like this change is rather making code too longer and complicated.
> > Hence I would suggest dropping this patch if you agree.  Please let me know if
> > you have different opinions, or I'm missing something.
> > 
> 
> Ok, I will drop this patch in next version RFC. As you mentiond, mtier
> module should be simple and intuitive enough. I will consider this
> kind of tiering module to mm/damon. I'll send you RFC v2 soon.

Thank you for kindly accepting my suggestion.  Looking forward to the next
patches!


Thanks,
SJ

[...]