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

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

[Liew Rui Yan]

Problem
=======

A DAMOS scheme filters its target regions using an access pattern, which
is constructed with the size, the access frequency (nr_accesses), and
the age of the regions. The age here means how long the current access
pattern of a region has been maintained.

For the pageout action, the age.min of the access pattern effectively
acts as the minimum amount of time that the target regions must have been
unused.

The definition of cold memory highly depends on the devices and
workloads, and thus setting a proper default age.min (e.g., min_age of
DAMON_RECLAIM) is both important and nearly impossible to make suitable
for all devices and workloads.

Solution
========

Add a per-scheme sysfs attribute, schemes/<N>/sort_type, whose default
value is 'none'. A scheme can set it to 'score_desc', which makes the
scheme to collect the target regions and apply its action in descending
order of the regions' scores, as calculated by the ops.get_scheme_score()
callback, during the application. For a pageout scheme, the callback
returns the coldness score of each region.

Instead of modifying the region list, DAMON copies the target valid
regions into a temporary array, sorts the array in descending order of
the regions' scores, and applies the action in the sorted order.

With this, the scheme gives absolute priority to the highest-scored
region. For example, a pageout scheme with 'score_desc' reclaims the
coldest region of the target first. Users can thus keep the age.min
relatively small and let the score ordering do the precise
prioritization.

Note that regions are applied in the score order, not the address order.
Therefore, the address-based quota charge resume mechanism is not
available for such schemes. Instead, the quota is spent on the
highest-scored regions of each charge window. Applying an action
resets the age of the applied regions (except for 'stat' action), so
those regions are naturally excluded from the next window if the scheme
has a non-zero age.min. Also, when a region is split for the quota, the
age of the split-out part is preserved, and thus the highest-scored
region is continuously applied until it is fully reclaimed, even when it
is larger than the remaining quota of a single window.

Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com>
---
I am currently running the corresponding benchmarks to ensure that this
does not introduce too much performance overhead, at least not on my
device.

The purpose of sending this patch is to make sure this is a right
direction.

About my device/VM
==================

CPU: AMD Ryzen 5 5600H (12 Cores)
RAM: 8GiB in VM (4GiB + 4GiB ZRAM)

I currently foresee two potential issues with thiss patch, though I
have not obtained the test results yet, so these are just guesses.

1. Excessive memory allocations and deallocations 

    The default aggr_interval is 100ms. Executing at least one
    allocation and deallocation every 100ms is very likely to cause
    unnecessary performance overhead.

    I think this issue could be resolved by having the scheme maintain
    its own buffer.

2. Performance overhead of re-ordering

    Although on my device the number of regions is not very large, and
    DAMON's default limit of 1,000 regions also helps avoid performance
    overhead, I suspect that large servers might not stick to just 1,000
    regions.

    A solution I can think of is using a Top K Min Heap, though that
    might significantly increase code complexity and reduce readability.

As a reminder, these are just my __guesses__ and do not necessarily
reflect what will happen in practice. I will send another email after
completing the benchmarks and micro-performance testing.

 include/linux/damon.h    |   6 ++
 mm/damon/core.c          | 125 ++++++++++++++++++++++++++++++++++++++-
 mm/damon/sysfs-schemes.c |  57 ++++++++++++++++++
 3 files changed, 187 insertions(+), 1 deletion(-)

diff --git a/include/linux/damon.h b/include/linux/damon.h
index 0c8b7ddef9ab..88a3f9f4a4ca 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -140,6 +140,11 @@ enum damos_action {
 	NR_DAMOS_ACTIONS,
 };
 
