Re: [RFC PATCH v1 3/4] mm/damon: introduce DAMON_HUGEPAGE for hot region hugepage collapsing

On 4/30/2026 6:43 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Silent failure and invalid state when target PID is not found.
> - [High] TOCTOU race condition on `aggr_interval` leading to divide-by-zero kernel panic.
> - [Medium] Memory leak of DAMON context upon module initialization failure.
> --
> 
> commit 982630eef231c8c59229da1208ed3240bf24f081
> Author: Asier Gutierrez <[email protected]>
> 
> mm/damon: introduce DAMON_HUGEPAGE for hot region hugepage collapsing
> 
> This patch introduces a new DAMON module (DAMON_HUGEPAGE)
> which collapses hot regions into huge pages.
> 
>> diff --git a/mm/damon/hugepage.c b/mm/damon/hugepage.c
>> new file mode 100644
>> index 0000000000000..02729736dc560
>> --- /dev/null
>> +++ b/mm/damon/hugepage.c
> 
> [ ... ]
> 
>> +static struct damos *damon_hugepage_new_scheme(void)
>> +{
>> +	struct damos_access_pattern pattern = {
>> +		/* Find regions having PMD_SIZE or larger size */
>> +		.min_sz_region = PMD_SIZE,
>> +		.max_sz_region = ULONG_MAX,
>> +		.min_nr_accesses = 0,
>> +		.max_nr_accesses = UINT_MAX,
>> +		.min_age_region = min_age /
>> +			damon_hugepage_mon_attrs.aggr_interval,
> 
> Is there a race condition here? Since damon_hugepage_mon_attrs is exposed
> as a module parameter, aggr_interval could be modified locklessly. If it is
> set to 0 concurrently between the check in damon_hugepage_apply_parameters()
> and this division, could it trigger a divide-by-zero kernel panic?
No, damon_hugepage_apply_parameters is called only when the module starts
or when a new change is committed by the user. The user first changes
the value and then commit the change.
> 
>> +		.max_age_region = UINT_MAX,
>> +	};
>> +
>> +	return damon_new_scheme(
>> +		&pattern,
>> +		/* synchrounous partial collapse as soon as found */
>> +		DAMOS_HUGEPAGE, 0,
>> +		/* under the quota. */
>> +		&damon_hugepage_quota,
>> +		&(struct damos_watermarks){}, NUMA_NO_NODE);
>> +}
>> +
>> +static int damon_hugepage_apply_parameters(void)
>> +{
>> +	struct damon_ctx *param_ctx;
>> +	struct damon_target *param_target;
>> +	struct damos *scheme;
>> +	struct damos_quota_goal *goal;
>> +	struct pid *spid;
>> +	int err;
>> +
>> +	err = damon_modules_new_vaddr_ctx_target(&param_ctx, &param_target);
>> +	if (err)
>> +		return err;
>> +
>> +	param_ctx->addr_unit = addr_unit;
>> +	param_ctx->min_region_sz = max(DAMON_MIN_REGION_SZ / addr_unit, 1);
>> +
>> +	spid = find_get_pid(monitored_pid);
>> +	if (!spid)
>> +		goto out;
> 
> Does this silently return success when the monitored PID is not found?
> 
> Because err is 0 from the successful context creation above, jumping
> to the out label returns 0.
> 
> This would cause DAMON to start with an empty context without informing
> the user that their configuration failed.

Good catch. I will handle this case.

>> +
>> +	param_target->pid = spid;
>> +
>> +	if (!damon_hugepage_mon_attrs.aggr_interval) {
>> +		err = -EINVAL;
>> +		goto out;
>> +	}
> 
> [ ... ]
> 
>> +static int __init damon_hugepage_init(void)
>> +{
>> +	int err;
>> +
>> +	if (!damon_initialized()) {
>> +		err = -ENOMEM;
>> +		goto out;
>> +	}
>> +	err = damon_modules_new_vaddr_ctx_target(&ctx, &target);
>> +	if (err)
>> +		goto out;
>> +
>> +	call_control.data = ctx;
>> +
>> +	/* 'enabled' has set before this function, probably via command line */
>> +	if (enabled)
>> +		err = damon_hugepage_turn(true);
>> +
>> +out:
>> +	if (err && enabled)
>> +		enabled = false;
>> +	return err;
>> +}
> 
> Does this code leak the allocated ctx and target if damon_hugepage_turn()
> fails?
> 
> Because module initialization failure prevents the module exit routine
> from running, it seems damon_destroy_ctx() should be called in the error
> path to avoid a memory leak.
> 
The code is correct. In case of initialization failure, we just mark the
module as "non running" and exit.

-- 
Asier Gutierrez
Huawei