Skip to contentSkip to conversation

mm/damon: introduce DAMOS failed region quota charge ratio

RFC PATCH Superseded SeongJae Park 1 participant 14 messages Apr 5

Subsystems (5)

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

SeongJae Park

author maintainer
#0 lore raw

v2 cover mm/damon: introduce DAMOS failed region quota charge ratio

TL; DR: Let users set different DAMOS quota charge ratios for DAMOS action failed regions, for deterministic and consistent DAMOS action progress.

Common Reports: Unexpectedly Slow DAMOS
=======================================

One common issue report that we get from DAMON users is that DAMOS action applying progress speed is sometimes much slower than expected. And one common root cause is that the DAMOS quota is exceeded by the action applying failed memory regions.

For example, a group of users tried to run DAMOS-based proactive memory reclamation (DAMON_RECLAIM) with 100 MiB per second DAMOS quota. They ran it on a system having no active workload which means all memory of the system is cold. The expectation was that the system will show 100 MiB per second reclamation until (nearly) all memory is reclaimed. But what they found is that the speed is quite inconsistent and sometimes it becomes very slower than the expectation, sometimes even no reclamation at all for about tens of seconds. The upper limit of the speed (100 MiB per second) was being kept as expected, though.

By monitoring the qt_exceeds (number of DAMOS quota exceed events) DAMOS stat, we found DAMOS quota is always exceeded when the speed is slow. By monitoring sz_tried and sz_applied (the total amount of DAMOS action tried memory and succeeded memory) DAMOS stats together, we found the reclamation attempts nearly always failed when the speed is slow.

DAMOS quota charges DAMOS action tried regions regardless of the successfulness of the try. Hence in the example reported case, there was unreclaimable memory spread around the system memory. Sometimes nearly 100 MiB of memory that DAMOS tried to reclaim in the given quota interval was reclaimable, and therefore showed nearly 100 MiB per second speed. Sometimes nearly 99 MiB of memory that DAMOS was trying to reclaim in the given quota interval was unreclaimable, and therefore showing only about 1 MiB per second reclaim speed.

We explained it is an expected behavior of the feature rather than a bug, as DAMOS quota is there for only the upper-limit of the speed. The users agreed and later reported a huge win from the adoption of DAMON_RECLAIM on their products.

It is Not a Bug but a Feature; But...
=====================================

So nothing is broken. DAMOS quota is working as intended, as the upper limit of the speed. It also provides its behavior observability via DAMOS stat. In the real world production environment that runs long term active workloads and matters stability, the speed sometimes being slow is not a real problem.

But, the non-deterministic behavior is sometimes annoying, especially in lab environments. Even in a realistic production environment, when there is a huge amount of DAMOS action unapplicable memory, the speed could be problematically slow. Let's suppose a virtual machines provider that setup 99% of the host memory as hugetlb pages that cannot be reclaimed, to give it to virtual machines. Also, when aim-oriented DAMOS auto-tuning is applied, this could also make the internal feedback loop confused.

The intention of the current behavior was that trying DAMOS action to regions would anyway impose some overhead, and therefore somehow be charged. But in the real world, the overhead for failed action is much lighter than successful action. Charging those at the same ratio may be unfair, or at least suboptimum in some environments.

DAMOS Action Failed Region Quota Charge Ratio
=============================================

Let users set the charge ratio for the action-failed memory, for more optimal and deterministic use of DAMOS. It allows users to specify the numerator and the denominator of the ratio for flexible setup. For example, let's suppose the numerator and the denominator are set to 1 and 4,096, respectively. The ratio is 1 / 4,096. A DAMOS scheme action is applied to 5 GiB memory. For 1 GiB of the memory, the action is succeeded. For the rest (4 GiB), the action is failed. Then, only 1 GiB and 1 MiB quota is charged.

The optimal charge ratio will depend on the use case and system/workload. I'd recommend starting from setting the nominator as 1 and the denominator as PAGE_SIZE and tune based on the results, because many DAMOS actions are applied at page level.

Tests
=====

I tested this feature in the steps below.

1. Allocate 50% of system memory and mlock() it using a test program.
2. Fill up the page cache to exhaust nearly all free memory.
3. Start DAMON-based proactive reclamation with 100 MiB/second DAMOS hard-quota. Auto-tune the DAMOS soft-quota under the hard-quota for achieving 40% free memory of the system with 'temporal' tuner.

For step 1, I run a simple C program that is written by Gemini. It is quite straightforward, so I'm not sharing the code here.

For step 2, I use dd command like below:

dd if=/dev/zero of=foo bs=1M count=$50_percent_of_system_memory

For step 3, I use the latest version of DAMON user-space tool (damo) like below.

    sudo damo start --damos_action pageout \
            ` # Do the pageout only up to 100 MiB per second ` \
            --damos_quota_space 100M --damos_quota_interval 1s \
            ` # Auto-tune the quota below the hard quota aiming` \
            ` # 40% free memory of the node 0 ` \
            ` # (entire node of the test system)` \
            --damos_quota_goal node_mem_free_bp 40% 0 \
            ` # use temporal tuner, which is easy to understnd ` \
            --damos_quota_goal_tuner temporal

As expected, the progress of the reclamation is not consistent, because the quota is exceeded for the failed reclamation of the unreclaimable memory.

I do this again, but with the failed region charge ratio feature. For this, the above 'damo' command is used, after appending command line option for setup of the charge ratio like below. Note that the option was added to 'damo' after v3.1.9.

    sudo ./damo start --damos_action pageout \
            [...]
            ` # quota-charge only 1/4096 for pageout-failed regions ` \
            --damos_quota_fail_charge_ratio 1 4096

The progress of the reclamation was nearly 100 MiB per second until the goal was achieved, meeting the expectation.

Patches Sequence
================

Patch 1 implements the feature and exposes it via DAMON core API. Patch 2 implements DAMON sysfs ABI for the feature. Three following patches (3-5) document the feature and ABI on design, usage, and ABI documents, respectively. Four patches for testing of the new feature follow. Patch 6 implements a kunit test for the feature. Patches 7 and 8 extend DAMON selftest helpers for DAMON sysfs control and internal state dumping for adding a new selftest for the feature. Patch 9 extends existing DAMON sysfs interface selftest to test the new feature using the extended helper scripts.