+enum damos_sort_type {
+	DAMOS_SORT_NONE,
+	DAMOS_SORT_SCORE_DESC,
+};
+
 /**
  * enum damos_quota_goal_metric - Represents the metric to be used as the goal
  *
@@ -565,6 +570,7 @@ struct damos {
 	};
 	struct damos_stat stat;
 	unsigned long max_nr_snapshots;
+	enum damos_sort_type sort_type;
 /* private: internal use only */
 	/*
 	 * number of sample intervals that should be passed before applying
diff --git a/mm/damon/core.c b/mm/damon/core.c
index 644daf5a1656..b0c52c9eabe8 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -15,6 +15,7 @@
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/string_choices.h>
+#include <linux/sort.h>
 
 /* for damon_get_folio() used by node eligible memory metrics */
 #include "ops-common.h"
@@ -705,6 +706,7 @@ struct damos *damon_new_scheme(struct damos_access_pattern *pattern,
 	INIT_LIST_HEAD(&scheme->ops_filters);
 	scheme->stat = (struct damos_stat){};
 	scheme->max_nr_snapshots = 0;
+	scheme->sort_type = DAMOS_SORT_NONE;
 	scheme->last_applied = NULL;
 	INIT_LIST_HEAD(&scheme->list);
 
@@ -1465,6 +1467,7 @@ static int damos_commit(struct damos *dst, struct damos *src)
 		return err;
 
 	dst->max_nr_snapshots = src->max_nr_snapshots;
+	dst->sort_type = src->sort_type;
 	return 0;
 }
 
@@ -2627,7 +2630,12 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
 		quota->total_charged_ns += timespec64_to_ns(&end) -
 			timespec64_to_ns(&begin);
 		damos_charge_quota(quota, sz, sz_applied);
