[PATCH] samples/damon/mtier: use damon_addr_range consistently

[PATCH] samples/damon/mtier: use damon_addr_range consistently

[Enze Li]

The DAMON provides struct damon_addr_range for unified address range
management.  However, the mtier sample still defines a local struct
region_range with an identical layout (start and end as phys_addr_t) and
mixes its usage with struct damon_addr_range in
damon_sample_mtier_build_ctx().

This patch removes the local struct region_range and changes the
parameter type of nid_to_phys() to struct damon_addr_range.

Signed-off-by: Enze Li <lienze@kylinos.cn>
---
 samples/damon/mtier.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/samples/damon/mtier.c b/samples/damon/mtier.c
index ac9c24b92ead..67f20b52dc7b 100644
--- a/samples/damon/mtier.c
+++ b/samples/damon/mtier.c
@@ -52,12 +52,7 @@ 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)
+static int nid_to_phys(int target_node, struct damon_addr_range *range)
 {
 	if (!node_online(target_node)) {
 		pr_err("NUMA node %d is not online\n", target_node);
@@ -78,8 +73,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;
-	struct damon_addr_range range;
+	struct damon_addr_range range, addr;
 	int ret;
 
 	ctx = damon_new_ctx();

base-commit: c872b70f5d6c742ad34b8e838c92af81c8920b3e
-- 
2.47.3

Re: [PATCH] samples/damon/mtier: use damon_addr_range consistently

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The commit message incorrectly claims `struct damon_addr_range` has an identical layout to `struct region_range`, masking a physical address truncation issue on 32-bit PAE architectures.
- [Low] The refactoring removes a custom struct but leaves a redundant local variable `range` and unnecessary field-by-field copy operations.
--

--- Patch [1]: [PATCH] samples/damon/mtier: use damon_addr_range consistently ---
commit 71c06f0196fa4997fb5e79c1b3b9f9f2e74eb370
Author: Enze Li <lienze@kylinos.cn>

samples/damon/mtier: use damon_addr_range consistently

This commit removes the local struct region_range and uses struct
damon_addr_range for unified address range management.

> The DAMON provides struct damon_addr_range for unified address range
> management.  However, the mtier sample still defines a local struct
> region_range with an identical layout (start and end as phys_addr_t) and
> mixes its usage with struct damon_addr_range in

[Severity: Medium]
Is this description of struct damon_addr_range accurate?

Looking at include/linux/damon.h, struct damon_addr_range is defined with
unsigned long fields, not phys_addr_t:

struct damon_addr_range {
	unsigned long start;
	unsigned long end;
};

On 32-bit architectures with PAE, phys_addr_t is 64-bit while unsigned long
is 32-bit, meaning assignments of physical addresses via PFN_PHYS() will
silently truncate.

Could the commit message be updated to avoid stating they have identical
layouts?

> diff --git a/samples/damon/mtier.c b/samples/damon/mtier.c
> index ac9c24b92ead8..67f20b52dc7bb 100644
> --- a/samples/damon/mtier.c
> +++ b/samples/damon/mtier.c
[ ... ]
> @@ -78,8 +73,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;
> -	struct damon_addr_range range;
> +	struct damon_addr_range range, addr;

[Severity: Low]
Since addr and range are now exactly the same type, is it still necessary to
keep the separate range variable?

Further down in damon_sample_mtier_build_ctx(), there is a field-by-field
copy that is now redundant:

	range.start = addr.start;
	range.end = addr.end;

	ret = damon_set_regions(target, &range, 1, DAMON_MIN_REGION_SZ);

Could we remove the range variable entirely and just pass &addr directly to
damon_set_regions() instead?

>  	int ret;
>  
>  	ctx = damon_new_ctx();

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720092355.237599-1-lienze@kylinos.cn?part=1

Re: [PATCH] samples/damon/mtier: use damon_addr_range consistently

[Enze Li]

Hi Sashiko AI,

On 2026/7/20 17:35, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] The commit message incorrectly claims `struct damon_addr_range` has an identical layout to `struct region_range`, masking a physical address truncation issue on 32-bit PAE architectures.
> - [Low] The refactoring removes a custom struct but leaves a redundant local variable `range` and unnecessary field-by-field copy operations.
> --
> 
> --- Patch [1]: [PATCH] samples/damon/mtier: use damon_addr_range consistently ---
> commit 71c06f0196fa4997fb5e79c1b3b9f9f2e74eb370
> Author: Enze Li <lienze@kylinos.cn>
> 
> samples/damon/mtier: use damon_addr_range consistently
> 
> This commit removes the local struct region_range and uses struct
> damon_addr_range for unified address range management.
> 
>> The DAMON provides struct damon_addr_range for unified address range
>> management.  However, the mtier sample still defines a local struct
>> region_range with an identical layout (start and end as phys_addr_t) and
>> mixes its usage with struct damon_addr_range in
> 
> [Severity: Medium]
> Is this description of struct damon_addr_range accurate?
> 
> Looking at include/linux/damon.h, struct damon_addr_range is defined with
> unsigned long fields, not phys_addr_t:
> 
> struct damon_addr_range {
> 	unsigned long start;
> 	unsigned long end;
> };
> 
> On 32-bit architectures with PAE, phys_addr_t is 64-bit while unsigned long
> is 32-bit, meaning assignments of physical addresses via PFN_PHYS() will
> silently truncate.
> 
> Could the commit message be updated to avoid stating they have identical
> layouts?

Thanks for catching this.  The commit message is inaccurate when
describing the two structs as having "identical layout".

I'll update the commit message in v2 to remove the "identical layout"
claim and instead describe the change as replacing the local struct
with struct damon_addr_range for consistency with the DAMON API.

> 
>> diff --git a/samples/damon/mtier.c b/samples/damon/mtier.c
>> index ac9c24b92ead8..67f20b52dc7bb 100644
>> --- a/samples/damon/mtier.c
>> +++ b/samples/damon/mtier.c
> [ ... ]
>> @@ -78,8 +73,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;
>> -	struct damon_addr_range range;
>> +	struct damon_addr_range range, addr;
> 
> [Severity: Low]
> Since addr and range are now exactly the same type, is it still necessary to
> keep the separate range variable?
> 
> Further down in damon_sample_mtier_build_ctx(), there is a field-by-field
> copy that is now redundant:
> 
> 	range.start = addr.start;
> 	range.end = addr.end;
> 
> 	ret = damon_set_regions(target, &range, 1, DAMON_MIN_REGION_SZ);
> 
> Could we remove the range variable entirely and just pass &addr directly to
> damon_set_regions() instead?

Nice catch!  Now that both variables are struct damon_addr_range, the
separate range variable and the field-by-field copy are redundant.
damon_set_regions() only reads from the ranges array and doesn't modify
it, so passing &addr directly is safe.  I'll fold this into v2.

Thanks,
Enze

<...>