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: <20250605152508.3275113-1-joshua.hahnjy@gmail.com>
Date: Thu,  5 Jun 2025 08:25:07 -0700
From: Joshua Hahn <joshua.hahnjy@...il.com>
To: SeongJae Park <sj@...nel.org>
Cc: 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 Wed,  4 Jun 2025 11:31:24 -0700 SeongJae Park <sj@...nel.org> wrote:

> To use DAMON for monitoring access patterns of the system, users should
> manually start DAMON via DAMON sysfs ABI with a number of parameters for
> specifying the monitoring target address space, address ranges, and
> monitoring intervals.  After that, users should also wait until desired
> amount of time data is captured into DAMON's monitoring results.  It is
> bothersome and take a long time to be practical for access monitoring on
> large fleet level production environments.
> 
> For access-aware system operations use cases like proactive cold memory
> reclamation, similar problems existed.  We we solved those by
> introducing dedicated static kernel modules such as DAMON_RECLAIM.
> 
> Implement such static kernel module for access monitoring, namely
> DAMON_STAT.  It monitors the entire physical address space with
> auto-tuned monitoring intervals.  The auto-tuning is set to capture 4 %
> of observable access events in each snapshot while keeping the sampling
> intervals 5 milliseconds in minimum and 10 seconds in maximum.  From
> a few production environments, we confirmed this setup provides high
> quality monitoring results with minimum overheads.  The module therefore
> receives only one user input, whether to enable or disable it.  It can
> be set on build or boot time via build configuration or kernel boot
> command line.  It can also be overridden at runtime.
> 
> Note that this commit only implements the DAMON control part of the
> module.  Users could get the monitoring results via
> damon:damon_aggregated tracepoint, but that's of course not the
> recommended way.  Following commits will implement convenient and
> optimized ways for serving the monitoring results to users.
> 
> Signed-off-by: SeongJae Park <sj@...nel.org>
> ---

Hi SJ, thank you for this patch! I have been looking forward to it : -)
I had a few questions about the init function:

[...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, so I was wondering under what condition it would not be initialized.
I see the comment you wrote above, but was still a little bit confused.

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().

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.

Thank you SJ! I hope you have a great day!
Joshua

> +	if (enabled)
> +		return damon_stat_start();
> +	damon_stat_stop();
> +	return 0;
> +}
> +
> +static int __init damon_stat_init(void)
> +{
> +	int err = 0;
> +
> +	damon_stat_init_called = true;
> +
> +	/* probably set via command line */
> +	if (enabled)
> +		err = damon_stat_start();
> +	return err;
> +}
> +
> +module_init(damon_stat_init);
> -- 
> 2.39.5