-		if (damos_quota_is_full(quota, c->min_region_sz)) {
+		/*
+		 * Since it can no longer be guaranteed that the re-ordered
+		 * Region List is sorted by address. So, no record.
+		 */
+		if (s->sort_type == DAMOS_SORT_NONE &&
+			damos_quota_is_full(quota, c->min_region_sz)) {
 			quota->charge_target_from = t;
 			quota->charge_addr_from = r->ar.end;
 		}
@@ -2648,6 +2656,9 @@ static void damon_do_apply_schemes(struct damon_ctx *c,
 	damon_for_each_scheme(s, c) {
 		struct damos_quota *quota = &s->quota;
 
+		if (s->sort_type != DAMOS_SORT_NONE)
+			continue;
+
 		if (time_before(c->passed_sample_intervals, s->next_apply_sis))
 			continue;
 
@@ -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)
+{
+	struct damon_region *ra = *(struct damon_region **)a;
+	struct damon_region *rb = *(struct damon_region **)b;
+	const struct damos_sort_priv *p = priv;
+	int score_a = p->c->ops.get_scheme_score(p->c, ra, p->s);
+	int score_b = p->c->ops.get_scheme_score(p->c, rb, p->s);
+
+	return cmp_int(score_b, score_a);
+}
+
+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;
+	/* 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;
+	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;
+		}
+
+		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;
+
+	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;
diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
index 32f495a96b17..1fcbc39b5cd6 100644
--- a/mm/damon/sysfs-schemes.c
+++ b/mm/damon/sysfs-schemes.c
@@ -2261,6 +2261,7 @@ struct damon_sysfs_scheme {
 	struct damon_sysfs_scheme_regions *tried_regions;
 	int target_nid;
 	struct damos_sysfs_dests *dests;
+	enum damos_sort_type sort_type;
 };
 
 struct damos_sysfs_action_name {
@@ -2315,6 +2316,20 @@ static struct damos_sysfs_action_name damos_sysfs_action_names[] = {
 	},
 };
 
+static struct damos_sysfs_sort_type_name {
+	enum damos_sort_type sort_type;
+	char *name;
+} damos_sysfs_sort_type_names[] = {
+	{
+		.sort_type = DAMOS_SORT_NONE,
+		.name = "none",
+	},
+	{
+		.sort_type = DAMOS_SORT_SCORE_DESC,
+		.name = "score_desc",
+	},
+};
+
 static struct damon_sysfs_scheme *damon_sysfs_scheme_alloc(
 		enum damos_action action, unsigned long apply_interval_us)
 {
@@ -2326,6 +2341,7 @@ static struct damon_sysfs_scheme *damon_sysfs_scheme_alloc(
 	scheme->action = action;
 	scheme->apply_interval_us = apply_interval_us;
 	scheme->target_nid = NUMA_NO_NODE;
+	scheme->sort_type = DAMOS_SORT_NONE;
 	return scheme;
 }
 
@@ -2645,6 +2661,42 @@ static ssize_t target_nid_store(struct kobject *kobj,
 	return err ? err : count;
 }
 
+static ssize_t sort_type_show(struct kobject *kobj, struct kobj_attribute *attr,
+		char *buf)
+{
+	struct damon_sysfs_scheme *scheme = container_of(kobj,
+			struct damon_sysfs_scheme, kobj);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(damos_sysfs_sort_type_names); i++) {
+		struct damos_sysfs_sort_type_name *type_name;
+
+		type_name = &damos_sysfs_sort_type_names[i];
+		if (type_name->sort_type == scheme->sort_type)
+			return sysfs_emit(buf, "%s\n", type_name->name);
+	}
+	return -EINVAL;
+}
+
+static ssize_t sort_type_store(struct kobject *kobj, struct kobj_attribute *attr,
+		const char *buf, size_t count)
+{
+	struct damon_sysfs_scheme *scheme = container_of(kobj,
+			struct damon_sysfs_scheme, kobj);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(damos_sysfs_sort_type_names); i++) {
+		struct damos_sysfs_sort_type_name *type_name;
+
+		type_name = &damos_sysfs_sort_type_names[i];
+		if (sysfs_streq(buf, type_name->name)) {
+			scheme->sort_type = type_name->sort_type;
+			return count;
+		}
+	}
+	return -EINVAL;
+}
+
 static void damon_sysfs_scheme_release(struct kobject *kobj)
 {
 	kfree(container_of(kobj, struct damon_sysfs_scheme, kobj));
@@ -2659,10 +2711,14 @@ static struct kobj_attribute damon_sysfs_scheme_apply_interval_us_attr =
 static struct kobj_attribute damon_sysfs_scheme_target_nid_attr =
 		__ATTR_RW_MODE(target_nid, 0600);
 
+static struct kobj_attribute damon_sysfs_scheme_sort_type_attr =
+		__ATTR_RW_MODE(sort_type, 0600);
+
 static struct attribute *damon_sysfs_scheme_attrs[] = {
 	&damon_sysfs_scheme_action_attr.attr,
 	&damon_sysfs_scheme_apply_interval_us_attr.attr,
 	&damon_sysfs_scheme_target_nid_attr.attr,
+	&damon_sysfs_scheme_sort_type_attr.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(damon_sysfs_scheme);
@@ -3038,6 +3094,7 @@ static struct damos *damon_sysfs_mk_scheme(
 		return NULL;
 	}
 	scheme->max_nr_snapshots = sysfs_scheme->stats->max_nr_snapshots;
+	scheme->sort_type = sysfs_scheme->sort_type;
 	return scheme;
 }
 
-- 
2.55.0

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

[sashiko-bot]

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 <aethernet65535@gmail.com>
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/20260804100719.116538-1-aethernet65535@gmail.com?part=1

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

[Liew Rui Yan]

> 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 <aethernet65535@gmail.com>
> 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/20260804100719.116538-1-aethernet65535@gmail.com?part=1

Best regards,
Rui Yan

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

[SJ Park]

Hello Liew,


Thank you for sharing this patch.

On Tue,  4 Aug 2026 18:07:19 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> Problem
> =======
> 
> A DAMOS scheme filters its target regions using an access pattern, which
> is constructed with the size, the access frequency (nr_accesses), and
> the age of the regions. The age here means how long the current access
> pattern of a region has been maintained.
> 
> For the pageout action, the age.min of the access pattern effectively
> acts as the minimum amount of time that the target regions must have been
> unused.
> 
> The definition of cold memory highly depends on the devices and
> workloads, and thus setting a proper default age.min (e.g., min_age of
> DAMON_RECLAIM) is both important and nearly impossible to make suitable
> for all devices and workloads.

The recommended way is setting the min_age naive, and auto-tune it using DAMOS
quota or aim-oriented quota auto-tuning.  Have you considered using that?

> 
> Solution
> ========
> 
> Add a per-scheme sysfs attribute, schemes/<N>/sort_type, whose default
> value is 'none'. A scheme can set it to 'score_desc', which makes the
> scheme to collect the target regions and apply its action in descending
> order of the regions' scores, as calculated by the ops.get_scheme_score()
> callback, during the application. For a pageout scheme, the callback
> returns the coldness score of each region.
> 
> Instead of modifying the region list, DAMON copies the target valid
> regions into a temporary array, sorts the array in descending order of
> the regions' scores, and applies the action in the sorted order.
> 
> With this, the scheme gives absolute priority to the highest-scored
> region. For example, a pageout scheme with 'score_desc' reclaims the
> coldest region of the target first. Users can thus keep the age.min
> relatively small and let the score ordering do the precise
> prioritization.
> 
> Note that regions are applied in the score order, not the address order.
> Therefore, the address-based quota charge resume mechanism is not
> available for such schemes. Instead, the quota is spent on the
> highest-scored regions of each charge window. Applying an action
> resets the age of the applied regions (except for 'stat' action), so
> those regions are naturally excluded from the next window if the scheme
> has a non-zero age.min. Also, when a region is split for the quota, the
> age of the split-out part is preserved, and thus the highest-scored
> region is continuously applied until it is fully reclaimed, even when it
> is larger than the remaining quota of a single window.

This sounds like very similar to DAMOS quota's regions prioritizing mechanism.
Have you considered using DAMOS quota?  If so, could you please add why
proposing this new mechanism, with some comparisons?

> 
> Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com>
> ---
> I am currently running the corresponding benchmarks to ensure that this
> does not introduce too much performance overhead, at least not on my
> device.
> 
> The purpose of sending this patch is to make sure this is a right
> direction.
> 
> About my device/VM
> ==================
> 
> CPU: AMD Ryzen 5 5600H (12 Cores)
> RAM: 8GiB in VM (4GiB + 4GiB ZRAM)
> 
> I currently foresee two potential issues with thiss patch, though I
> have not obtained the test results yet, so these are just guesses.
> 
> 1. Excessive memory allocations and deallocations 
> 
>     The default aggr_interval is 100ms. Executing at least one
>     allocation and deallocation every 100ms is very likely to cause
>     unnecessary performance overhead.
> 
>     I think this issue could be resolved by having the scheme maintain
>     its own buffer.
> 
> 2. Performance overhead of re-ordering
> 
>     Although on my device the number of regions is not very large, and
>     DAMON's default limit of 1,000 regions also helps avoid performance
>     overhead, I suspect that large servers might not stick to just 1,000
>     regions.
> 
>     A solution I can think of is using a Top K Min Heap, though that
>     might significantly increase code complexity and reduce readability.

I'm indeed concerned about this.

> 
> As a reminder, these are just my __guesses__ and do not necessarily
> reflect what will happen in practice. I will send another email after
> completing the benchmarks and micro-performance testing.
> 
>  include/linux/damon.h    |   6 ++
>  mm/damon/core.c          | 125 ++++++++++++++++++++++++++++++++++++++-
>  mm/damon/sysfs-schemes.c |  57 ++++++++++++++++++
>  3 files changed, 187 insertions(+), 1 deletion(-)

I to be honest feel this is too much for "just guess" based patch.  I'd like to
get clear reasoning of the change and/or test results before reviewing the
code.  So I'm holding the code review until the high level discussion is done.


Thanks,
SJ

[...]

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

[Liew Rui Yan]

Hi SJ,

First of all, I apologize for the noise, and I will drop this patch.
Secondly, thank you for pointing this out, it made me realize that I was
fixing a non-existent problem.

On Tue,  4 Aug 2026 06:58:33 -0700 SJ Park <sj@kernel.org> wrote:

> The recommended way is setting the min_age naive, and auto-tune it using DAMOS
> quota or aim-oriented quota auto-tuning.  Have you considered using that?

> This sounds like very similar to DAMOS quota's regions prioritizing mechanism.
> Have you considered using DAMOS quota?  If so, could you please add why
> proposing this new mechanism, with some comparisons?

Yes, I have. And you are right; the existing mechanism is better. I
wrote this patch because I misunderstood quota.min_score and did not
even check the implementation of damos_adjust_quota().

Whether when implementing dama [1] or this damos_sort_type, I failed to
realize that min_age might not be such an important parameter on a
long-running system, and there is really no need to introduce
auto-tuning or a new mechanism for it.

> I to be honest feel this is too much for "just guess" based patch.  I'd like to
> get clear reasoning of the change and/or test results before reviewing the
> code.  So I'm holding the code review until the high level discussion is done.

Moving forward, I will ensure I am not "reinventing the wheel" or
"fixing non-existent problem".

Thank you again for pointing this out. In the future contributions, I
will try my best to avoid spending time fixing problems that do not
actually exist.

[1] https://lore.kernel.org/damon/20260628085155.20828-1-aethernet65535@gmail.com

Best regards,
Rui Yan

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

[SJ Park]

On Wed,  5 Aug 2026 16:12:46 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> Hi SJ,
> 
> First of all, I apologize for the noise, and I will drop this patch.
> Secondly, thank you for pointing this out, it made me realize that I was
> fixing a non-existent problem.

No worry, it was good to discuss the idea together!

> 
> On Tue,  4 Aug 2026 06:58:33 -0700 SJ Park <sj@kernel.org> wrote:
> 
> > The recommended way is setting the min_age naive, and auto-tune it using DAMOS
> > quota or aim-oriented quota auto-tuning.  Have you considered using that?
> 
> > This sounds like very similar to DAMOS quota's regions prioritizing mechanism.
> > Have you considered using DAMOS quota?  If so, could you please add why
> > proposing this new mechanism, with some comparisons?
> 
> Yes, I have. And you are right; the existing mechanism is better. I
> wrote this patch because I misunderstood quota.min_score and did not
> even check the implementation of damos_adjust_quota().

Thank you for transparently clarifying these details, Liew.  This is very
helpful at conversations.

> 
> Whether when implementing dama [1] or this damos_sort_type, I failed to
> realize that min_age might not be such an important parameter on a
> long-running system, and there is really no need to introduce
> auto-tuning or a new mechanism for it.
> 
> > I to be honest feel this is too much for "just guess" based patch.  I'd like to
> > get clear reasoning of the change and/or test results before reviewing the
> > code.  So I'm holding the code review until the high level discussion is done.
> 
> Moving forward, I will ensure I am not "reinventing the wheel" or
> "fixing non-existent problem".

I appreciate that.  That will be super helpful.  But I understand we are human
that cannot "ensure" everything always.  Please try not to spend too much time
on unnecessary thing.  But don't be scared at making some mistakes, because we
learn from mistakes.

> 
> Thank you again for pointing this out. In the future contributions, I
> will try my best to avoid spending time fixing problems that do not
> actually exist.

Sounds nice.  And feel free to ask questions or help.

> 
> [1] https://lore.kernel.org/damon/20260628085155.20828-1-aethernet65535@gmail.com


Thanks,
SJ

[...]