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

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 <[email protected]>

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.

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?

> +		}
> +	}
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8