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?

You are correct. The original DAMON implementation still increments
s->stat.nr_snapshots even if damos_valid_target() returns false.

I will fix this missing behavior in the next revision.

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

I am somewhat uncertain about the exact semantics of nr_snapshots.

The documentation defines it as:

    "Total number of DAMON snapshots that the scheme is tried to be
    applied."

But during damon_do_apply_schemes(), nr_snapshots does not increment
unless the scheme meets the aforementioned guard conditions, regardless
of how many times it has been attempted before.

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

Yes. Same as the previous answers regarding nr_snapshots.

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

Yes. Same as the previous answers regarding 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.

Good catch! This is indeed a bug. I will fix this in the next revision
to ensure that the scheme evaluation order strictly same as the original
one.

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

Best regards,
Rui Yan