[RFC PATCH v2] samples/damon: support automatic node address detection

[RFC PATCH v2] samples/damon: support automatic node address detection

[Yunjeong Mun]

This patch adds a new knob `detect_node_addresses`, which determines
whether the physical address range is set manually using the existing
knobs or automatically by the mtier module. When `detect_node_addresses`
set to 'Y', mtier automatically converts node0 and node1 to their
physical addresses. If set to 'N', it uses the existing
'node#_start_addr' and 'node#_end_addr' to define regions as before.

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

diff --git a/samples/damon/mtier.c b/samples/damon/mtier.c
index f3220d6e6739..3570ebe10fab 100644
--- a/samples/damon/mtier.c
+++ b/samples/damon/mtier.c
@@ -42,8 +42,34 @@ 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 detect_node_addresses __read_mostly;
+module_param(detect_node_addresses, bool, 0600);
+
 static struct damon_ctx *ctxs[2];
 
+struct region_range {
+	phys_addr_t start;
+	phys_addr_t end;
+};
+
+static int nid_to_phys(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 = PFN_PHYS(start_pfn);
+	range->end  = PFN_PHYS(end_pfn);
+
+	return 0;
+}
+
 static struct damon_ctx *damon_sample_mtier_build_ctx(bool promote)
 {
 	struct damon_ctx *ctx;
@@ -53,6 +79,8 @@ 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;
+	int ret;
 
 	ctx = damon_new_ctx();
 	if (!ctx)
@@ -82,9 +110,17 @@ 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);
+
+	if (detect_node_addresses) {
+		ret = promote ? nid_to_phys(1, &addr) : nid_to_phys(0, &addr);
+		if (ret)
+			goto free_out;
+	} else {
+		addr.start = promote ? node1_start_addr : node0_start_addr;
+		addr.end = promote ? node1_end_addr : node0_end_addr;
+	}
+
+	region = damon_new_region(addr.start, addr.end);
 	if (!region)
 		goto free_out;
 	damon_add_region(region, target);

base-commit: db16fe88cdf83a1e7fdf75de282025b6ad61d08f
-- 
2.34.1

Re: [RFC PATCH v2] samples/damon: support automatic node address detection

[SeongJae Park]

Hello Yunjeong,

On Thu,  3 Jul 2025 16:44:22 +0900 Yunjeong Mun <yunjeong.mun@sk.com> wrote:

> This patch adds a new knob `detect_node_addresses`, which determines
> whether the physical address range is set manually using the existing
> knobs or automatically by the mtier module. When `detect_node_addresses`
> set to 'Y', mtier automatically converts node0 and node1 to their
> physical addresses. If set to 'N', it uses the existing
> 'node#_start_addr' and 'node#_end_addr' to define regions as before.

Thank you for this patch!

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

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

> ---

From next time, please consider adding a summary of what changes have made from
the previous version here, like suggested[1] on the documentation.

>  samples/damon/mtier.c | 42 +++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/samples/damon/mtier.c b/samples/damon/mtier.c
> index f3220d6e6739..3570ebe10fab 100644
> --- a/samples/damon/mtier.c
> +++ b/samples/damon/mtier.c
> @@ -42,8 +42,34 @@ 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 detect_node_addresses __read_mostly;
> +module_param(detect_node_addresses, bool, 0600);
> +
>  static struct damon_ctx *ctxs[2];
>  
> +struct region_range {
> +	phys_addr_t start;
> +	phys_addr_t end;
> +};
> +
> +static int nid_to_phys(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?  */

I understand you are saying we might need to remove address ranges in the node
that DAMON will anyway unable to check accesses, e.g., reserved memory.  Since
those are uusally only a small portion and this is a sample code, I think we
don't really need to do that, so I think you can drop this TODO comment from
your next version, if you are gonna make it.

> +	unsigned long start_pfn = node_start_pfn(target_node);
> +	unsigned long end_pfn   = node_end_pfn(target_node);
> +
> +	range->start = PFN_PHYS(start_pfn);
> +	range->end  = PFN_PHYS(end_pfn);
> +
> +	return 0;
> +}
[...]

[1] https://docs.kernel.org/process/submitting-patches.html#commentary


Thanks,
SJ

Re: [RFC PATCH v2] samples/damon: support automatic node address detection

[Yunjeong Mun]

Hello Seongjae,

On Thu,  3 Jul 2025 09:52:37 -0700 SeongJae Park <sj@kernel.org> wrote:
> Hello Yunjeong,
> 
> On Thu,  3 Jul 2025 16:44:22 +0900 Yunjeong Mun <yunjeong.mun@sk.com> wrote:
> 
> > This patch adds a new knob `detect_node_addresses`, which determines
> > whether the physical address range is set manually using the existing
> > knobs or automatically by the mtier module. When `detect_node_addresses`
> > set to 'Y', mtier automatically converts node0 and node1 to their
> > physical addresses. If set to 'N', it uses the existing
> > 'node#_start_addr' and 'node#_end_addr' to define regions as before.
> 
> Thank you for this patch!
> 
> > 
> > Suggested-by: Honggyu Kim <honggyu.kim@sk.com>
> > Signed-off-by: Yunjeong Mun <yunjeong.mun@sk.com>
> 
> Reviewed-by: SeongJae Park <sj@kernel.org>
> 
> > ---
> 
> From next time, please consider adding a summary of what changes have made from
> the previous version here, like suggested[1] on the documentation.

