mm/damon: introduce DAMOS failed region quota charge ratio
Subsystems (5)
SeongJae Park
maintainercover 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 temporalAs 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 4096The progress of the reclamation was nearly 100 MiB per second until the goal was achieved, meeting the expectation.
Patches Sequence
================
The first two patches make preparational changes. Patch 1 updates fully charged quota check to handle <min_region_sz remaining quota, which will be able to exist after this series is applied. Patch 2 merges regions after applying schemes is done as long as it is ok to do, since regions split operations for quota could happen much more frequently under a corner case that this series will make available.
Patch 3 implements the feature and exposes it via DAMON core API. Patch 4 implements DAMON sysfs ABI for the feature. Three following patches (5-7) document the feature and ABI on design, usage, and ABI documents, respectively. Four patches for testing of the new feature follow. Patch 8 implements a kunit test for the feature. Patches 9 and 10 extend DAMON selftest helpers for DAMON sysfs control and internal state dumping for adding a new selftest for the feature. Patch 11 extends existing DAMON sysfs interface selftest to test the new feature using the extended helper scripts.
Changelog
=========
Changes from RFC v5
(https://lore.kernel.org/[email protected])
- Merge back: merge whatever if it doesn't lose monitoring infomration and not violating min_nr_regions.
Changes from RFC v4
(https://lore.kernel.org/[email protected])
- Fix quota-sliced region merge-back issues.
- Use damon_for_each_region() instead of damon_for_each_region_safe().
- Avoid merging back of sliced but scheme unapplied regions, to keep the monitoring information.
Changes from RFC v3
(https://lore.kernel.org/[email protected])
- Make damos_quota_is_full() safe from overflow and easier to read.
- Avoid quota-based region split making too many new regions. Changes from RFC v2
(https://lore.kernel.org/[email protected])
- Handle <min_region_sz remaining quota.
- Document zero denum behavior.
- Fix typos: s/selftets/selftests/
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 (11):
mm/damon/core: handle <min_region_sz remaining quota as empty mm/damon/core: merge regions after applying DAMOS schemes 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 selftests/damon/_damon_sysfs: support failed region quota charge ratio selftests/damon/drgn_dump_damon_status: support failed region quota
charge ratio
selftests/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 | 22 +++++ include/linux/damon.h | 9 +++ mm/damon/core.c | 80 ++++++++++++++++--- 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, 213 insertions(+), 17 deletions(-)
base-commit: 45df8a80cb5d9b548f8586bf6dee79b6c77c3703
Signature · 1 line
2.47.3
SeongJae Park
maintainerRFC PATCH 1/11 mm/damon/core: handle <min_region_sz remaining quota as empty
Less than min_region_sz remaining quota effectively means the quota is fully charged. In other words, no remaining quota. This is because DAMOS actions are applied in the region granularity, and each region should have min_region_sz or larger size. However the existing fully charged quota check, which is also used for setting charge_target_from and charge_addr_from of the quota, is not aware of the case. For the reason, charge_target_from and charge_addr_from of the quota will not be updated in the case. This can result in DAMOS action being applied more frequently to a specific area of the memory.
The case is unreal because quota charging is also made in the region granularity. It could be changed in future, though. Actually, the following commit will make the change, by allowing users to set arbitrary quota charging ratio for action-failed regions. To be prepared for the change, update the fully charged quota checks to treat having less than min_region_sz remaining quota as fully charged.
--- mm/damon/core.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)
mm/damon/core.c +17 -6
@@ -2042,6 +2042,20 @@ static void damos_walk_cancel(struct damon_ctx *ctx)mutex_unlock (&ctx->walk_control_lock); } +static bool damos_quota_is_full (struct damos_quota *quota, +unsigned long min_region_sz) +{ +if (!damos_quota_is_set (quota)) +return false; +if (quota->charged_sz >= quota->esz) +return true; +/* +* DAMOS action is applied per region, so <min_region_sz remaining +* quota means the quota is effectively full. +*/ +return quota->esz - quota->charged_sz < min_region_sz; +} +static void damos_apply_scheme (struct damon_ctx *c,struct damon_target *t,struct damon_region *r,struct damos *s) {
@@ -2099,8 +2113,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); quota->charged_sz += sz; -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 ; }
@@ -2128,8 +2141,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))
@@ -2455,8 +2467,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; quota->charged_from = jiffies;
Signature · 1 line
2.47.3
SeongJae Park
maintainerRe: (sashiko review) [RFC PATCH v5.1 01/11] mm/damon/core: handle <min_region_sz remaining quota as empty
TL; DR: no blocker of this patch from sashiko review.
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 44 lines
# review url: https://sashiko.dev/#/patchset/[email protected] # start of sashiko.dev inline review commit 2f0a2c02b528b6c75ac3074d06b729bc93bd8589 Author: SeongJae Park <[email protected]>
mm/damon/core: handle <min_region_szremaining quota as emptyThis commit updates DAMON quota handling to treat a remaining quota of less than
min_region_szas effectively fully charged. This ensures thatcharge_target_fromandcharge_addr_fromof the quota are updated properly, avoiding applying DAMOS actions too frequently to specific memory areas.Quoted 4 lines
mm/damon/core.c +0
-0[ ... ]
mm/damon/core.c static void damon_do_apply_schemes(struct damon_ctx *c, · line 2144
Quoted 8 lines
mm/damon/core.c +1
-2@@ -2127,8 +2140,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 ;With this change,
damon_do_apply_schemes()ensures that we only proceed to calldamos_apply_scheme()when the remaining quota is at leastc->min_region_sz.Does this make the if (!sz) check inside
damos_apply_scheme()unreachable?mm/damon/core.c:damos_apply_scheme() { ... if (c->ops.apply_scheme) { if (damos_quota_is_set(quota) && quota->charged_sz + sz > quota->esz) { sz = ALIGN_DOWN(quota->esz - quota->charged_sz, c->min_region_sz); if (!sz) goto update_stat; ... }
Quoted 5 lines
Since the remaining quota (
quota->esz-quota->charged_sz) is guaranteed to be >=c->min_region_szwhen reaching this point,ALIGN_DOWNwill always result in sz >=c->min_region_sz.Could this check and the goto be removed to clean up the dead code?
Same question was asked on the previous version of this patch. My answer is also same. This makes sense but better to be done as a separate cleanup work.
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
SeongJae Park
maintainerRFC PATCH 2/11 mm/damon/core: merge regions after applying DAMOS schemes
In reply to SeongJae Parkdamos_apply_scheme() could split the given region if applying the scheme's action to the entire region can result in violating the quota-set upper limit. Keeping regions that are created by such split operations is unnecessary overhead.
The overhead would be negligible in the common case because such split operations could happen only up to the number of installed schemes per scheme apply interval. The following commit could make the impact larger, though. The following commit will allow the action-failed region to be charged in a different ratio. If both the ratio and the remaining quota is quite small while the region to apply the scheme is quite large and the action is nearly always failing, a high number of split operations could happen.
Remove the unnecessary overhead by merging regions after applying schemes is done for each region. The merge operation is made only if it will not lose monitoring information and keep min_nr_regions constraint. In the worst case, the max_nr_regions could still be violated until the next per-aggregation interval merge operation is made.
--- mm/damon/core.c | 36 ++++++++++++++++++++++++++++++++---- 1 file changed, 32 insertions(+), 4 deletions(-)
mm/damon/core.c +32 -4
@@ -2159,6 +2159,37 @@ static void damon_do_apply_schemes(struct damon_ctx *c, } } +static void damos_apply_target (struct damon_ctx *c,struct damon_target *t) +{ +struct damon_region *r; + +damon_for_each_region (r, t) { +struct damon_region *prev_r; + +damon_do_apply_schemes (c, t, r); +/* +* damon_do_apply_scheems() could split the region for the +* quota. Keeping the new slices is an overhead. Merge back +* the slices into the previous region if it doesn't lose any +* information. +*/ +if (damon_first_region (t) == r) +continue ; + prev_r =damon_prev_region (r); +if (prev_r->ar.end != r->ar.start) +continue ; +if (prev_r->age != r->age) +continue ; +if (prev_r->last_nr_accesses != r->last_nr_accesses) +continue ; +if (prev_r->nr_accesses != r->nr_accesses) +continue ; + prev_r->ar.end = r->ar.end; +damon_destroy_region (r, t); + r = prev_r; + } +} +/* * damon_feed_loop_next_input() - get next input to achieve a target score. * @last_input The last input.
@@ -2528,7 +2559,6 @@ static void damos_trace_stat(struct damon_ctx *c, struct damos *s)static void kdamond_apply_schemes (struct damon_ctx *c) {struct damon_target *t; -struct damon_region *r;struct damos *s;bool has_schemes_to_apply = false;
@@ -2551,9 +2581,7 @@ static void kdamond_apply_schemes(struct damon_ctx *c)damon_for_each_target (t, c) {if (c->ops.target_valid && c->ops.target_valid (t) == false)continue ; - -damon_for_each_region (r, t) -damon_do_apply_schemes (c, t, r); +damos_apply_target (c, t); }damon_for_each_scheme (s, c) {
Signature · 1 line
2.47.3
SeongJae Park
maintainerRe: (sashiko review) [RFC PATCH v5.1 02/11] mm/damon/core: merge regions after applying DAMOS schemes
TL; DR: sashiko found a typo and a fact that I forgot adding a fixup that I promised on the previous version. I will fix those in the next spin.
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 24 lines
# review url: https://sashiko.dev/#/patchset/[email protected] # start of sashiko.dev inline review commit 1a56d6af604a3fc86a23180a9619447641294a2f Author: SeongJae Park <[email protected]>
mm/damon/core: merge regions after applying DAMOS schemesThis commit introduces a mechanism to merge regions after applying schemes to avoid overhead from regions split during quota-limited scheme application.
Quoted 2 lines
mm/damon/core.c +0
-0[ ... ]
mm/damon/core.c static void damon_do_apply_schemes(struct damon_ctx *c, · line 2166
Quoted 5 lines
mm/damon/core.c +5
-0+static void damos_apply_target (struct damon_ctx *c,struct damon_target *t) +{ +struct damon_region *r; + +damon_for_each_region (r, t) {Is this list iteration pattern safe for long-term maintenance?
This loop uses
damon_for_each_region(), which is an unsafe list iterator, but destroys r inside the loop and reassigns r =prev_rto safely continue.
Quoted 6 lines
While this works by relying on internal macro expansion mechanics, if a future developer converts this to use
damon_for_each_region_safe(), the cached next pointer would be evaluated beforedamon_do_apply_schemes()potentially splits the region.Could this cause the loop to silently skip the newly inserted slices entirely?
Yes, such mistake could happen in future, and this code could be better written for long term maitenance. But that seems better to be revisited as a future cleanup work.
Quoted 8 lines
mm/damon/core.c static void damon_do_apply_schemes(struct damon_ctx *c, · line 2171
Quoted 6 lines
mm/damon/core.c +5
-0+struct damon_region *prev_r; + +damon_do_apply_schemes (c, t, r); +/* +* damon_do_apply_scheems() could split the region for the This isn't a bug, but there is a typo in the comment (scheems).
Good eyes! I will fix this in the next spin.
Quoted 17 lines
Quoted 17 lines
mm/damon/core.c +16
-0+* quota. Keeping the new slices is an overhead. Merge back +* the slices into the previous region if it doesn't lose any +* information. +*/ +if (damon_first_region (t) == r) +continue ; + prev_r =damon_prev_region (r); +if (prev_r->ar.end != r->ar.start) +continue ; +if (prev_r->age != r->age) +continue ; +if (prev_r->last_nr_accesses != r->last_nr_accesses) +continue ; +if (prev_r->nr_accesses != r->nr_accesses) +continue ; + prev_r->ar.end = r->ar.end;
Quoted 8 lines
mm/damon/core.c static void damon_do_apply_schemes(struct damon_ctx *c, · line 2191
Quoted 4 lines
mm/damon/core.c +4
-0+damon_destroy_region (r, t); + r = prev_r; + } +}The commit message explicitly states the merge operation is made only if it keeps the
min_nr_regionsconstraint. Does this inline merge logic bypass that constraint?
Argh, I forgot adding the fixup [1] to this patch. I will add that in the next version.
Quoted 14 lines
Earlier in the execution,
damon_apply_min_nr_regions()enforces the limit by calculating a size limit and splitting any region larger than this limit into smaller sub-regions. These newly created sub-regions naturally inherit identical age andnr_accessesvalues from their parent.When
kdamond_apply_schemes()subsequently callsdamos_apply_target(), this logic will immediately merge those freshly split regions back together because their age andnr_accessesmatch, entirely omitting the size limit check.Does this unintentionally destroy the spatial monitoring granularity every sample interval?
Quoted 7 lines
Additionally, when merging the regions, this code expands
prev_r->ar.endand destroys r without accounting forr->nr_accesses_bp.Since standard functions like
damon_merge_two_regions()re-average metrics includingnr_accesses_bp, could this inline discard of the fractionalnr_accesses_bptracking result in a slight precision loss, contrary to the commit message's goal of not losing monitoring information?
Yes, that's valid.
Quoted 4 lines
# end of sashiko.dev inline review # review url: https://sashiko.dev/#/patchset/[email protected]
[1] https://lore.kernel.org/[email protected]
Thanks,
SJ
SeongJae Park
maintainerRFC PATCH 3/11 mm/damon/core: introduce failed region quota charge ratio
In reply to SeongJae ParkDAMOS 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
@@ -235,6 +235,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.
@@ -264,6 +266,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,
@@ -278,6 +284,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
@@ -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 bool damos_quota_is_full (struct damos_quota *quota,unsigned long min_region_sz) {
@@ -2112,7 +2131,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_full (quota, c->min_region_sz)) { quota->charge_target_from = t; quota->charge_addr_from = r->ar.end +1 ;
Signature · 1 line
2.47.3
SeongJae Park
maintainerRe: (sashiko review) [RFC PATCH v5.1 03/11] mm/damon/core: introduce failed region quota charge ratio
TL; DR: sashiko found no blocker for 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 55 lines
# review url: https://sashiko.dev/#/patchset/[email protected] # start of sashiko.dev inline review commit cb228850cc3bb33cac86cdc674c79e2557af28a8 Author: SeongJae Park <[email protected]>
mm/damon/core: introduce failed region quota charge ratioThis commit introduces a DAMON core API to set the charge ratio for action-failed memory. It allows API callers to specify the ratio flexibly using a numerator and denominator to avoid charging failed memory the same as successfully applied memory.
Quoted 4 lines
mm/damon/core.c +0
-0[ ... ]
Quoted 9 lines
mm/damon/core.c +5
-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 mm/damon/core.c static void damos_walk_cancel(struct damon_ctx *ctx) · line 2060
Quoted 10 lines
mm/damon/core.c +10
-0+* 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);Could the
mult_frac()calculation silently overflow here?The
fail_charge_numandfail_charge_denomvariables are user-controlled unsigned int parameters without bounds checking. Themult_frac(x, numer, denom)macro evaluates as quot * numer + (rem * numer) / denom, using the type of x, which is unsigned long in this case.On a 32-bit architecture, if a user configures a fine-grained ratio like 1000000 / 1000000, the remainder could be up to 999999. The intermediate calculation for rem * numer would evaluate to nearly 10^12, which overflows the 32-bit limit.
Similarly, on 64-bit architectures, setting
fail_charge_numto a very large value likeUINT_MAXcould cause the quot * numer operation to exceed the 64-bit limit for large memory regions.
Quoted 6 lines
If an overflow occurs, it would severely under-account the charged size, breaking the quota enforcement in
damos_quota_is_full()and allowing the scheme to silently bypass its configured action limits.Does this code need bounds checking for the numerator and denominator, or should it use a safer math helper to prevent the quota bypass?
Same sashiko question to the previous patch. My answer is also same. I will keep this as is in favor of simplicity.
Quoted 6 lines
mm/damon/core.c static void damos_walk_cancel(struct damon_ctx *ctx) · line 2062
Quoted 2 lines
mm/damon/core.c +1
-0+}# 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
SeongJae Park
maintainerRFC PATCH 4/11 mm/damon/sysfs-schemes: implement fail_charge_{num,denom} files
In reply to SeongJae ParkImplement 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
@@ -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 , "as->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 , "as->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);
@@ -2780,6 +2832,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
SeongJae Park
maintainerRFC PATCH 5/11 Docs/mm/damon/design: document fail_charge_{num,denom}
In reply to SeongJae ParkUpdate DAMON design document for the DAMOS action failed region quota charge ratio.
--- Documentation/mm/damon/design.rst | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
Documentation/mm/damon/design.rst +22 -0
@@ -576,6 +576,28 @@ 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. The +feature is enabled only if ``fail_charge_denom`` is not zero. + +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
SeongJae Park
maintainerRFC PATCH 6/11 Docs/admin-guide/mm/damon/usage: document fail_charge_{num,denom} files
In reply to SeongJae ParkUpdate 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
@@ -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
SeongJae Park
maintainerRFC PATCH 7/11 Docs/ABI/damon: document fail_charge_{num,denom}
In reply to SeongJae ParkUpdate 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
@@ -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
SeongJae Park
maintainerRFC PATCH 8/11 mm/damon/tests/core-kunit: test fail_charge_{num,denom} committing
In reply to SeongJae ParkExtend 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
@@ -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
SeongJae Park
maintainerRFC PATCH 9/11 selftests/damon/_damon_sysfs: support failed region quota charge ratio
In reply to SeongJae ParkExtend _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
@@ -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
SeongJae Park
maintainerRFC PATCH 10/11 selftests/damon/drgn_dump_damon_status: support failed region quota charge ratio
In reply to SeongJae ParkExtend 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
@@ -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
SeongJae Park
maintainerRFC PATCH 11/11 selftests/damon/sysfs.py: test failed region quota charge ratio
In reply to SeongJae ParkExtend 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
@@ -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)
@@ -278,6 +282,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