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

On Sat, 25 Jul 2026 21:15:30 +0000 [email protected] wrote:

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

Yes.  But wise users wouldn't set preps in such ways.  No issue.

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

I told I will fix patch 7.

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


Thanks,
SJ