Ok, I'll add it next time, thanks:)
One concern I have about this patch is the requirement to set
'detect_node_addresses=Y' before setting 'enable=Y'. Not following
this order causes an error, which makes it difficult for users to use
the module. So, how about removing 'detect_node_address'? Instead, we
could convert node0,1 to physical address automatically by default, and use
existing 'node#_*_addr' values only when those files are explicitly set.
The diff is as follows:

```diff
diff --git a/samples/damon/mtier.c b/samples/damon/mtier.c
index 3a6ae78efafe..03e0540611e0 100644
--- a/samples/damon/mtier.c
+++ b/samples/damon/mtier.c
@@ -42,9 +42,6 @@ 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 detect_node_addresses __read_mostly;
-module_param(detect_node_addresses, bool, 0600);
-
 static struct damon_ctx *ctxs[2];

 struct region_range {
@@ -110,10 +107,12 @@ static struct damon_ctx *damon_sample_mtier_build_ctx(bool promote)
                goto free_out;
        damon_add_target(ctx, target);

-       if (detect_node_addresses) {
+       if (!node0_start_addr && !node0_end_addr
+                               && !node1_start_addr && !node1_end_addr) {
                ret = promote ? nid_to_phys(1, &addr) : nid_to_phys(0, &addr);
                if (ret)
                        goto free_out;
+
        } else {
                addr.start = promote ? node1_start_addr : node0_start_addr;
                addr.end = promote ? node1_end_addr : node0_end_addr;
```

> 
> >  samples/damon/mtier.c | 42 +++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 39 insertions(+), 3 deletions(-)
> > 
> > diff --git a/samples/damon/mtier.c b/samples/damon/mtier.c
> > index f3220d6e6739..3570ebe10fab 100644
> > --- a/samples/damon/mtier.c
> > +++ b/samples/damon/mtier.c
> > @@ -42,8 +42,34 @@ 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 detect_node_addresses __read_mostly;
> > +module_param(detect_node_addresses, bool, 0600);
> > +
> >  static struct damon_ctx *ctxs[2];
> >  
> > +struct region_range {
> > +	phys_addr_t start;
> > +	phys_addr_t end;
> > +};
> > +
> > +static int nid_to_phys(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?  */
> 
> I understand you are saying we might need to remove address ranges in the node
> that DAMON will anyway unable to check accesses, e.g., reserved memory.  Since
> those are uusally only a small portion and this is a sample code, I think we
> don't really need to do that, so I think you can drop this TODO comment from
> your next version, if you are gonna make it.
> 

Ok, I will drop that comment from next version.

Best Regards,
Yunjeong Mun

...snip...

> 
> [1] https://docs.kernel.org/process/submitting-patches.html#commentary
> 
> 
> Thanks,
> SJ

Re: [RFC PATCH v2] samples/damon: support automatic node address detection

[SeongJae Park]

On Fri,  4 Jul 2025 16:05:59 +0900 Yunjeong Mun <yunjeong.mun@sk.com> wrote:

> Hello Seongjae,
> 
> On Thu,  3 Jul 2025 09:52:37 -0700 SeongJae Park <sj@kernel.org> wrote:
> > Hello Yunjeong,
> > 
> > On Thu,  3 Jul 2025 16:44:22 +0900 Yunjeong Mun <yunjeong.mun@sk.com> wrote:
> > 
> > > This patch adds a new knob `detect_node_addresses`, which determines
> > > whether the physical address range is set manually using the existing
> > > knobs or automatically by the mtier module. When `detect_node_addresses`
> > > set to 'Y', mtier automatically converts node0 and node1 to their
> > > physical addresses. If set to 'N', it uses the existing
> > > 'node#_start_addr' and 'node#_end_addr' to define regions as before.
> > 
> > Thank you for this patch!
> > 
> > > 
> > > Suggested-by: Honggyu Kim <honggyu.kim@sk.com>
> > > Signed-off-by: Yunjeong Mun <yunjeong.mun@sk.com>
> > 
> > Reviewed-by: SeongJae Park <sj@kernel.org>
> > 
> > > ---
> > 
> > From next time, please consider adding a summary of what changes have made from
> > the previous version here, like suggested[1] on the documentation.
> 
> Ok, I'll add it next time, thanks:)
> One concern I have about this patch is the requirement to set
> 'detect_node_addresses=Y' before setting 'enable=Y'. Not following
> this order causes an error, which makes it difficult for users to use
> the module.

That's same to existing address parameters, and similar to existing DAMON user
interfaces.  Parameters are applied when starting DAMON.  Parameters can be
updated while DAMON is running, but it requires explicit "commit" action for
updating those at once.

DAMON sample modules don't support the runtime commit feature, though.  I think
it is not a bad tradeoff for simplicity of the code, given the purpose of
sample modules.

> So, how about removing 'detect_node_address'? Instead, we
> could convert node0,1 to physical address automatically by default, and use
> existing 'node#_*_addr' values only when those files are explicitly set.

This would make old usage broken.  Since this is a sample module, I think that
could be justified if there is a very good reason.  But I don't think we have a
very good reason here.  So I suggest not to do that.


Thanks,
SJ

[...]