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

[SeongJae Park <sj@kernel.org>]

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