Re: [PATCH] mm/damon/core: Initialize sampling_addr in damon_new_region()

[Enze Li <lienze@kylinos.cn>]

Hi SeongJae,

Thanks for your review!  When examining this function, I instinctively
verified the initialization of all member variables.  Personally, I'm
quite partial to the practice of initializing all fields when creating
an object - though I fully acknowledge that initializing sampling_addr
may currently have no practical consequences.  This patch serves purely
as a preventive measure, so please don't hesitate to drop it if you
consider it superfluous.

Best Regards,
Enze

On Mon, Apr 21 2025 at 09:55:25 AM -0700, SeongJae Park wrote:
> On Mon, 21 Apr 2025 11:39:19 +0800 Enze Li <lienze@kylinos.cn> wrote:
>
>> Since sampling_addr is used across vaddr and paddr modules, initialize
>> it in damon_new_region().
>> 
>> Signed-off-by: Enze Li <lienze@kylinos.cn>
>> ---
>>  mm/damon/core.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/mm/damon/core.c b/mm/damon/core.c
>> index f0c1676f0599..d197a5e3901c 100644
>> --- a/mm/damon/core.c
>> +++ b/mm/damon/core.c
>> @@ -128,6 +128,7 @@ struct damon_region *damon_new_region(unsigned long start, unsigned long end)
>>  
>>  	region->ar.start = start;
>>  	region->ar.end = end;
>> +	region->sampling_addr = 0;
>>  	region->nr_accesses = 0;
>>  	region->nr_accesses_bp = 0;
>>  	INIT_LIST_HEAD(&region->list);
>
> I unfortunately cannot find why this is required.  Is there a use case that
> reads uninitialized sampling_addr or any problem that comes from the fact that
> damon_new_regions() is not initializing the field?
>
> Only operations set implementations write and read damon_region->sampling_addr,
> so I was thinking not initializing the field on the core layer is no problem.
> I will be happy to be corrected if I'm missing something.
>
>
> Thanks,
> SJ
>
>> 
>> base-commit: 9d7a0577c9db35c4cc52db90bc415ea248446472
>> -- 
>> 2.43.0