Changelog
=========

Changes from RFC v1
(https://lore.kernel.org/[email protected])
- Avoid overflows in charge amount calculation.
- Fix/wordsmith documentation for grammar, typo, and wrong examples.
- Improve unit test for more consistent comparison source use.

SeongJae Park (9):
mm/damon/core: introduce failed region quota charge ratio mm/damon/sysfs-schemes: implement fail_charge_{num,denom} files Docs/mm/damon/design: document fail_charge_{num,denom} Docs/admin-guide/mm/damon/usage: document fail_charge_{num,denom}
files
Docs/ABI/damon: document fail_charge_{num,denom} mm/damon/tests/core-kunit: test fail_charge_{num,denom} committing selftets/damon/_damon_sysfs: support failed region quota charge ratio selftests/damon/drgn_dump_damon_status: support failed region quota
charge ratio
selftets/damon/sysfs.py: test failed region quota charge ratio

 .../ABI/testing/sysfs-kernel-mm-damon         | 12 +++++
 Documentation/admin-guide/mm/damon/usage.rst  | 18 +++++--
 Documentation/mm/damon/design.rst             | 21 ++++++++
 include/linux/damon.h                         |  9 ++++
 mm/damon/core.c                               | 21 +++++++-
 mm/damon/sysfs-schemes.c                      | 54 +++++++++++++++++++
 mm/damon/tests/core-kunit.h                   |  6 +++
 tools/testing/selftests/damon/_damon_sysfs.py | 21 +++++++-
 .../selftests/damon/drgn_dump_damon_status.py |  2 +
 tools/testing/selftests/damon/sysfs.py        |  6 +++
 10 files changed, 163 insertions(+), 7 deletions(-)

base-commit: 36eed23041e834175ca57dc38ac0e128808b1abb

Signature · 1 line
2.47.3
author maintainer
#1 lore raw

RFC PATCH v2 1/9 mm/damon/core: introduce failed region quota charge ratio

DAMOS quota is charged to all DAMOS action application attempted memory, regardless of how much of the memory the action was successful and failed. This makes understanding quota behavior without DAMOS stat but only with end level metrics (e.g., increased amount of free memory for DAMOS_PAGEOUT action) difficult. Also, charging action-failed memory same as action-successful memory is somewhat unfair, as successful action application will induce more overhead in most cases.

Introduce DAMON core API for setting the charge ratio for such action-failed memory. It allows API callers to specify the ratio in a flexible way, by setting the numerator and the denominator.

---
 include/linux/damon.h |  9 +++++++++
 mm/damon/core.c       | 21 ++++++++++++++++++++-
 2 files changed, 29 insertions(+), 1 deletion(-)
include/linux/damon.h +9 -0
index 4b69f4553267d..9ab7331775b9e 100644
@@ -233,6 +233,8 @@ enum damos_quota_goal_tuner {
  * @goals:		Head of quota tuning goals (&damos_quota_goal) list.
  * @goal_tuner:		Goal-based @esz tuning algorithm to use.
  * @esz:		Effective size quota in bytes.
+ * @fail_charge_num:	Failed regions charge rate numerator.
+ * @fail_charge_denom:	Failed regions charge rate denominator.
  *
  * @weight_sz:		Weight of the region's size for prioritization.
  * @weight_nr_accesses:	Weight of the region's nr_accesses for prioritization.
@@ -262,6 +264,10 @@ enum damos_quota_goal_tuner {
  *
  * The resulting effective size quota in bytes is set to @esz.
  *
+ * For DAMOS action applying failed amount of regions, charging those same to
+ * those that the action has successfully applied may be unfair.  For the
+ * reason, 'the size * @fail_charge_num / @fail_charge_denom' is charged.
+ *
  * For selecting regions within the quota, DAMON prioritizes current scheme's
  * target memory regions using the &struct damon_operations->get_scheme_score.
  * You could customize the prioritization logic by setting &weight_sz,
@@ -276,6 +282,9 @@ struct damos_quota {
 	enum damos_quota_goal_tuner goal_tuner;
 	unsigned long esz;
 
+	unsigned int fail_charge_num;
+	unsigned int fail_charge_denom;
+
 	unsigned int weight_sz;
 	unsigned int weight_nr_accesses;
 	unsigned int weight_age;
mm/damon/core.c +20 -1
index fe5a4a8d5b294..fdeab474b7456 100644
@@ -918,6 +918,8 @@ static int damos_commit_quota(struct damos_quota *dst, struct damos_quota *src)
 	if (err)
 		return err;
 	dst->goal_tuner = src->goal_tuner;
+	dst->fail_charge_num = src->fail_charge_num;
+	dst->fail_charge_denom = src->fail_charge_denom;
 	dst->weight_sz = src->weight_sz;
 	dst->weight_nr_accesses = src->weight_nr_accesses;
 	dst->weight_age = src->weight_age;
@@ -2042,6 +2044,23 @@ static void damos_walk_cancel(struct damon_ctx *ctx)
 	mutex_unlock(&ctx->walk_control_lock);
 }
 
+static void damos_charge_quota(struct damos_quota *quota,
+		unsigned long sz_region, unsigned long sz_applied)
+{
+	/*
+	 * sz_applied could be bigger than sz_region, depending on ops
+	 * implementation of the action, e.g., damos_pa_pageout().  Charge only
+	 * the region size in the case.
+	 */
+	if (!quota->fail_charge_denom || sz_applied > sz_region)
+		quota->charged_sz += sz_region;
+	else
+		quota->charged_sz += sz_applied + mult_frac(
+				(sz_region - sz_applied),
+				quota->fail_charge_num,
+				quota->fail_charge_denom);
+}
+
 static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
 		struct damon_region *r, struct damos *s)
 {
@@ -2098,7 +2117,7 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
 		ktime_get_coarse_ts64(&end);
 		quota->total_charged_ns += timespec64_to_ns(&end) -
 			timespec64_to_ns(&begin);
-		quota->charged_sz += sz;
+		damos_charge_quota(quota, sz, sz_applied);
 		if (damos_quota_is_set(quota) &&
 				quota->charged_sz >= quota->esz) {
 			quota->charge_target_from = t;
Signature · 1 line
2.47.3
author maintainer
#2 lore raw

Re: (sashiko review) [RFC PATCH v2 1/9] mm/damon/core: introduce failed region quota charge ratio

TL;DR: sashiko found one minor issue that better not to be fixed, one issue that orthogonal to this patch, and one minor issue that better to be fixed. I will separately work on the hotfix of the second issue and address the third issue in the next revision of this series.

Forwarding sashiko review in a reply format with my inline comments below, for sharing details of my view and the revision plan, and also doing followup discussions via mails if needed.

Quoted 39 lines
# review url: https://sashiko.dev/#/patchset/[email protected]
# start of sashiko.dev inline review
commit feec6568732fbe7a81b21aa5fac5c470ce0f7381
Author: SeongJae Park <[email protected]>
Subject: mm/damon/core: introduce failed region quota charge ratio

This commit introduces a configurable ratio to charge memory quota for DAMOS actions that failed, aiming for fairer quota accounting when actions do not fully succeed.

Quoted 3 lines
mm/damon/core.c +0 -0

[ ... ]

Quoted 10 lines
mm/damon/core.c +6 -0
@@ -2041,6 +2043,23 @@ static void damos_walk_cancel(struct damon_ctx *ctx)
 	mutex_unlock(&ctx->walk_control_lock);
 }
 
+static void damos_charge_quota(struct damos_quota *quota,
+		unsigned long sz_region, unsigned long sz_applied)
+{
+	/*
+	 * sz_applied could be bigger than sz_region, depending on ops
+	 * implementation of the action, e.g., damos_pa_pageout().  Charge only

mm/damon/core.c static void damos_walk_cancel(struct damon_ctx *ctx) · line 2061

Quoted 10 lines
mm/damon/core.c +10 -0
+	 * the region size in the case.
+	 */
+	if (!quota->fail_charge_denom || sz_applied > sz_region)
+		quota->charged_sz += sz_region;
+	else
+		quota->charged_sz += sz_applied + mult_frac(
+				(sz_region - sz_applied),
+				quota->fail_charge_num,
+				quota->fail_charge_denom);
+}

Can mult_frac() overflow here?

Quoted 4 lines

If a user configures fail_charge_num to a large value, the multiplication of the failed region size (which could be terabytes) and fail_charge_num could exceed ULONG_MAX on 64-bit systems, resulting in a truncated value added to charged_sz.

It can indeed make DAMOS work in an odd manner, but it doesn't break something in real. Also, the real world privileged users ain't set such large value. I'd not make change for this, as I prefer code simplicity here rather than handling the weird user inputs. We can revisit this later if a real issue from this is reported, or a simple fix is found.

Quoted 10 lines

Also, if fail_charge_num is 0 and actions fail, charged_sz increases by 0, leaving total_charged_sz at 0.

If total_charged_ns continues to accumulate the time spent attempting these actions, throughput in damos_set_effective_quota() would evaluate to 0:

throughput = mult_frac(quota->total_charged_sz, 1000000,

                       quota->total_charged_ns);
Quoted 8 lines

If throughput is 0, the effective size quota (esz) becomes 0. When esz is 0, damos_apply_scheme() calculates sz as 0:

sz = ALIGN_DOWN(quota->esz - quota->charged_sz, c->min_region_sz);

This triggers the if (!sz) goto update_stat; condition, skipping time measurement entirely. Does this cause the scheme to permanently lock up with 0 throughput and 0 quota?

Nice finding. But the issue exists even before this patch, because throughput could be zero if 'total_charged_sz < total_charged_ns / 1000000'.

I will fix this with a separate hotfix, like below:

'''

mm/damon/core.c +1 -1
@@ -2683,7 +2683,7 @@ static void damos_set_effective_quota(struct damos_quota *quota)
        }
 
        if (quota->ms) {
-               if (quota->total_charged_ns)
+               if (quota->total_charged_ns && quota->total_charged_sz)
                        throughput = mult_frac(quota->total_charged_sz,
                                        1000000, quota->total_charged_ns);
                else

'''

Quoted 23 lines

[ ... ]

mm/damon/core.c static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t, · line 2122

Quoted 9 lines
mm/damon/core.c +1 -1
@@ -2097,7 +2116,7 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
 		ktime_get_coarse_ts64(&end);
 		quota->total_charged_ns += timespec64_to_ns(&end) -
 			timespec64_to_ns(&begin);
-		quota->charged_sz += sz;
+		damos_charge_quota(quota, sz, sz_applied);
 		if (damos_quota_is_set(quota) &&
 				quota->charged_sz >= quota->esz) {
 			quota->charge_target_from = t;

Since the new fractional charging logic can produce unaligned charged_sz values, what happens if the remaining quota drops below min_region_sz?

In damos_apply_scheme(), sz is calculated as:

sz = ALIGN_DOWN(quota->esz - quota->charged_sz, c->min_region_sz);

If esz - charged_sz < c->min_region_sz, sz evaluates to 0. It then skips the charge update and jumps to update_stat.

Quoted 8 lines

Because charged_sz doesn't increase, this skip condition evaluates to true for every subsequent region, iterating through the remaining address space without doing anything.

Furthermore, the loop exits without ever satisfying charged_sz >= esz. Without satisfying this condition, charge_target_from is never set, causing the next charge window to start from the beginning of the address space again. Does this prevent tail regions from ever being processed?

Good finding. Not a critical issue for a quite rare case, but I agree this (charge_target_from never set) is better to be fixed. This happens because the quota exceed check logic is not aware of this case. I will address this in the next version by updating the quota exceed check to aware of this case, like below.

'''

mm/damon/core.c +11 -6
index 47770221abe29..42e444b48634f 100644
@@ -2320,6 +2320,14 @@ static void damos_charge_quota(struct damos_quota *quota,
 				quota->fail_charge_denom);
 }
 
+static bool damos_quota_is_full(struct damos_quota *quota,
+		unsigned long min_region_sz)
+{
+	if (!damos_quota_is_set(quota))
+		return false;
+	return quota->charged_sz + min_region_sz > quota->esz;
+}
+
 static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
 		struct damon_region *r, struct damos *s)
 {
@@ -2377,8 +2385,7 @@ 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_set(quota) &&
-				quota->charged_sz >= quota->esz) {
+		if (damos_quota_is_full(quota, c->min_region_sz)) {
 			quota->charge_target_from = t;
 			quota->charge_addr_from = r->ar.end + 1;
 		}
@@ -2406,8 +2413,7 @@ static void damon_do_apply_schemes(struct damon_ctx *c,
 			continue;
 
 		/* Check the quota */
-		if (damos_quota_is_set(quota) &&
-				quota->charged_sz >= quota->esz)
+		if (damos_quota_is_full(quota, c->min_region_sz))
 			continue;
 
 		if (damos_skip_charged_region(t, r, s, c->min_region_sz))
@@ -2736,8 +2742,7 @@ static void damos_adjust_quota(struct damon_ctx *c, struct damos *s)
 	if (!time_in_range_open(jiffies, quota->charged_from,
 				quota->charged_from +
 				msecs_to_jiffies(quota->reset_interval))) {
-		if (damos_quota_is_set(quota) &&
-				quota->charged_sz >= quota->esz)
+		if (damos_quota_is_full(quota, c->min_region_sz))
 			s->stat.qt_exceeds++;
 		quota->total_charged_sz += quota->charged_sz;
 		if (quota->total_charged_sz > ULONG_MAX / 2 ||

'''

Quoted 4 lines

# end of sashiko.dev inline review
# review url: https://sashiko.dev/#/patchset/[email protected]

Thanks,
SJ

# hkml [1] generated a draft of this mail.  You can regenerate
# this using below command:
#
#     hkml patch sashiko_dev --for_forwarding \
#             [email protected]
#
# [1] https://github.com/sjp38/hackermail
author maintainer
#3 lore raw

RFC PATCH v2 2/9 mm/damon/sysfs-schemes: implement fail_charge_{num,denom} files

In reply to SeongJae Park

Implement the user-space ABI for the DAMOS action failed region quota-charge ratio setup. For this, add two new sysfs files under the DAMON sysfs interface for DAMOS quotas. Names of the files are fail_charge_num and fail_charge_denom, and work for reading and setting the numerator and denominator of the failed regions charge ratio.

---
 mm/damon/sysfs-schemes.c | 54 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)
mm/damon/sysfs-schemes.c +54 -0
index 5186966dafb35..d5bfba79f105f 100644
@@ -1489,6 +1489,8 @@ struct damon_sysfs_quotas {
 	unsigned long reset_interval_ms;
 	unsigned long effective_sz;	/* Effective size quota in bytes */
 	enum damos_quota_goal_tuner goal_tuner;
+	unsigned int fail_charge_num;
+	unsigned int fail_charge_denom;
 };
 
 static struct damon_sysfs_quotas *damon_sysfs_quotas_alloc(void)
@@ -1663,6 +1665,48 @@ static ssize_t goal_tuner_store(struct kobject *kobj,
 	return -EINVAL;
 }
 
+static ssize_t fail_charge_num_show(struct kobject *kobj,
+		struct kobj_attribute *attr, char *buf)
+{
+	struct damon_sysfs_quotas *quotas = container_of(kobj,
+			struct damon_sysfs_quotas, kobj);
+
+	return sysfs_emit(buf, "%u\n", quotas->fail_charge_num);
+}
+
+static ssize_t fail_charge_num_store(struct kobject *kobj,
+		struct kobj_attribute *attr, const char *buf, size_t count)
+{
+	struct damon_sysfs_quotas *quotas = container_of(kobj,
+			struct damon_sysfs_quotas, kobj);
+	int err = kstrtouint(buf, 0, &quotas->fail_charge_num);
+
+	if (err)
+		return -EINVAL;
+	return count;
+}
+
+static ssize_t fail_charge_denom_show(struct kobject *kobj,
+		struct kobj_attribute *attr, char *buf)
+{
+	struct damon_sysfs_quotas *quotas = container_of(kobj,
+			struct damon_sysfs_quotas, kobj);
+
+	return sysfs_emit(buf, "%u\n", quotas->fail_charge_denom);
+}
+
+static ssize_t fail_charge_denom_store(struct kobject *kobj,
+		struct kobj_attribute *attr, const char *buf, size_t count)
+{
+	struct damon_sysfs_quotas *quotas = container_of(kobj,
+			struct damon_sysfs_quotas, kobj);
+	int err = kstrtouint(buf, 0, &quotas->fail_charge_denom);
+
+	if (err)
+		return -EINVAL;
+	return count;
+}
+
 static void damon_sysfs_quotas_release(struct kobject *kobj)
 {
 	kfree(container_of(kobj, struct damon_sysfs_quotas, kobj));
@@ -1683,12 +1727,20 @@ static struct kobj_attribute damon_sysfs_quotas_effective_bytes_attr =
 static struct kobj_attribute damon_sysfs_quotas_goal_tuner_attr =
 		__ATTR_RW_MODE(goal_tuner, 0600);
 
+static struct kobj_attribute damon_sysfs_quotas_fail_charge_num_attr =
+		__ATTR_RW_MODE(fail_charge_num, 0600);
+
+static struct kobj_attribute damon_sysfs_quotas_fail_charge_denom_attr =
+		__ATTR_RW_MODE(fail_charge_denom, 0600);
+
 static struct attribute *damon_sysfs_quotas_attrs[] = {
 	&damon_sysfs_quotas_ms_attr.attr,
 	&damon_sysfs_quotas_sz_attr.attr,
 	&damon_sysfs_quotas_reset_interval_ms_attr.attr,
 	&damon_sysfs_quotas_effective_bytes_attr.attr,
 	&damon_sysfs_quotas_goal_tuner_attr.attr,
+	&damon_sysfs_quotas_fail_charge_num_attr.attr,
+	&damon_sysfs_quotas_fail_charge_denom_attr.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(damon_sysfs_quotas);
@@ -2776,6 +2828,8 @@ static struct damos *damon_sysfs_mk_scheme(
 		.weight_nr_accesses = sysfs_weights->nr_accesses,
 		.weight_age = sysfs_weights->age,
 		.goal_tuner = sysfs_quotas->goal_tuner,
+		.fail_charge_num = sysfs_quotas->fail_charge_num,
+		.fail_charge_denom = sysfs_quotas->fail_charge_denom,
 	};
 	struct damos_watermarks wmarks = {
 		.metric = sysfs_wmarks->metric,
Signature · 1 line
2.47.3
author maintainer
#4 lore raw

Re: (sashiko review) [RFC PATCH v2 2/9] mm/damon/sysfs-schemes: implement fail_charge_{num,denom} files

TL; DR: sashiko found no issue in this patch.

Forwarding full sashiko review in a reply format with my inline comments below, for sharing details of my view and doing followup discussions via mails if needed.

Quoted 40 lines
# review url: https://sashiko.dev/#/patchset/[email protected]
# start of sashiko.dev inline review
commit f076dcb3d9e8a7170daf7c4fcb085e3a2c869183
Author: SeongJae Park <[email protected]>

mm/damon/sysfs-schemes: implement fail_charge_{num,denom} files

Implement the user-space ABI for the DAMOS action failed region quota-charge ratio setup. For this, add two new sysfs files under the DAMON sysfs interface for DAMOS quotas.

Quoted 4 lines
mm/damon/sysfs-schemes.c +0 -0
index 5186966dafb35..d5bfba79f105f 100644

[ ... ]

Quoted 4 lines
mm/damon/sysfs-schemes.c +0 -0
@@ -1663,6 +1665,48 @@ static ssize_t goal_tuner_store(struct kobject *kobj,
 	return -EINVAL;
 }
 

[ ... ]

mm/damon/sysfs-schemes.c static ssize_t goal_tuner_store(struct kobject *kobj, · line 1685

Quoted 9 lines
mm/damon/sysfs-schemes.c +9 -0
+static ssize_t fail_charge_num_store(struct kobject *kobj,
+		struct kobj_attribute *attr, const char *buf, size_t count)
+{
+	struct damon_sysfs_quotas *quotas = container_of(kobj,
+			struct damon_sysfs_quotas, kobj);
+	int err = kstrtouint(buf, 0, &quotas->fail_charge_num);
+
+	if (err)
+		return -EINVAL;

Could this introduce a regression by allowing an integer overflow when calculating the charge ratio?

By using kstrtouint(), fail_charge_num can be set to any value up to UINT_MAX.

Later, in mm/damon/core.c:damos_charge_quota(), this numerator is passed to mult_frac(sz_region - sz_applied, fail_charge_num, fail_charge_denom).

Quoted 7 lines

The mult_frac() macro performs multiplications with the numerator. If a very large fail_charge_num is provided, could this overflow the unsigned long result, especially on 32-bit architectures?

When the calculation wraps around, quota->charged_sz could be incremented by a small or negative-equivalent value, potentially bypassing the DAMOS scheme quota limit.

Same question was raised to the previous version of this patch, and I decided to ignore this. Read the previous discussion for details.

Quoted 7 lines

mm/damon/sysfs-schemes.c

Quoted 3 lines
mm/damon/sysfs-schemes.c +2 -0
+	return count;
+}
# end of sashiko.dev inline review
# review url: https://sashiko.dev/#/patchset/[email protected]

Thanks,
SJ

# hkml [1] generated a draft of this mail.  You can regenerate
# this using below command:
#
#     hkml patch sashiko_dev --for_forwarding \
#             [email protected]
#
# [1] https://github.com/sjp38/hackermail
author maintainer
#5 lore raw

RFC PATCH v2 3/9 Docs/mm/damon/design: document fail_charge_{num,denom}

In reply to SeongJae Park

Update DAMON design document for the DAMOS action failed region quota charge ratio.

---
 Documentation/mm/damon/design.rst | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
Documentation/mm/damon/design.rst +21 -0
index 510ec6375178d..3ea9c81b756c8 100644
@@ -572,6 +572,27 @@ interface <sysfs_interface>`, refer to :ref:`weights <sysfs_quotas>` part of
 the documentation.
 
 
+.. _damon_design_damos_quotas_failed_memory_charging_ratio:
+
+Action-failed Memory Charging Ratio
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+DAMOS action to a given region can fail for some subsets of the memory of the
+region.  For example, if the action is ``pageout`` and the region has some
+unreclaimable pages, applying the action to the pages will fail.  The amount of
+system resource that is taken for such failed action applications is usually
+different from that for successful action applications.  For such cases, users
+can set different charging ratio for such failed memory.  The ratio can be
+specified using ``fail_charge_num`` and ``fail_charge_denom`` parameters.  The
+two parameters represent the numerator and denominator of the ratio.
+
+For example, let's suppose a DAMOS action is applied to a region of 1,000 MiB
+size.  The action is successfully applied to only 700 MiB of the region.
+``fail_charge_num`` and ``fail_charge_denom`` are set to ``1`` and ``1024``,
+respectively.  Then only 700 MiB and 300 KiB of size (``700 MiB + 300 MiB * 1 /
+1024``) will be charged.
+
+
 .. _damon_design_damos_quotas_auto_tuning:
 
 Aim-oriented Feedback-driven Auto-tuning
Signature · 1 line
2.47.3
author maintainer
#6 lore raw

Re: (sashiko review) [RFC PATCH v2 3/9] Docs/mm/damon/design: document fail_charge_{num,denom}

TL; DR: sashiko gave me a sounds-good suggestion. I will make a change on the next revision of this patch.

Forwarding full sashiko review in a reply format with my inline comments below, for sharing details of my view and doing followup discussions via mails if needed.

Quoted 36 lines
# review url: https://sashiko.dev/#/patchset/[email protected]
# start of sashiko.dev inline review
commit c4d090b9bc99178548ec06da52d6f75e0b392a34
Author: SeongJae Park <[email protected]>

Docs/mm/damon/design: document fail_charge_{num,denom}

Update the DAMON design document to describe the new fail_charge_num and fail_charge_denom parameters. These parameters configure the DAMOS action failed region quota charge ratio.

Quoted 11 lines
Documentation/mm/damon/design.rst +3 -0
index afc7d52bda2f7..8a8b04b5e8537 100644
@@ -565,6 +565,27 @@ interface <sysfs_interface>`, refer to :ref:`weights <sysfs_quotas>` part of
 the documentation.
 
 
+.. _damon_design_damos_quotas_failed_memory_charging_ratio:
+
+Action-failed Memory Charging Ratio

Documentation/mm/damon/design.rst interface <sysfs_interface>`, refer to :ref:`weights <sysfs_quotas>` part of · line 580

Quoted 10 lines
Documentation/mm/damon/design.rst +10 -0
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+DAMOS action to a given region can fail for some subsets of the memory of the
+region.  For example, if the action is ``pageout`` and the region has some
+unreclaimable pages, applying the action to the pages will fail.  The amount of
+system resource that is taken for such failed action applications is usually
+different from that for successful action applications.  For such cases, users
+can set different charging ratio for such failed memory.  The ratio can be
+specified using ``fail_charge_num`` and ``fail_charge_denom`` parameters.  The
+two parameters represent the numerator and denominator of the ratio.

Should this documentation include the fallback and default behavior when fail_charge_denom is set to 0?

Quoted 8 lines

Looking at the core kernel implementation in mm/damon/core.c:damos_charge_quota(), the code checks for !quota->fail_charge_denom and falls back to charging the entire region size to avoid a division-by-zero error.

Additionally, since struct damon_sysfs_quotas is allocated using kzalloc(), fail_charge_denom is 0 by default. Would it be helpful to explain this default 0 state and the resulting fallback behavior here?

Good suggestion, I will add that.

Quoted 12 lines
Quoted 8 lines
Documentation/mm/damon/design.rst +7 -0
+
+For example, let's suppose a DAMOS action is applied to a region of 1,000 MiB
+size.  The action is successfully applied to only 700 MiB of the region.
+``fail_charge_num`` and ``fail_charge_denom`` are set to ``1`` and ``1024``,
+respectively.  Then only 700 MiB and 300 KiB of size (``700 MiB + 300 MiB * 1 /
+1024``) will be charged.
+

[ ... ]

Quoted 2 lines
# end of sashiko.dev inline review
# review url: https://sashiko.dev/#/patchset/[email protected]

Thanks,
SJ

# hkml [1] generated a draft of this mail.  You can regenerate
# this using below command:
#
#     hkml patch sashiko_dev --for_forwarding \
#             [email protected]
#
# [1] https://github.com/sjp38/hackermail
author maintainer
#7 lore raw

RFC PATCH v2 4/9 Docs/admin-guide/mm/damon/usage: document fail_charge_{num,denom} files

In reply to SeongJae Park

Update DAMON usage document for the DAMOS action failed regions quota charge ratio control sysfs files.

---
 Documentation/admin-guide/mm/damon/usage.rst | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)
Documentation/admin-guide/mm/damon/usage.rst +14 -4
index bfdb717441f05..d5548e460857c 100644
@@ -84,7 +84,9 @@ comma (",").
     │ │ │ │ │ │ │ │ sz/min,max
     │ │ │ │ │ │ │ │ nr_accesses/min,max
     │ │ │ │ │ │ │ │ age/min,max
-    │ │ │ │ │ │ │ :ref:`quotas <sysfs_quotas>`/ms,bytes,reset_interval_ms,effective_bytes,goal_tuner
+    │ │ │ │ │ │ │ :ref:`quotas <sysfs_quotas>`/ms,bytes,reset_interval_ms,
+    │ │ │ │ │ │ │     effective_bytes,goal_tuner,
+    │ │ │ │ │ │ │     fail_charge_num,fail_charge_denom
     │ │ │ │ │ │ │ │ weights/sz_permil,nr_accesses_permil,age_permil
     │ │ │ │ │ │ │ │ :ref:`goals <sysfs_schemes_quota_goals>`/nr_goals
     │ │ │ │ │ │ │ │ │ 0/target_metric,target_value,current_value,nid,path
@@ -381,9 +383,10 @@ schemes/<N>/quotas/
 The directory for the :ref:`quotas <damon_design_damos_quotas>` of the given
 DAMON-based operation scheme.
 
-Under ``quotas`` directory, five files (``ms``, ``bytes``,
-``reset_interval_ms``, ``effective_bytes`` and ``goal_tuner``) and two
-directories (``weights`` and ``goals``) exist.
+Under ``quotas`` directory, seven files (``ms``, ``bytes``,
+``reset_interval_ms``, ``effective_bytes``, ``goal_tuner``, ``fail_charge_num``
+and ``fail_charge_denom``) and two directories (``weights`` and ``goals``)
+exist.
 
 You can set the ``time quota`` in milliseconds, ``size quota`` in bytes, and
 ``reset interval`` in milliseconds by writing the values to the three files,
@@ -402,6 +405,13 @@ the background design of the feature and the name of the selectable algorithms.
 Refer to :ref:`goals directory <sysfs_schemes_quota_goals>` for the goals
 setup.
 
+You can set the action-failed memory quota charging ratio by writing the
+numerator and the denominator for the ratio to ``fail_charge_num`` and
+``fail_charge_denom`` files, respectively.  Reading those files will return the
+current set values.  Refer to :ref:`design
+<damon_design_damos_quotas_failed_memory_charging_ratio>` for more details of
+the ratio feature.
+
 The time quota is internally transformed to a size quota.  Between the
 transformed size quota and user-specified size quota, smaller one is applied.
 Based on the user-specified :ref:`goal <sysfs_schemes_quota_goals>`, the
Signature · 1 line
2.47.3
author maintainer
#8 lore raw

RFC PATCH v2 5/9 Docs/ABI/damon: document fail_charge_{num,denom}

In reply to SeongJae Park

Update DAMON ABI document for the DAMOS action failed regions quota charge ratio control sysfs files.

---
 Documentation/ABI/testing/sysfs-kernel-mm-damon | 12 ++++++++++++
 1 file changed, 12 insertions(+)
Documentation/ABI/testing/sysfs-kernel-mm-damon +12 -0
index 7059f540940f0..971c22e34e722 100644
@@ -329,6 +329,18 @@ Contact:	SeongJae Park <[email protected]>
 Description:	Writing to and reading from this file sets and gets the
 		goal-based effective quota auto-tuning algorithm to use.
 
+What:		/sys/kernel/mm/damon/admin/kdamonds/<K>/contexts/<C>/schemes/<S>/quotas/fail_charge_num
+Date:		Mar 2026
+Contact:	SeongJae Park <[email protected]>
+Description:	Writing to and reading from this file sets and gets the
+		action-failed memory quota charging ratio numerator.
+
+What:		/sys/kernel/mm/damon/admin/kdamonds/<K>/contexts/<C>/schemes/<S>/quotas/fail_charge_denom
+Date:		Mar 2026
+Contact:	SeongJae Park <[email protected]>
+Description:	Writing to and reading from this file sets and gets the
+		action-failed memory quota charging ratio denominator.
+
 What:		/sys/kernel/mm/damon/admin/kdamonds/<K>/contexts/<C>/schemes/<S>/quotas/weights/sz_permil
 Date:		Mar 2022
 Contact:	SeongJae Park <[email protected]>
Signature · 1 line
2.47.3
author maintainer
#9 lore raw

RFC PATCH v2 6/9 mm/damon/tests/core-kunit: test fail_charge_{num,denom} committing

In reply to SeongJae Park

Extend damos_test_commit_quotas() kunit test to ensure damos_commit_quota() handles fail_charge_{num,denom} parameters.

---
 mm/damon/tests/core-kunit.h | 6 ++++++
 1 file changed, 6 insertions(+)
mm/damon/tests/core-kunit.h +6 -0
index 0030f682b23b7..1b23a22ac04c4 100644
@@ -694,6 +694,8 @@ static void damos_test_commit_quota(struct kunit *test)
 		.ms = 2,
 		.sz = 3,
 		.goal_tuner = DAMOS_QUOTA_GOAL_TUNER_CONSIST,
+		.fail_charge_num = 2,
+		.fail_charge_denom = 3,
 		.weight_sz = 4,
 		.weight_nr_accesses = 5,
 		.weight_age = 6,
@@ -703,6 +705,8 @@ static void damos_test_commit_quota(struct kunit *test)
 		.ms = 8,
 		.sz = 9,
 		.goal_tuner = DAMOS_QUOTA_GOAL_TUNER_TEMPORAL,
+		.fail_charge_num = 1,
+		.fail_charge_denom = 1024,
 		.weight_sz = 10,
 		.weight_nr_accesses = 11,
 		.weight_age = 12,
@@ -717,6 +721,8 @@ static void damos_test_commit_quota(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, dst.ms, src.ms);
 	KUNIT_EXPECT_EQ(test, dst.sz, src.sz);
 	KUNIT_EXPECT_EQ(test, dst.goal_tuner, src.goal_tuner);
+	KUNIT_EXPECT_EQ(test, dst.fail_charge_num, src.fail_charge_num);
+	KUNIT_EXPECT_EQ(test, dst.fail_charge_denom, src.fail_charge_denom);
 	KUNIT_EXPECT_EQ(test, dst.weight_sz, src.weight_sz);
 	KUNIT_EXPECT_EQ(test, dst.weight_nr_accesses, src.weight_nr_accesses);
 	KUNIT_EXPECT_EQ(test, dst.weight_age, src.weight_age);
Signature · 1 line
2.47.3
author maintainer
#10 lore raw

RFC PATCH v2 7/9 selftets/damon/_damon_sysfs: support failed region quota charge ratio

In reply to SeongJae Park

Extend _damon_sysfs.py for DAMOS action failed regions quota charge ratio setup, so that we can add kselftest for the new feature.

---
 tools/testing/selftests/damon/_damon_sysfs.py | 21 +++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)
tools/testing/selftests/damon/_damon_sysfs.py +19 -2
index 120b96ecbd741..8b12cc0484405 100644
@@ -132,14 +132,17 @@ class DamosQuota:
     goals = None                # quota goals
     goal_tuner = None           # quota goal tuner
     reset_interval_ms = None    # quota reset interval
+    fail_charge_num = None
+    fail_charge_denom = None
     weight_sz_permil = None
     weight_nr_accesses_permil = None
     weight_age_permil = None
     scheme = None               # owner scheme
 
     def __init__(self, sz=0, ms=0, goals=None, goal_tuner='consist',
-                 reset_interval_ms=0, weight_sz_permil=0,
-                 weight_nr_accesses_permil=0, weight_age_permil=0):
+                 reset_interval_ms=0, fail_charge_num=0, fail_charge_denom=0,
+                 weight_sz_permil=0, weight_nr_accesses_permil=0,
+                 weight_age_permil=0):
         self.sz = sz
         self.ms = ms
         self.reset_interval_ms = reset_interval_ms
@@ -151,6 +154,8 @@ class DamosQuota:
         for idx, goal in enumerate(self.goals):
             goal.idx = idx
             goal.quota = self
+        self.fail_charge_num = fail_charge_num
+        self.fail_charge_denom = fail_charge_denom
 
     def sysfs_dir(self):
         return os.path.join(self.scheme.sysfs_dir(), 'quotas')
@@ -197,6 +202,18 @@ class DamosQuota:
                 os.path.join(self.sysfs_dir(), 'goal_tuner'), self.goal_tuner)
         if err is not None:
             return err
+
+        err = write_file(
+                os.path.join(self.sysfs_dir(), 'fail_charge_num'),
+                self.fail_charge_num)
+        if err is not None:
+            return err
+        err = write_file(
+                os.path.join(self.sysfs_dir(), 'fail_charge_denom'),
+                self.fail_charge_denom)
+        if err is not None:
+            return err
+
         return None
 
 class DamosWatermarks:
Signature · 1 line
2.47.3
author maintainer
#11 lore raw

Re: (sashiko review) [RFC PATCH v2 7/9] selftets/damon/_damon_sysfs: support failed region quota charge ratio

TL; DR: sashiko found a typo and a false positive issue. I will fix the typo in the next revision of this patch.

Forwarding full sashiko review in a reply format with my inline comments below, for sharing details of my view and doing followup discussions via mails if needed.

Quoted 5 lines
# review url: https://sashiko.dev/#/patchset/[email protected]
# start of sashiko.dev inline review
commit 47cf42ea4e5f609d65881ae5f8f8491bb884a8bf
Author: SeongJae Park <[email protected]>
Quoted 8 lines

selftets/damon/_damon_sysfs: support failed region quota charge ratio

This commit extends the _damon_sysfs.py selftests utility to support the new DAMOS action failed regions quota charge ratio feature.

Quoted 1 line

selftets/damon/_damon_sysfs: support failed region quota charge ratio

This isn't a bug, but there is a typo (selftets) in the subject line.

Good eye, I will fix it in the next revision.

Quoted 19 lines
Quoted 5 lines
tools/testing/selftests/damon/_damon_sysfs.py +0 -0
index 2b4df655d9fd0..0f13512fa5e6d 100644

[ ... ]

Quoted 1 line
tools/testing/selftests/damon/_damon_sysfs.py +0 -0
@@ -132,14 +132,17 @@ class DamosQuota:

[ ... ]

tools/testing/selftests/damon/_damon_sysfs.py class DamosQuota: · line 145

Quoted 6 lines
     def __init__(self, sz=0, ms=0, goals=None, goal_tuner='consist',
tools/testing/selftests/damon/_damon_sysfs.py +3 -2
-                 reset_interval_ms=0, weight_sz_permil=0,
-                 weight_nr_accesses_permil=0, weight_age_permil=0):
+                 reset_interval_ms=0, fail_charge_num=0, fail_charge_denom=0,
+                 weight_sz_permil=0, weight_nr_accesses_permil=0,
+                 weight_age_permil=0):

Will inserting fail_charge_num and fail_charge_denom before the weight parameters cause issues for any existing code that passes arguments positionally?

Quoted 6 lines

Any scripts or in-flight patches instantiating DamosQuota using positional arguments for the weight parameters might silently assign those values to the new charge ratio variables instead.

Could these optional parameters be added to the end of the argument list to avoid altering the existing positional signature?

No. There is no existing code that calls DAmosQuota() with positional arguments. Hence no change to this patch is needed.

Quoted 4 lines

# end of sashiko.dev inline review
# review url: https://sashiko.dev/#/patchset/[email protected]

Thanks,
SJ

# hkml [1] generated a draft of this mail.  You can regenerate
# this using below command:
#
#     hkml patch sashiko_dev --for_forwarding \
#             [email protected]
#
# [1] https://github.com/sjp38/hackermail
author maintainer
#12 lore raw

RFC PATCH v2 8/9 selftests/damon/drgn_dump_damon_status: support failed region quota charge ratio

In reply to SeongJae Park

Extend drgn_dump_damon_status.py to dump DAMON internal state for DAMOS action failed regions quota charge ratio, to be able to show if the internal state for the feature is working, with future DAMON selftests.

---
 tools/testing/selftests/damon/drgn_dump_damon_status.py | 2 ++
 1 file changed, 2 insertions(+)
tools/testing/selftests/damon/drgn_dump_damon_status.py +2 -0
index 5b90eb8e7ef88..972948e6215f1 100755
@@ -112,6 +112,8 @@ def damos_quota_to_dict(quota):
         ['goals', damos_quota_goals_to_list],
         ['goal_tuner', int],
         ['esz', int],
+        ['fail_charge_num', int],
+        ['fail_charge_denom', int],
         ['weight_sz', int],
         ['weight_nr_accesses', int],
         ['weight_age', int],
Signature · 1 line
2.47.3
author maintainer
#13 lore raw

RFC PATCH v2 9/9 selftets/damon/sysfs.py: test failed region quota charge ratio

In reply to SeongJae Park

Extend sysfs.py DAMON selftest to setup DAMOS action failed region quota charge ratio and assert the setup is made into DAMON internal state.

---
 tools/testing/selftests/damon/sysfs.py | 6 ++++++
 1 file changed, 6 insertions(+)
tools/testing/selftests/damon/sysfs.py +6 -0
index b8d6e0f8fd906..d8d4717128898 100755
@@ -73,6 +73,10 @@ def assert_quota_committed(quota, dump):
             }
     assert_true(dump['goal_tuner'] == tuner_val[quota.goal_tuner],
                 'goal_tuner', dump)
+    assert_true(dump['fail_charge_num'] == quota.fail_charge_num,
+                'fail_charge_num', dump)
+    assert_true(dump['fail_charge_denom'] == quota.fail_charge_denom,
+                'fail_charge_denom', dump)
     assert_true(dump['weight_sz'] == quota.weight_sz_permil, 'weight_sz', dump)
     assert_true(dump['weight_nr_accesses'] == quota.weight_nr_accesses_permil,
                 'weight_nr_accesses', dump)
@@ -277,6 +281,8 @@ def main():
                         nid=1)],
                     goal_tuner='temporal',
                     reset_interval_ms=1500,
+                    fail_charge_num=1,
+                    fail_charge_denom=4096,
                     weight_sz_permil=20,
                     weight_nr_accesses_permil=200,
                     weight_age_permil=1000),
Signature · 1 line
2.47.3