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

Hi Sashiko AI,

On 2026/7/20 17:35, [email protected] 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 <[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?

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

<...>