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

[sashiko-bot@kernel.org]

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?

[ ... ]

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

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

> +				i++;
> +			}
> +			if (folio)
> +				folio_put(folio);
> +		}
> +	}
> +}

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