Re: [PATCH v2 1/4] mm/damon: introduce DAMON_STAT module

[Joshua Hahn <joshua.hahnjy@gmail.com>]

On Thu,  5 Jun 2025 09:11:29 -0700 SeongJae Park <sj@kernel.org> wrote:

> On Thu,  5 Jun 2025 08:25:07 -0700 Joshua Hahn <joshua.hahnjy@gmail.com> wrote:
> 
> > On Wed,  4 Jun 2025 11:31:24 -0700 SeongJae Park <sj@kernel.org> wrote:
> [...]
> > Hi SJ, thank you for this patch! I have been looking forward to it : -)
> > I had a few questions about the init function:
> 
> Hi Joshua, I'm more than happy to get your questions :)
> 
> > 
> > [...snip...]
> > 
> > > +static int damon_stat_start(void)
> > > +{
> > > +	damon_stat_context = damon_stat_build_ctx();
> > > +	if (!damon_stat_context)
> > > +		return -ENOMEM;
> > > +	return damon_start(&damon_stat_context, 1, true);
> > > +}
> > > +
> > > +static void damon_stat_stop(void)
> > > +{
> > > +	damon_stop(&damon_stat_context, 1);
> > > +	damon_destroy_ctx(damon_stat_context);
> > > +}
> > > +
> > > +static bool damon_stat_init_called;
> > > +
> > > +static int damon_stat_enabled_store(
> > > +		const char *val, const struct kernel_param *kp)
> > > +{
> > > +	bool is_enabled = enabled;
> > > +	int err;
> > > +
> > > +	err = kstrtobool(val, &enabled);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	if (is_enabled == enabled)
> > > +		return 0;
> > > +
> > > +	if (!damon_stat_init_called)
> > > +		/*
> > > +		 * probably called from command line parsing (parse_args()).
> > > +		 * Cannot call damon_new_ctx().  Let damon_stat_init() handle.
> > > +		 */
> > > +		return 0;
> > 
> > I was hoping you could educate me about how damon_stat_init_called works here.
> > I think my confusion comes from my lack of knowledge about kernel modules : -)
> > In the cover letter, you wrote that DAMON_STAT is a static kernel module.
> > My understanding was that this would mean damon_stat_init would always be
> > called,
> 
> To my understanding, the function is called back only when the parameteer value
> is being changed.  Such changes could be made in runtime via parameter files,
> and in boot time via the kernel command line.  If there is not kernel command
> line for setting the parameter, this callback function is not called.
> 
> > so I was wondering under what condition it would not be initialized.
> 
> The enabled parameter value is initialized at build time, based on
> CONFIG_DAMON_STAT_ENABLED_DEFAULT.  So, the parameter value will always be
> initialized.
> 
> > I see the comment you wrote above, but was still a little bit confused.
> 
> The kernel command line parameters parsing is called in pretty early stage of
> the bootup, before slab is ready.  Hence, if enabled parmeter is set by the
> kernel command line, damon_stat_enabled_store() is called in the early stage,
> and fails from damon_stat_start(), since it needs slab, to initialize DAMON
> contexts.  For more details of such failure, you could refer to a previous
> issue report[1].
> 
> Meanwhile, damon_stat_init() and module init functions are called later, when
> slab is ready.  We therefore check the case, and defer real handling of enabled
> to damon_stat_init() in the case.
> 
> Thank you for this question, I find the comment has rooms to improve.  I'll try
> to make this better documented or easier to read.

I see, I think it makes more sense to me now. Thank you for explaining this, SJ!
I think my confusion came from my lack of knowledge. Please do not feel the need
to update the comment on this section, if you feel it is already enough : -)

> > Also, should we perhaps call damon_stat_init() if !damon_stat_init_called?
> > That way, the first caller would just eat up the time it takes to run
> > damon_stat_start().
> 
> damon_stat_init() is a module init function, and hence it will be called in
> boot time, regardless of enabled parameter setup on kernel command line.  In
> other words, it will be always invoked once, with !damon_stat_init_called.  And
> it will call damon_stat_start(), unless enabled is unset via
> CONFIG_DAMON_STAT_ENABLED_DEFAULT or kernel command line.
> 
> So, the current implementation is working as you suggested, to my
> understanding.  Please let me know if I'm missing something.
> > 
> > One other thought I have is that if this config checks for whether
> > damon_stat_init was called, this can be moved to the beginning of the function
> > before the other checks are run, but that is just my thought : -) Feel free
> > to keep the input check first, since having this at the beginning of the
> > function would mean incorrect inputs would be silently ignored.
> 
> In the kernel command line based parameter setup scenario, later
> damon_stat_init() call should see the updated 'enabled' variable value.  Hence,
> the user input value check should be done here, regardless of if this is called
> before or after damon_stat_init().
> 
> So I find no needs to change the code for now.  Nonetheless, I believe this
> code has many rooms to improve, and I'm always getting more than glad to get
> this kind of improvement ideas.  Thank you, Joshua.  Please feel free to let me
> know if you get another idea later.
> 
> I hope I answered your questions, but please let me know if I'm missing
> something!

Thanks SJ, it all makes a lot more sense now. Thank you for taking the time
to explain things!

> > 
> > Thank you SJ! I hope you have a great day!
> 
> You too.  Friday is coming!
> 
> [1] https://lore.kernel.org/linux-mm/20220604192222.1488-1-sj@kernel.org/
> 
> 
> Thanks,
> SJ
> 
> [...]

Sent using hkml (https://github.com/sjp38/hackermail)