Re: [RFC PATCH v3 08/28] mm/damon/paddr: support data attributes monitoring

[SeongJae Park <sj@kernel.org>]

On Sat, 16 May 2026 19:05:31 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Brittle filter evaluation logic in `damon_pa_filter_pass()` causes unpredictable default fallback behavior.
> - [High] Silent integer overflow of `r->probe_hits[i]` due to an 8-bit limit.
> - [High] Costly page lookups and folio refcount operations are unconditionally performed even when no probes are configured.
> --
> 
> commit 3ac6d88b44843f7304cd50448ef416c14428371c
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/paddr: support data attributes monitoring
> 
> This commit implements the apply_probes() callback for physical address
> space monitoring in DAMON. It introduces filter evaluation and applies
> probe hits to region statistics.
> 
> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> --- a/mm/damon/paddr.c
> +++ b/mm/damon/paddr.c
> 
> [ ... ]
> 
> > +static bool damon_pa_filter_pass(phys_addr_t pa, struct folio *folio,
> > +		struct damon_probe *p)
> > +{
> > +	struct damon_filter *f;
> > +	bool pass = true;
> > +
> > +	damon_for_each_filter(f, p) {
> > +		if (damon_pa_filter_match(f, folio)) {
> > +			pass = f->allow;
> > +			break;
> > +		}
> > +		pass = !f->allow;
> > +	}
> > +	return pass;
> > +}
> 
> If a folio does not match any of the filters, the loop continues and pass
> is repeatedly updated to the inverted allow flag of each evaluated filter.
> 
> When the loop finishes, the final returned result is purely the inverted
> allow flag of the last filter in the list.
> 
> Is this arbitrary fallback logic intentional?

Yes, intentional [1].

> 
> [ ... ]
> 
> > +static void damon_pa_apply_probes(struct damon_ctx *ctx)
> > +{
> > +	struct damon_target *t;
> > +	struct damon_region *r;
> > +	struct damon_probe *p;
> > +
> > +	damon_for_each_target(t, ctx) {
> > +		damon_for_each_region(r, t) {
> > +			int i = 0;
> > +			phys_addr_t pa;
> > +			struct folio *folio;
> > +
> > +			pa = damon_pa_phys_addr(r->sampling_addr,
> > +					ctx->addr_unit);
> > +			folio = damon_get_folio(PHYS_PFN(pa));
> 
> Because damon_pa_apply_probes() executes unconditionally on every sampling
> interval, damon_get_folio() and folio_put() will be called globally across
> all regions even if the user has not configured any probes.
> 
> Could we add an early return or check if ctx->probes is empty before
> performing these expensive atomic refcount operations on the hot path?

The user can set the maximum number of regions, and this is arguably not a hot
path.  So no blocker.

> 
> > +			damon_for_each_probe(p, ctx) {
> > +				if (damon_pa_filter_pass(pa, folio, p))
> > +					r->probe_hits[i]++;
> 
> The r->probe_hits[i] counter is declared as an unsigned char in
> struct damon_region, which has a maximum value of 255.
> 
> Since the number of samples per DAMON aggregation interval is configurable
> and can easily exceed 255 in standard use cases, won't this counter
> silently wrap around to 0 and corrupt the monitoring statistics?

No blocker [2].

> 
> > +				i++;
> > +			}
> > +			if (folio)
> > +				folio_put(folio);
> > +		}
> > +	}
> > +}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260516183712.81393-1-sj@kernel.org?part=8

[1] https://lore.kernel.org/20260516005632.144711-1-sj@kernel.org
[2] https://lore.kernel.org/20260516003559.141476-1-sj@kernel.org


Thanks,
SJ