Re: [RFC PATCH 08/17] mm/damon/paddr: support damon_prep

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Double randomization of `r->sampling_addr` silently overwrites the prepared address when access checks are enabled, leading to an orphaned prepared page and evaluation of an unprepared page.
- [Medium] Redundant application of `DAMON_PREP_SET_PGIDLE` causes duplicate expensive rmap walks for the same folio when multiple probes exist.
--

--- Patch [8]: [RFC PATCH 08/17] mm/damon/paddr: support damon_prep ---
commit 4bd5ea4d30cf668863712fb6c9fd487551e2b59c
Author: SJ Park <[email protected]>

mm/damon/paddr: support damon_prep

This commit implements the prep_probes() callback for physical address 
operations. It supports the DAMON_PREP_SET_PGIDLE action by unsetting page 
table accessed bits and setting the PG_Idle flag.

> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> index 199de1463fa06..8d8fcd1369e56 100644
> --- a/mm/damon/paddr.c
> +++ b/mm/damon/paddr.c

[ ... ]

> +static void damon_pa_prep_probes_region(struct damon_region *r,
> +		struct damon_probe *probe, struct damon_ctx *ctx)
> +{
> +	struct damon_prep *p;
> +
> +	damon_for_each_prep(p, probe) {
> +		switch (p->action) {
> +		case DAMON_PREP_SET_PGIDLE:
> +			damon_pa_mkold(damon_pa_phys_addr(r->sampling_addr,
> +						ctx->addr_unit));
> +			break;

[Severity: Medium]
Will this result in redundant expensive rmap walks if multiple probes share
the same prep action?

When iterating over all probes in damon_pa_prep_probes(), this is called for
each probe. If a user configures multiple probes with the pgidle_unset
filter in a single context, it looks like damon_pa_mkold() will be invoked
multiple times for the same sampling_addr, causing duplicate folio rmap
traversals.

> +		default:
> +			break;
> +		}
> +	}
> +}
> +
> +static void damon_pa_prep_probes(struct damon_ctx *ctx, bool set_samples)
> +{
> +	struct damon_target *t;
> +	struct damon_region *r;
> +	struct damon_probe *p;
> +
> +	damon_for_each_target(t, ctx) {
> +		damon_for_each_region(r, t) {
> +			if (set_samples)
> +				r->sampling_addr = damon_rand(ctx, r->ar.start,
> +						r->ar.end);

[Severity: High]
Does this conditionally overwrite the sampling address that was already set
and prepared?

In mm/damon/core.c:kdamond_fn(), prepare_access_checks() and prep_probes()
can both be called in sequence:

    if (!access_check_disabled && ctx->ops.prepare_access_checks)
        ctx->ops.prepare_access_checks(ctx);
    if (ctx->ops.prep_probes)
        ctx->ops.prep_probes(ctx, has_prep);

If a user configures a probe with weight=0 and a prep action,
access_check_disabled is false and has_prep is true. Since has_prep is
passed as set_samples, damon_pa_prep_probes() will overwrite the
r->sampling_addr that was just prepared by prepare_access_checks(). This
would leave the originally prepared page orphaned and cause an unprepared
page to be evaluated.

> +			damon_for_each_probe(p, ctx)
> +				damon_pa_prep_probes_region(r, p, ctx);
> +		}
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8