mm/damon: fix damos quota walk-position tracking
Subsystems (2)
RFC PATCH 1/1 mm/damon: fix damos quota walk-position tracking
DAMOS uses charge_target_from/charge_addr_from to remember how far a quota-limited walk has progressed. The current implementation has two problems:
1. Once set, the cursor unconditionally skips and resets at the last region of the tracked target, so the last region can be skipped even when it has not been processed.
2. The cursor only advances when the quota becomes full. Regions that are filtered out do not move the cursor, and the scheme can remain stuck on the same regions.
Fix this by treating the fields as a rotating walk cursor:
- Advance the cursor for every region walked while a quota is set, regardless of whether the region is applied or filtered out.
- Remove the unconditional last-region reset.
- Skip regions before the cursor, and rotate to the next valid target only after the current target has been fully walked.
- Rename charge_{target,addr}_from to walk_{target,addr}_from and damos_skip_charged_region() to damos_skip_walked_region().
--- include/linux/damon.h | 6 +- mm/damon/core.c | 156 +++++++++++++++++++++++++++++------------- 2 files changed, 111 insertions(+), 51 deletions(-)
include/linux/damon.h +4 -2
@@ -296,8 +296,10 @@ struct damos_quota {/* For charging the quota */ unsigned long charged_sz;unsigned long charged_from; -struct damon_target *charge_target_from; -unsigned long charge_addr_from; + +/* For tracking the DAMOS walk position (rotating cursor) */ +struct damon_target *walk_target_from; +unsigned long walk_addr_from;/* For prioritization */ unsigned int min_score;
mm/damon/core.c +107 -49
238 lines not on this page · raw message
Signature · 1 line
2.55.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] The DAMOS rotating walk cursor (`walk_target_from`) becomes permanently stuck if it points to a…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The DAMOS rotating walk cursor (walk_target_from) becomes permanently stuck if it points to a target with zero regions, silently disabling the scheme entirely. --
--- Patch [1]: [RFC PATCH] mm/damon: fix damos quota walk-position tracking --- commit f919352b7b1f727f53877f4c9877e9c62ea1cfad
Author: Liew Rui Yan <[email protected]>
mm/damon: fix damos quota walk-position tracking
DAMOS uses charge_target_from/charge_addr_from to remember how far a quota-limited walk has progressed. This commit treats the fields as a rotating walk cursor to resolve issues where regions could be skipped or remain stuck.
Quoted 4 lines
mm/damon/core.c +0
-0
[ ... ]
Quoted 13 lines
mm/damon/core.c +9
-0@@ -404,6 +404,52 @@ static bool damon_is_last_region(struct damon_region *r,return list_is_last (&r->list, &t->regions_list); } +static bool damos_completely_walked (struct damos *s,struct damon_target *t) +{ +/* +* '>=' instead of '==', because the region set of @t could have been +* shrunken by damon_set_regions() after the cursor was saved. +*/ +return s->quota.walk_target_from == t && + s->quota.walk_addr_from >=damon_last_region (t)->ar.end; +}
[ ... ]
Quoted 8 lines
mm/damon/core.c +0
-3@@ -2658,18 +2701,31 @@ static void damon_do_apply_schemes(struct damon_ctx *c,if (damos_quota_is_full (quota, c->min_region_sz))continue ; -if (damos_skip_charged_region (t, r, s, c->min_region_sz)) -continue ; - if (s->max_nr_snapshots &&
mm/damon/core.c static void damon_do_apply_schemes(struct damon_ctx *c, · line 2714
Quoted 10 lines
mm/damon/core.c +7
-0s->max_nr_snapshots <= s->stat.nr_snapshots)continue ; +/* +* Skip regions before the cursor. If the skip makes the cursor +* reach the end of this target, rotate to the next valid target +* before continuing with the next region/scheme. +*/ +if (damos_skip_walked_region (t, r, s, c->min_region_sz)) { +damos_walk_maybe_rotate (c, t, s);
[Severity: High]
Does this code permanently skip all regions if walk_target_from points to a target with zero regions?
Because cursor rotation only happens here inside damon_do_apply_schemes() which is called during region iteration, if walk_target_from is a target without any regions, this code never runs for that target.
When other targets are evaluated, damos_skip_walked_region() will continually return true since walk_target_from != t. However, damos_walk_maybe_rotate() won't advance the cursor because damos_completely_walked() requires walk_target_from == t.
Can we ensure the cursor properly advances past empty targets?
Quoted 16 lines
mm/damon/core.c +9
-0+continue ; + } +if (damos_valid_target (c, r, s))damos_apply_scheme (c, t, r, s);if (damon_is_last_region (r, t)) s->stat.nr_snapshots++; + +if (damos_quota_is_set (quota)) { + quota->walk_target_from = t; + quota->walk_addr_from = r->ar.end; +damos_walk_maybe_rotate (c, t, s); + } } }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
SJ Park
maintainerQuoted 7 lines · On Tue, 25 Aug 2026 20:46:16 +0800 Liew Rui Yan <[email protected]>…
DAMOS uses charge_target_from/charge_addr_from to remember how far a quota-limited walk has progressed. The current implementation has two problems:
1. Once set, the cursor unconditionally skips and resets at the last region of the tracked target, so the last region can be skipped even when it has not been processed.
I don't fully understand this. Could you please clarify more? Maybe adding a realistic example scenario would be helpful.
Quoted 4 lines
2. The cursor only advances when the quota becomes full. Regions that are filtered out do not move the cursor, and the scheme can remain stuck on the same regions.
I don't fully understand this, either. Could you pleae clarify more?
Quoted 10 lines
Fix this by treating the fields as a rotating walk cursor:
- Advance the cursor for every region walked while a quota is set, regardless of whether the region is applied or filtered out.
- Remove the unconditional last-region reset.
- Skip regions before the cursor, and rotate to the next valid target only after the current target has been fully walked.
- Rename charge_{target,addr}_from to walk_{target,addr}_from anddamos_skip_charged_region()todamos_skip_walked_region().
Because I don't understand the problem, I can't understand benefit of this change. Again, more clarification would be helpful.
Quoted 6 lines
Signed-off-by Liew Rui Yan <[email protected]>--- include/linux/damon.h | 6 +- mm/damon/core.c | 156 +++++++++++++++++++++++++++++------------- 2 files changed, 111 insertions(+), 51 deletions(-)
Quite amount of change. I will review the code after the high level understanding is done.
Thanks,
SJ
[...]
Quoted 6 lines · On Tue, 25 Aug 2026 06:54:57 -0700 SJ Park <[email protected]> wrote:
Quoted 4 lines · On Tue, 25 Aug 2026 20:46:16 +0800 Liew Rui Yan <[email protected]>…
DAMOS uses charge_target_from/charge_addr_from to remember how far a quota-limited walk has progressed. The current implementation has two problems:
Quoted 7 lines
Quoted 3 lines
1. Once set, the cursor unconditionally skips and resets at the last region of the tracked target, so the last region can be skipped even when it has not been processed.
I don't fully understand this. Could you please clarify more? Maybe adding a realistic example scenario would be helpful.
Problem: Unconditional skip of the last region
In the current damos_skip_charged_region(), there is this logic:
if (r == damon_last_region(t)) {
quota->charge_target_from = NULL;
quota->charge_addr_from = 0;
return true; /* Skip */
}Scenario:
1. Target has 2 regions: R1 (0-100 bytes) and R2 (100-200 bytes).
2. Quota is configured to process only 50 bytes per window.
3. Window 1: Processes R1 (0-50). Quota is full. Cursor is saved at (Target, 50).
4. Window 2: Skips R1 (0-50). Processes R1 (50-100). Quota is full. Cursor is saved at (Target, 100), which is exactly the start of R2.
5. Window 3: The loop reaches R2. Because R2 is damon_last_region(t), the old code unconditionally returns true, skipping R2 entirely and resetting the cursor.
Result: R2 is permanently skipped even though it has never been processed.
To fix this, the patch advances the cursor every time a region is walked, regardless of whether it is applied or filtered out. This allows DAMON to accurately track whether the last region has already been visited, eliminating the need for the unconditional reset.
Quoted 7 lines
Quoted 4 lines
2. The cursor only advances when the quota becomes full. Regions that are filtered out do not move the cursor, and the scheme can remain stuck on the same regions.
I don't fully understand this, either. Could you pleae clarify more?
Problem 2 is a false positive. As long as the quota is not full, DAMON will continue iterating to find applicable regions. I mistakenly assumed in the commit message that encountering filtered-out or invalid regions would cause the cursor to stall.
This patch ensures that every target is traversed sequentially and deterministically, even when the quota is set very low. I omitted this benefit in the initial problem description. If you think it is okay, I will add it in the next revision.
Best regards,
Rui Yan
SJ Park
maintainerQuoted 37 lines · On Wed, 26 Aug 2026 18:24:13 +0800 Liew Rui Yan <[email protected]>…
Quoted 6 lines · On Tue, 25 Aug 2026 06:54:57 -0700 SJ Park <[email protected]> wrote:
Quoted 4 lines · On Tue, 25 Aug 2026 20:46:16 +0800 Liew Rui Yan <[email protected]>…
DAMOS uses charge_target_from/charge_addr_from to remember how far a quota-limited walk has progressed. The current implementation has two problems:
Quoted 7 lines
Quoted 3 lines
1. Once set, the cursor unconditionally skips and resets at the last region of the tracked target, so the last region can be skipped even when it has not been processed.
I don't fully understand this. Could you please clarify more? Maybe adding a realistic example scenario would be helpful.
Problem: Unconditional skip of the last region
In the current
damos_skip_charged_region(), there is this logic:if (r == damon_last_region(t)) { quota->charge_target_from = NULL; quota->charge_addr_from = 0; return true; /* Skip */ }Scenario:
1. Target has 2 regions: R1 (0-100 bytes) and R2 (100-200 bytes).2. Quota is configured to process only 50 bytes per window.
3. Window 1: Processes R1 (0-50). Quota is full. Cursor is saved at (Target, 50).
4. Window 2: Skips R1 (0-50). Processes R1 (50-100). Quota is full. Cursor is saved at (Target, 100), which is exactly the start of R2.
Quoted 6 lines
5. Window 3: The loop reaches R2. Because R2 is
damon_last_region(t), the old code unconditionally returns true, skipping R2 entirely and resetting the cursor.Result: R2 is permanently skipped even though it has never been processed.
Ok, makes sense. The user impact should be not that big, though.
Quoted 5 lines
To fix this, the patch advances the cursor every time a region is walked, regardless of whether it is applied or filtered out. This allows DAMON to accurately track whether the last region has already been visited, eliminating the need for the unconditional reset.
Sounds like a big change compared to the problem. Why we cannot modify the last region case? Have you also considered other possible simpler approaches?
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
2. The cursor only advances when the quota becomes full. Regions that are filtered out do not move the cursor, and the scheme can remain stuck on the same regions.
Quoted 7 lines
Quoted 2 lines
I don't fully understand this, either. Could you pleae clarify more?
Problem 2 is a false positive. As long as the quota is not full, DAMON will continue iterating to find applicable regions. I mistakenly assumed in the commit message that encountering filtered-out or invalid regions would cause the cursor to stall.
Thank you for clarifying. Please try to reduce this kind of false positives from the next time.
Quoted 5 lines
This patch ensures that every target is traversed sequentially and deterministically, even when the quota is set very low. I omitted this benefit in the initial problem description. If you think it is okay, I will add it in the next revision.
What's the problem and benefit? I still don't get it. More clarification would be nice.
Thanks,
SJ
[...]
SJ Park
maintainerQuoted 49 lines · On Wed, 26 Aug 2026 07:05:08 -0700 SJ Park <[email protected]> wrote:
Quoted 37 lines · On Wed, 26 Aug 2026 18:24:13 +0800 Liew Rui Yan <[email protected]>…
Quoted 6 lines · On Tue, 25 Aug 2026 06:54:57 -0700 SJ Park <[email protected]> wrote:
Quoted 4 lines · On Tue, 25 Aug 2026 20:46:16 +0800 Liew Rui Yan <[email protected]>…
DAMOS uses charge_target_from/charge_addr_from to remember how far a quota-limited walk has progressed. The current implementation has two problems:
Quoted 7 lines
Quoted 3 lines
1. Once set, the cursor unconditionally skips and resets at the last region of the tracked target, so the last region can be skipped even when it has not been processed.
I don't fully understand this. Could you please clarify more? Maybe adding a realistic example scenario would be helpful.
Problem: Unconditional skip of the last region
In the current
damos_skip_charged_region(), there is this logic:if (r == damon_last_region(t)) { quota->charge_target_from = NULL; quota->charge_addr_from = 0; return true; /* Skip */ }Scenario:
1. Target has 2 regions: R1 (0-100 bytes) and R2 (100-200 bytes).2. Quota is configured to process only 50 bytes per window.
3. Window 1: Processes R1 (0-50). Quota is full. Cursor is saved at (Target, 50).
4. Window 2: Skips R1 (0-50). Processes R1 (50-100). Quota is full. Cursor is saved at (Target, 100), which is exactly the start of R2.
Quoted 6 lines
5. Window 3: The loop reaches R2. Because R2 is
damon_last_region(t), the old code unconditionally returns true, skipping R2 entirely and resetting the cursor.Result: R2 is permanently skipped even though it has never been processed.
Ok, makes sense. The user impact should be not that big, though.
Quoted 1 line
Quoted 7 lines
Quoted 4 lines
To fix this, the patch advances the cursor every time a region is walked, regardless of whether it is applied or filtered out. This allows DAMON to accurately track whether the last region has already been visited, eliminating the need for the unconditional reset.
Sounds like a big change compared to the problem. Why we cannot modify the last region case? Have you also considered other possible simpler approaches?
For example,
'''
mm/damon/core.c +7 -6
@@ -2686,14 +2686,15 @@ static bool damos_skip_charged_region(struct damon_target *t,if (quota->charge_target_from) {if (t != quota->charge_target_from)return true; -if (r ==damon_last_region (t)) { - quota->charge_target_from =NULL ; - quota->charge_addr_from =0 ; -return true; - }if (quota->charge_addr_from && - r->ar.end <= quota->charge_addr_from) + r->ar.end <= quota->charge_addr_from) { +if (r->ar.end == quota->charge_addr_from || + r ==damon_last_region (t)) { + quota->charge_target_from =NULL ; + quota->charge_addr_from =0 ; + }return true; + }if (quota->charge_addr_from && r->ar.start < quota->charge_addr_from) {
'''
Quoted 24 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
2. The cursor only advances when the quota becomes full. Regions that are filtered out do not move the cursor, and the scheme can remain stuck on the same regions.
Quoted 7 lines
Quoted 2 lines
I don't fully understand this, either. Could you pleae clarify more?
Problem 2 is a false positive. As long as the quota is not full, DAMON will continue iterating to find applicable regions. I mistakenly assumed in the commit message that encountering filtered-out or invalid regions would cause the cursor to stall.
Thank you for clarifying. Please try to reduce this kind of false positives from the next time.
Quoted 5 lines
This patch ensures that every target is traversed sequentially and deterministically, even when the quota is set very low. I omitted this benefit in the initial problem description. If you think it is okay, I will add it in the next revision.
Quoted 8 lines
What's the problem and benefit? I still don't get it. More clarification would be nice.
Thanks,
SJ[...]
Thanks,
SJ
Quoted 86 lines · On Wed, 26 Aug 2026 17:44:38 -0700 SJ Park <[email protected]> wrote:
Quoted 49 lines · On Wed, 26 Aug 2026 07:05:08 -0700 SJ Park <[email protected]> wrote:
Quoted 37 lines · On Wed, 26 Aug 2026 18:24:13 +0800 Liew Rui Yan <[email protected]>…
Quoted 6 lines · On Tue, 25 Aug 2026 06:54:57 -0700 SJ Park <[email protected]> wrote:
Quoted 4 lines · On Tue, 25 Aug 2026 20:46:16 +0800 Liew Rui Yan <[email protected]>…
DAMOS uses charge_target_from/charge_addr_from to remember how far a quota-limited walk has progressed. The current implementation has two problems:
Quoted 7 lines
Quoted 3 lines
1. Once set, the cursor unconditionally skips and resets at the last region of the tracked target, so the last region can be skipped even when it has not been processed.
I don't fully understand this. Could you please clarify more? Maybe adding a realistic example scenario would be helpful.
Problem: Unconditional skip of the last region
In the current
damos_skip_charged_region(), there is this logic:if (r == damon_last_region(t)) { quota->charge_target_from = NULL; quota->charge_addr_from = 0; return true; /* Skip */ }Scenario:
1. Target has 2 regions: R1 (0-100 bytes) and R2 (100-200 bytes).2. Quota is configured to process only 50 bytes per window.
3. Window 1: Processes R1 (0-50). Quota is full. Cursor is saved at (Target, 50).
4. Window 2: Skips R1 (0-50). Processes R1 (50-100). Quota is full. Cursor is saved at (Target, 100), which is exactly the start of R2.
Quoted 6 lines
5. Window 3: The loop reaches R2. Because R2 is
damon_last_region(t), the old code unconditionally returns true, skipping R2 entirely and resetting the cursor.Result: R2 is permanently skipped even though it has never been processed.
Ok, makes sense. The user impact should be not that big, though.
Quoted 1 line
Quoted 7 lines
Quoted 4 lines
To fix this, the patch advances the cursor every time a region is walked, regardless of whether it is applied or filtered out. This allows DAMON to accurately track whether the last region has already been visited, eliminating the need for the unconditional reset.
Sounds like a big change compared to the problem. Why we cannot modify the last region case? Have you also considered other possible simpler approaches?
For example,
'''
mm/damon/core.c +7
-6@@ -2686,14 +2686,15 @@ static bool damos_skip_charged_region(struct damon_target *t,if (quota->charge_target_from) {if (t != quota->charge_target_from)return true; -if (r ==damon_last_region (t)) { - quota->charge_target_from =NULL ; - quota->charge_addr_from =0 ; -return true; - }if (quota->charge_addr_from && - r->ar.end <= quota->charge_addr_from) + r->ar.end <= quota->charge_addr_from) { +if (r->ar.end == quota->charge_addr_from || + r ==damon_last_region (t)) { + quota->charge_target_from =NULL ; + quota->charge_addr_from =0 ; + }return true; + }if (quota->charge_addr_from && r->ar.start < quota->charge_addr_from) {
Quoted 2 lines
'''
Thank you for the example!
While your approach works, I am curious, why should the cursor be reset every time the function returns false (does not skip)?
In my opinion, a cleaner and more deterministic approach is to reset the cursor only after the target has been fully iterated through. This separates "skip" logic from the "state reset" logic, making the flow easier to reason about.
Here is my proposed minimal change:
'''
mm/damon/core.c +12 -11
@@ -2347,11 +2347,6 @@ static bool damos_skip_charged_region(struct damon_target *t,if (quota->charge_target_from) {if (t != quota->charge_target_from)return true; -if (r ==damon_last_region (t)) { - quota->charge_target_from =NULL ; - quota->charge_addr_from =0 ; -return true; - }if (quota->charge_addr_from && r->ar.end <= quota->charge_addr_from)return true;
@@ -2368,8 +2363,6 @@ static bool damos_skip_charged_region(struct damon_target *t,damon_split_region_at (t, r, sz_to_skip);return true; } - quota->charge_target_from =NULL ; - quota->charge_addr_from =0 ; }return false; }
@@ -2658,18 +2651,26 @@ static void damon_do_apply_schemes(struct damon_ctx *c,if (damos_quota_is_full (quota, c->min_region_sz))continue ; -if (damos_skip_charged_region (t, r, s, c->min_region_sz)) -continue ; -if (s->max_nr_snapshots && s->max_nr_snapshots <= s->stat.nr_snapshots)continue ; +if (damos_skip_charged_region (t, r, s, c->min_region_sz)) { +if (damon_is_last_region (r, t)) { + quota->charge_target_from =NULL ; + quota->charge_addr_from =0 ; + } +continue ; + } +if (damos_valid_target (c, r, s))damos_apply_scheme (c, t, r, s); -if (damon_is_last_region (r, t)) +if (damon_is_last_region (r, t)) { s->stat.nr_snapshots++; + quota->charge_target_from =NULL ; + quota->charge_addr_from =0 ; + } } }
'''
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
This patch ensures that every target is traversed sequentially and deterministically, even when the quota is set very low. I omitted this benefit in the initial problem description. If you think it is okay, I will add it in the next revision.
What's the problem and benefit? I still don't get it. More clarification would be nice.
My original idea was to ensure that every target would be checked sequentially, which seemed like a fairer approach. However, upon further reflection, I realize this might not offer tangible benefits and could introduce unnecessary complexity.
Since the DAMOS Quota min_score mechanism already ensures that regions truly needing action are prioritized, the current behavior (eventually resetting at the last region and moving on) is functionally sufficient for typical workloads. Therefore, I do not see a strong justification for this change at this stage.
Thank you for pointing this out and pushing me to clarify. In the next revision, I will drop this changes and focus on the minimal fix for Problem 1.
Best regards,
Rui Yan
SJ Park
maintainerQuoted 93 lines · On Fri, 28 Aug 2026 02:08:22 +0800 Liew Rui Yan <[email protected]>…
Quoted 86 lines · On Wed, 26 Aug 2026 17:44:38 -0700 SJ Park <[email protected]> wrote:
Quoted 49 lines · On Wed, 26 Aug 2026 07:05:08 -0700 SJ Park <[email protected]> wrote:
Quoted 37 lines · On Wed, 26 Aug 2026 18:24:13 +0800 Liew Rui Yan <[email protected]>…
Quoted 6 lines · On Tue, 25 Aug 2026 06:54:57 -0700 SJ Park <[email protected]> wrote:
Quoted 4 lines · On Tue, 25 Aug 2026 20:46:16 +0800 Liew Rui Yan <[email protected]>…
DAMOS uses charge_target_from/charge_addr_from to remember how far a quota-limited walk has progressed. The current implementation has two problems:
Quoted 7 lines
Quoted 3 lines
1. Once set, the cursor unconditionally skips and resets at the last region of the tracked target, so the last region can be skipped even when it has not been processed.
I don't fully understand this. Could you please clarify more? Maybe adding a realistic example scenario would be helpful.
Problem: Unconditional skip of the last region
In the current
damos_skip_charged_region(), there is this logic:if (r == damon_last_region(t)) { quota->charge_target_from = NULL; quota->charge_addr_from = 0; return true; /* Skip */ }Scenario:
1. Target has 2 regions: R1 (0-100 bytes) and R2 (100-200 bytes).2. Quota is configured to process only 50 bytes per window.
3. Window 1: Processes R1 (0-50). Quota is full. Cursor is saved at (Target, 50).
4. Window 2: Skips R1 (0-50). Processes R1 (50-100). Quota is full. Cursor is saved at (Target, 100), which is exactly the start of R2.
Quoted 6 lines
5. Window 3: The loop reaches R2. Because R2 is
damon_last_region(t), the old code unconditionally returns true, skipping R2 entirely and resetting the cursor.Result: R2 is permanently skipped even though it has never been processed.
Ok, makes sense. The user impact should be not that big, though.
Quoted 1 line
Quoted 7 lines
Quoted 4 lines
To fix this, the patch advances the cursor every time a region is walked, regardless of whether it is applied or filtered out. This allows DAMON to accurately track whether the last region has already been visited, eliminating the need for the unconditional reset.
Sounds like a big change compared to the problem. Why we cannot modify the last region case? Have you also considered other possible simpler approaches?
For example,
'''
mm/damon/core.c +7
-6@@ -2686,14 +2686,15 @@ static bool damos_skip_charged_region(struct damon_target *t,if (quota->charge_target_from) {if (t != quota->charge_target_from)return true; -if (r ==damon_last_region (t)) { - quota->charge_target_from =NULL ; - quota->charge_addr_from =0 ; -return true; - }if (quota->charge_addr_from && - r->ar.end <= quota->charge_addr_from) + r->ar.end <= quota->charge_addr_from) { +if (r->ar.end == quota->charge_addr_from || + r ==damon_last_region (t)) { + quota->charge_target_from =NULL ; + quota->charge_addr_from =0 ; + }return true; + }if (quota->charge_addr_from && r->ar.start < quota->charge_addr_from) {Quoted 2 lines
'''
Thank you for the example!
Quoted 2 lines
While your approach works, I am curious, why should the cursor be reset every time the function returns false (does not skip)?
It doesn't. It resets charge_{target,addr}_from only once after the regions to skip are all skipped. Am I missing something?
Quoted 3 lines
In my opinion, a cleaner and more deterministic approach is to reset the cursor only after the target has been fully iterated through.
So I will be able to give you some comments only after the above question is answered.
Thanks,
SJ
[...]
Quoted 95 lines · On Thu, 27 Aug 2026 17:40:48 -0700 SJ Park <[email protected]> wrote:
Quoted 93 lines · On Fri, 28 Aug 2026 02:08:22 +0800 Liew Rui Yan <[email protected]>…
Quoted 86 lines · On Wed, 26 Aug 2026 17:44:38 -0700 SJ Park <[email protected]> wrote:
Quoted 49 lines · On Wed, 26 Aug 2026 07:05:08 -0700 SJ Park <[email protected]> wrote:
Quoted 37 lines · On Wed, 26 Aug 2026 18:24:13 +0800 Liew Rui Yan <[email protected]>…
Quoted 6 lines · On Tue, 25 Aug 2026 06:54:57 -0700 SJ Park <[email protected]> wrote:
Quoted 4 lines · On Tue, 25 Aug 2026 20:46:16 +0800 Liew Rui Yan <[email protected]>…
DAMOS uses charge_target_from/charge_addr_from to remember how far a quota-limited walk has progressed. The current implementation has two problems:
Quoted 7 lines
Quoted 3 lines
1. Once set, the cursor unconditionally skips and resets at the last region of the tracked target, so the last region can be skipped even when it has not been processed.
I don't fully understand this. Could you please clarify more? Maybe adding a realistic example scenario would be helpful.
Problem: Unconditional skip of the last region
In the current
damos_skip_charged_region(), there is this logic:if (r == damon_last_region(t)) { quota->charge_target_from = NULL; quota->charge_addr_from = 0; return true; /* Skip */ }Scenario:
1. Target has 2 regions: R1 (0-100 bytes) and R2 (100-200 bytes).2. Quota is configured to process only 50 bytes per window.
3. Window 1: Processes R1 (0-50). Quota is full. Cursor is saved at (Target, 50).
4. Window 2: Skips R1 (0-50). Processes R1 (50-100). Quota is full. Cursor is saved at (Target, 100), which is exactly the start of R2.
Quoted 6 lines
5. Window 3: The loop reaches R2. Because R2 is
damon_last_region(t), the old code unconditionally returns true, skipping R2 entirely and resetting the cursor.Result: R2 is permanently skipped even though it has never been processed.
Ok, makes sense. The user impact should be not that big, though.
Quoted 1 line
Quoted 7 lines
Quoted 4 lines
To fix this, the patch advances the cursor every time a region is walked, regardless of whether it is applied or filtered out. This allows DAMON to accurately track whether the last region has already been visited, eliminating the need for the unconditional reset.
Sounds like a big change compared to the problem. Why we cannot modify the last region case? Have you also considered other possible simpler approaches?
For example,
'''
mm/damon/core.c +7
-6@@ -2686,14 +2686,15 @@ static bool damos_skip_charged_region(struct damon_target *t,if (quota->charge_target_from) {if (t != quota->charge_target_from)return true; -if (r ==damon_last_region (t)) { - quota->charge_target_from =NULL ; - quota->charge_addr_from =0 ; -return true; - }if (quota->charge_addr_from && - r->ar.end <= quota->charge_addr_from) + r->ar.end <= quota->charge_addr_from) { +if (r->ar.end == quota->charge_addr_from || + r ==damon_last_region (t)) { + quota->charge_target_from =NULL ; + quota->charge_addr_from =0 ; + }return true; + }if (quota->charge_addr_from && r->ar.start < quota->charge_addr_from) {Quoted 2 lines
'''
Thank you for the example!
Quoted 5 lines
Quoted 2 lines
While your approach works, I am curious, why should the cursor be reset every time the function returns false (does not skip)?
It doesn't. It resets charge_{target,addr}_from only once after the regions to skip are all skipped. Am I missing something?
You are right.
My concern was that the current
'return false == reset charge_{target, addr}_from' might be a bit hard to understand. However, I realize that my change was quite significant.
To make the existing logic clearer for future readers, I think adding a brief comment would be helpful. For example:
'''
mm/damon/core.c +9 -0
@@ -2368,6 +2368,15 @@ static bool damos_skip_charged_region(struct damon_target *t,damon_split_region_at (t, r, sz_to_skip);return true; } +/* +* Reset the charge_{target,addr}_from so that the remaining +* regions in this/next target can be processed normally. If +* the quota becomes full later during the walk, +* damos_apply_scheme() will update the +* charge_{target,addr}_from to the correct position. +* Otherwise, it implies that all applicable regions in this +* target have been processed. +*/ quota->charge_target_from =NULL ; quota->charge_addr_from =0 ; }
'''
If this is not necessary or redundant, I am perfectly fine with dropping it and just applying your minimal fix for the last-region issue in the next revision.
Best regards,
Rui Yan
SJ Park
maintainerQuoted 3 lines · On Fri, 28 Aug 2026 09:54:29 +0800 Liew Rui Yan <[email protected]>…
Quoted 1 line · On Thu, 27 Aug 2026 17:40:48 -0700 SJ Park <[email protected]> wrote:
On Fri, 28 Aug 2026 02:08:22 +0800 Liew Rui Yan <[email protected]> wrote:
[...]
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
While your approach works, I am curious, why should the cursor be reset every time the function returns false (does not skip)?
It doesn't. It resets charge_{target,addr}_from only once after the regions to skip are all skipped. Am I missing something?
You are right.
Thank you for confirming.
Quoted 7 lines
My concern was that the current
'return false == reset charge_{target, addr}_from' might be a bit hard to understand. However, I realize that my change was quite significant.To make the existing logic clearer for future readers, I think adding a brief comment would be helpful.
Makes sense to me.
Quoted 23 lines
For example:
'''
mm/damon/core.c +9
-0@@ -2368,6 +2368,15 @@ static bool damos_skip_charged_region(struct damon_target *t,damon_split_region_at (t, r, sz_to_skip);return true; } +/* +* Reset the charge_{target,addr}_from so that the remaining +* regions in this/next target can be processed normally. If +* the quota becomes full later during the walk, +* damos_apply_scheme() will update the +* charge_{target,addr}_from to the correct position. +* Otherwise, it implies that all applicable regions in this +* target have been processed. +*/ quota->charge_target_from =NULL ; quota->charge_addr_from =0 ; }'''
Quoted 3 lines
If this is not necessary or redundant, I am perfectly fine with dropping it and just applying your minimal fix for the last-region issue in the next revision.
I'd suggest to separate the minimal fix and readability improvement. Could you please post the minimal fix first, with Cc: stable@? After it is done, we could revisit this readability improvement.
Thanks,
SJ
[...]