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

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

[Enze Li]

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);

base-commit: 9d7a0577c9db35c4cc52db90bc415ea248446472
-- 
2.43.0

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

[SeongJae Park]

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

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

[Enze Li]

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

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

[SeongJae Park]

On Tue, 22 Apr 2025 14:09:09 +0800 Enze Li <lienze@kylinos.cn> wrote:

> 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,

Thank you for kindly clarifying the context.

> so please don't hesitate to drop it if you
> consider it superfluous.

Yes, I don't think we need this exact change, based on the details that you
kindly shared.  Thank you again for the sharing and generously understanding
this decision.

Nevertheless, I feel like maybe the documentations and comments could be
improved to reduce this kind of confusion.  If you find the room to improve,
please feel free to submit a patch or suggest changes.

FYI, please consider trimmed interleaved replies in future:
https://docs.kernel.org/process/submitting-patches.html#use-trimmed-interleaved-replies-in-email-discussions


Thanks,
SJ

> 
> 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