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

[Yunjeong Mun <yunjeong.mun@sk.com>]

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