Re: [RFC PATCH] mm/damon: introduce damos_sort_type for re-ordering regions list

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Broken `stat.nr_snapshots` tracking in `damos_apply_sorted_scheme()` bypasses `max_nr_snapshots` limit enforcement.
- [High] Sorted schemes are evaluated out of order, violating DAMON's strict scheme priority semantics.
- [Medium] Silent disabling of schemes when `get_scheme_score` is not supported by the underlying operation set.
--

--- Patch [1]: [RFC PATCH] mm/damon: introduce damos_sort_type for re-ordering regions list ---
commit 2f127c3d13c04b9a2b95946211fee29deac96c0d
Author: Liew Rui Yan <[email protected]>
Subject: mm/damon: introduce damos_sort_type for re-ordering regions list

This patch adds a per-scheme sysfs attribute schemes/<N>/sort_type to allow a
scheme to apply actions to target regions in descending order of their
ops.get_scheme_score() values.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 644daf5a16560..b0c52c9eabe82 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c

[ ... ]

> @@ -2673,6 +2684,107 @@ static void damon_do_apply_schemes(struct damon_ctx *c,
>  	}
>  }
>  
> +struct damos_sort_priv {
> +	struct damon_ctx *c;
> +	struct damos *s;
> +};
> +
> +static int damos_sort_score_desc_cmp(const void *a, const void *b,
> +		const void *priv)
> +{

[ ... ]

> +}
> +
> +static void damos_apply_sorted_scheme(struct damon_ctx *c,
> +		struct damon_target *t, struct damos *s)
> +{
> +	struct damon_region **arr;
> +	struct damon_region *r;
> +	struct damos_quota *quota = &s->quota;
> +	struct damos_sort_priv priv = { .c = c, .s = s };
> +	unsigned long nr = 0, i = 0;
> +
> +	if (!c->ops.get_scheme_score)
> +		return;

[Severity: Medium]
When the operations set does not implement get_scheme_score, does this early
return silently disable the scheme and prevent stat.nr_snapshots from ever
incrementing?

The unsorted path gracefully handles a missing score callback by falling back
to basic filtering without the score.

> +	/* Avoid unnecessary kvmalloc_array() */
> +	if (damos_quota_is_full(quota, c->min_region_sz))
> +		return;
> +
> +	damon_for_each_region(r, t) {
> +		if (__damos_valid_target(r, s, c))
> +			nr++;
> +	}
> +	if (nr == 0)
> +		return;

[Severity: High]
If there are no valid regions (nr == 0), this returns early without
incrementing s->stat.nr_snapshots. Does this prevent the scheme from ever
reaching max_nr_snapshots and automatically deactivating?

> +	if (nr == 1)
> +		goto single_valid_region;
> +
> +	arr = kvmalloc_array(nr, sizeof(*arr), GFP_KERNEL);
> +
> +	if (!arr)
> +		return;
> +
> +	damon_for_each_region(r, t) {
> +		if (__damos_valid_target(r, s, c))
> +			arr[i++] = r;
> +	}
> +
> +	sort_r_nonatomic(arr, nr, sizeof(*arr), damos_sort_score_desc_cmp, NULL, &priv);
> +
> +	for (i = 0; i < nr; i++) {
> +		r = arr[i];
> +
> +		/* Check the quota */
> +		if (damos_quota_is_full(quota, c->min_region_sz))
> +			break;
> +
> +		if (s->max_nr_snapshots &&
> +				s->max_nr_snapshots <= s->stat.nr_snapshots)
> +			continue;
> +
> +		if (damos_valid_target(c, r, s)) {
> +			damos_apply_scheme(c, t, r, s);
> +		} else {
> +			/*
> +			 * There is no need to continue because the score is
> +			 * already lower than quota.min_score.
> +			 */
> +			break;

[Severity: High]
If a region fails damos_valid_target() due to low quota scores, this breaks
out of the loop and skips the (i == nr - 1) check below. Does this omit
tracking the snapshot and prevent the scheme from deactivating?

> +		}
> +
> +		if (i == nr - 1)
> +			s->stat.nr_snapshots++;
> +	}
> +
> +	kvfree(arr);
> +	return;
> +
> +single_valid_region:
> +	damon_for_each_region(r, t) {
> +		if (__damos_valid_target(r, s, c))
> +			break;
> +	}
> +
> +	/* Check the quota */
> +	if (damos_quota_is_full(quota, c->min_region_sz))
> +		return;

[Severity: High]
If the quota is full in this single_valid_region fast-path, it returns early.
Does this also bypass incrementing s->stat.nr_snapshots?

> +
> +	if (s->max_nr_snapshots &&
> +			s->max_nr_snapshots <= s->stat.nr_snapshots)
> +		return;
> +
> +	if (damos_valid_target(c, r, s))
> +		damos_apply_scheme(c, t, r, s);
> +
> +	s->stat.nr_snapshots++;
> +}
> +
>  /*
>   * damos_apply_target() - Apply DAMOS schemes to a given target.
>   * @c:			monitoring context to apply its DAMOS schemes to..
> @@ -2695,6 +2807,17 @@ static void damos_apply_target(struct damon_ctx *c, struct damon_target *t,
>  		unsigned long max_region_sz)
>  {
>  	struct damon_region *r;
> +	struct damos *s;
> +
> +	damon_for_each_scheme(s, c) {
> +		if (s->sort_type == DAMOS_SORT_NONE)
> +			continue;
> +		if (!s->wmarks.activated)
> +			continue;
> +		if (time_before(c->passed_sample_intervals, s->next_apply_sis))
> +			continue;
> +		damos_apply_sorted_scheme(c, t, s);
> +	}
>  
>  	damon_for_each_region(r, t) {
>  		struct damon_region *prev_r;
>  
>  		damon_do_apply_schemes(c, t, r);

[Severity: High]
By iterating and applying all sorted schemes first, and then iterating to
apply unsorted schemes via damon_do_apply_schemes(), are we executing sorted
schemes out of order?

DAMON evaluates schemes in the order they are defined. If a user defines an
unsorted scheme before a sorted scheme, this two-pass execution will cause
the sorted scheme to evaluate the regions first, breaking the execution
order priority.

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