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

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 <[email protected]>

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/[email protected]?part=1