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

[SJ Park <sj@kernel.org>]

On Tue, 28 Jul 2026 04:24:56 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] damon_pa_prep_probes() executes prep actions redundantly and ignores probe filters, causing excessive rmap walks and global LRU degradation.
> --
> 
> --- Patch [8]: [RFC PATCH v2.2 08/17] mm/damon/paddr: support damon_prep ---
> commit 5d5b08fab7578294743c8bbbba6c3bfa16036115
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/paddr: support damon_prep
> 
> This commit implements the damon_operations->prep_probes() callback to
> support the DAMON_PREP_SET_PGIDLE action. It unsets page table accessed
> bits and sets the PG_Idle flag, similar to what is done for access check
> preparation.
> 
> > 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
> 
> [ ... ]
> 
> > @@ -105,6 +105,40 @@ static unsigned int damon_pa_check_accesses(struct damon_ctx *ctx)
> >  	return max_nr_accesses;
> >  }
> >  
> > +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;
> > +		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);
> > +			damon_for_each_probe(p, ctx)
> > +				damon_pa_prep_probes_region(r, p, ctx);
> 
> [Severity: High]
> Does this code redundantly execute damon_pa_mkold() and ignore probe filters?
> 
> In kdamond_fn(), if standard access checks are enabled, both
> prepare_access_checks() and prep_probes() can execute on the same tick:
> 
>     if (!access_check_disabled && ctx->ops.prepare_access_checks)
>         ctx->ops.prepare_access_checks(ctx);
>     if (do_prep)
>         ctx->ops.prep_probes(ctx, access_check_disabled);
> 
> Because prepare_access_checks() also calls damon_pa_mkold(), the subsequent
> iteration over all probes here unconditionally calls it again on the exact same
> sampling address for every probe configured with DAMON_PREP_SET_PGIDLE. Since
> damon_pa_mkold() performs a costly rmap_walk, this multiplies the overhead.

That's true, but no problem.  Sane users may not use the classic monitoring
with set_pgidle prep.

> 
> Additionally, this loop does not evaluate probe filters (such as MEMCG or
> ANON) before applying prep actions. Could this blindly clear the accessed bit
> for all sampled pages globally, artificially aging pages that the user
> explicitly excluded and degrading system-wide memory reclaim decisions?

Yes, and that's the intended behavior.  If user asks to set pgidle, DAMON sets
pgidle.

> 
> > +		}
> > +	}
> > +}
> > +
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260728041242.187380-1-sj@kernel.org?part=8


Thanks,
SJ