Re: [RFC PATCH v2.2 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] Flawed and order-dependent filter fallback logic in damon_pa_filter_pass().
- [Medium] The probe_hits counters can silently overflow and wrap around.
- [Medium] Redundant folio lookups and reference count operations for each probe.
--

commit dd2c6385be410e674ab72a5dd4ff210f2f41dd48
Author: SeongJae Park <sj@kernel.org>

mm/damon/paddr: support data attributes monitoring

This commit implements and registers the apply_probes callback to
support data attributes monitoring in the paddr operations.

> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> index 5cdcc5037cbc1..493bcca8ff104 100644
> --- a/mm/damon/paddr.c
> +++ b/mm/damon/paddr.c
[ ... ]
> @@ -120,6 +120,67 @@ static unsigned int damon_pa_check_accesses(struct damon_ctx *ctx)
>  	return max_nr_accesses;
>  }
>  
[ ... ]
> +static bool damon_pa_filter_pass(phys_addr_t pa, struct damon_probe *p)
> +{
> +	struct damon_filter *f;
> +	struct folio *folio;
> +	bool pass = true;
> +
> +	folio = damon_get_folio(PHYS_PFN(pa));
> +	damon_for_each_filter(f, p) {
> +		if (damon_pa_filter_match(f, folio)) {
> +			pass = f->allow;
> +			break;
> +		}
> +		pass = !f->allow;

If no filters match the folio, does this logic make the final fallback
decision entirely dependent on the allow boolean of the last filter in the
list?

Reordering the filters could silently invert the fallback behavior for
non-matching folios.

> +	}
> +	if (folio)
> +		folio_put(folio);
> +	return pass;
> +}
> +
> +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;
> +
> +			damon_for_each_probe(p, ctx) {
> +				phys_addr_t pa;
> +
> +				pa = damon_pa_phys_addr(r->sampling_addr,
> +						ctx->addr_unit);
> +				if (damon_pa_filter_pass(pa, p))

Does this redundantly repeat expensive folio lookups and reference count
operations for each probe?

The physical address calculation and folio lookup in damon_pa_filter_pass()
are executed inside the probe loop, meaning they will be repeated for the
exact same physical address if multiple probes exist.

Could the folio lookup be hoisted outside the probe loop to avoid unnecessary
overhead?

> +					r->probe_hits[i]++;

Could this counter silently overflow and wrap around?

r->probe_hits[i] is defined as an unsigned char in struct damon_region.
If the aggregation interval is configured to be thousands of times larger than
the sample interval, this 8-bit counter might easily overflow.

> +				i++;
> +			}
> +		}
> +	}
> +}

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