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

[SJ Park <sj@kernel.org>]

On Tue, 01 Sep 2026 04:47:02 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] `damon_pa_prep_probes` applies probe preparation actions without checking probe filters, causing unintended page state modifications and redundant folio lookups.
> --
> 
> commit 05f1236264960a0054ba7741a1b9b7ae78666ee1
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/paddr: support damon_prep
> 
> This commit implements the prep_probes() callback for damon_operations. It
> adds support for the DAMON_PREP_SET_PGIDLE preparation action by unsetting
> page table accessed bits and setting the PG_idle flag via damon_pa_mkold().
> 
> > diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> > index 5b307dcbb2a50..8fb71183b76c0 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));
> 
> [Severity: Medium]
> Will this unconditionally mutate the page state for every prep action,
> regardless of whether the page passes the probe's filters?
> 
> Because damon_pa_prep_probes_region() doesn't check the filters, it appears
> it will clear the accessed bit and set PG_idle on pages that are explicitly
> excluded (e.g., when a probe only targets a specific memcg).
> 
> Could this disrupt legitimate LRU tracking and reclaim for processes not
> targeted by the probe?

No.  damon_pa_mkold() will set page as referenced if it was referenced before,
to not disrupt LRU tracking and reclaim, same to the classical access
monitoring and idle page tracking.

> 
> > +			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: Medium]
> Since damon_pa_prep_probes_region() is called in a nested loop for each probe,
> could this cause redundant work?
> 
> If multiple probes use DAMON_PREP_SET_PGIDLE, this will redundantly invoke
> damon_pa_mkold() multiple times on the exact same physical address, leading to
> unnecessary folio lookups and reference cycles per region.

No issue.  DAMON users would know what they are asking to do.

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


Thanks,
SJ