linux-kernel - Re: [PATCH v2 1/4] mm/damon: introduce DAMON

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20250605161129.82107-1-sj@kernel.org>
Date: Thu,  5 Jun 2025 09:11:29 -0700
From: SeongJae Park <sj@...nel.org>
To: Joshua Hahn <joshua.hahnjy@...il.com>
Cc: SeongJae Park <sj@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	damon@...ts.linux.dev,
	kernel-team@...a.com,
	linux-kernel@...r.kernel.org,
	linux-mm@...ck.org
Subject: Re: [PATCH v2 1/4] mm/damon: introduce DAMON_STAT module

On Thu,  5 Jun 2025 08:25:07 -0700 Joshua Hahn <joshua.hahnjy@...il.com> wrote:

> On Wed,  4 Jun 2025 11:31:24 -0700 SeongJae Park <sj@...nel.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.

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

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

[...]