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

[SeongJae Park <sj@kernel.org>]

Hello Yunjeong,


On Mon,  7 Jul 2025 11:31:34 +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.
> 
> Suggested-by: Honggyu Kim <honggyu.kim@sk.com>
> Signed-off-by: Yunjeong Mun <yunjeong.mun@sk.com>
> ---
> V2 -> V3: Removed an unusable comment
> V1 -> V2: Removed second patch, added a new knob, and cleand up code

Thank you for revisioning.

Let's clearly mention the v1 and v2 were RFC versions.

Also, from the next time, it would be greater if you could add links to the
previous versions.

For other reviewers,
RFC v2: https://lore.kernel.org/20250703074423.1771-1-yunjeong.mun@sk.com
RFC v1: https://lore.kernel.org/20250701085417.1734-1-yunjeong.mun@sk.com

> ---
>  samples/damon/mtier.c | 41 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/samples/damon/mtier.c b/samples/damon/mtier.c
> index f3220d6e6739..3a6ae78efafe 100644
> --- a/samples/damon/mtier.c
> +++ b/samples/damon/mtier.c
> @@ -42,8 +42,33 @@ 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)
> +{
> +

Let's drop this unnecessary blank line.

> +	if (!node_online(target_node)) {
> +		pr_err("NUMA node %d is not online\n", target_node);
> +		return -EINVAL;
> +	}
> +
> +	unsigned long start_pfn = node_start_pfn(target_node);
> +	unsigned long end_pfn   = node_end_pfn(target_node);

Let's put declarations before executable statements, for the coding style
consistency.

> +
> +	range->start = PFN_PHYS(start_pfn);
> +	range->end  = PFN_PHYS(end_pfn);

Or, you could just skip the declarations and do something like below?

range->start = PFN_PHYS(node_start_pfn(target_node));

> +
> +	return 0;
> +}
> +
>  static struct damon_ctx *damon_sample_mtier_build_ctx(bool promote)
>  {
>  	struct damon_ctx *ctx;
> @@ -53,6 +78,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 +109,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


Thanks,
SJ