Skip to contentSkip to conversation

mm/damon/core: make charge_addr_from aware of end-address exclusivity

PATCH Applied SeongJae Park 2 participants 3 messages Apr 28

Subsystems (3)
lore
Share & tools
lore

SeongJae Park

author maintainer
#0 lore raw

PATCH 1/1 mm/damon/core: make charge_addr_from aware of end-address exclusivity

DAMON region end address is exclusive one, but charge_addr_from is assigned assuming the end address is inclusive. As a result, DAMOS action to next up to min_region_sz memory can be skipped. This is quite negligible user impact. But, the bug is a bug that can be very simply fixed. Fix the wrong assignment to respect the exclusiveness of the address.

The issue was discovered [1] by Sashiko.

[1] https://lore.kernel.org/[email protected]

Fixes 50585192bc2e ("mm/damon/schemes: skip already charged targets and regions")
Cc <[email protected]> # 5.16.x
---
 mm/damon/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/damon/core.c +1 -1
index 278594847cf94..37c9a40d0577b 100644
@@ -2107,7 +2107,7 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
 		if (damos_quota_is_set(quota) &&
 				quota->charged_sz >= quota->esz) {
 			quota->charge_target_from = t;
-			quota->charge_addr_from = r->ar.end + 1;
+			quota->charge_addr_from = r->ar.end;
 		}
 	}
 	if (s->action != DAMOS_STAT)

base-commit: 986c714ac6faa9750e15ccaec72ff2823c96a5c6

Signature · 1 line
2.47.3
#1 lore raw
Sashiko AI review found 3 potential issue(s): - [High] The remainder of the last region of a target is unconditionally skipped during quota resumption. - [High] Quota state is not saved when the…

Sashiko AI review found 3 potential issue(s):
- [High] The remainder of the last region of a target is unconditionally skipped during quota resumption.
- [High] Quota state is not saved when the remaining quota is less than min_region_sz, breaking round-robin fairness.
- [Medium] Setting charge_addr_from = r->ar.end causes resumption failure when a region covers the end of the address space. --

commit 50bbd8d578080e27beac4fe1ad0cecbb013a8fd7
Author: SeongJae Park <[email protected]>

mm/damon/core: make charge_addr_from aware of end-address exclusivity

This commit updates the DAMOS quota charge assignment to use the exclusive region end address rather than assuming it is inclusive. This addresses a bug where the action might skip the next memory block up to min_region_sz.

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

Quoted 10 lines
mm/damon/core.c +1 -1
index 3dbbbfdeff71..901ffdaefb7f 100644
@@ -2106,7 +2106,7 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
 		if (damos_quota_is_set(quota) &&
 				quota->charged_sz >= quota->esz) {
 			quota->charge_target_from = t;
-			quota->charge_addr_from = r->ar.end + 1;
+			quota->charge_addr_from = r->ar.end;

Does this cause a resumption failure if the region covers the absolute end of the address space?

If r->ar.end wraps to 0 (e.g., ULONG_MAX + 1), charge_addr_from is assigned
0. Since DAMON uses 0 as a sentinel value for an uninitialized state, the checks for quota->charge_addr_from in damos_skip_charged_region() will evaluate to false.

Will this completely fail to identify the resumption point, prematurely clear the state, and restart from the first region instead of resuming?

Additionally, this isn't a bug introduced by this patch, but does the resuming logic unconditionally skip the remainder of the last region of a target?

In damos_skip_charged_region(), the damon_last_region() check executes before evaluating charge_addr_from:

mm/damon/core.c:damos_skip_charged_region() {
    ...
	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 a target's last region is partially charged and split during a window, the uncharged remainder becomes the new damon_last_region. In the next quota window, when the loop reaches this remainder, will it immediately match the last region check, clear the state, and permanently starve the end of targets by returning true (skip)?

Finally, this isn't a bug introduced by this patch, but is the quota state lost when the remaining quota is less than min_region_sz?

Earlier in damos_apply_scheme(), if the remaining quota is small:

mm/damon/core.c:damos_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;
    ...
}

Does the goto update_stat bypass incrementing quota->charged_sz and saving charge_target_from and charge_addr_from?

Because charged_sz never reaches esz, the loop will uselessly evaluate all remaining regions. Because the resume state is never saved, will the next interval incorrectly restart from the very first target and region, breaking round-robin fairness and starving later regions whenever the dynamically calculated quota is not perfectly aligned?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
author maintainer
#2 lore raw
Quoted 4 lines · On Tue, 28 Apr 2026 05:49:03 +0000 [email protected] wrote:

Sashiko AI review found 3 potential issue(s):
- [High] The remainder of the last region of a target is unconditionally skipped during quota resumption.
- [High] Quota state is not saved when the remaining quota is less than min_region_sz, breaking round-robin fairness.
- [Medium] Setting charge_addr_from = r->ar.end causes resumption failure when a region covers the end of the address space.

No blocker of this patch. Please read below for details.

Quoted 27 lines

--

commit 50bbd8d578080e27beac4fe1ad0cecbb013a8fd7
Author: SeongJae Park <[email protected]>

mm/damon/core: make charge_addr_from aware of end-address exclusivity

This commit updates the DAMOS quota charge assignment to use the exclusive region end address rather than assuming it is inclusive. This addresses a bug where the action might skip the next memory block up to min_region_sz.

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

Quoted 10 lines
mm/damon/core.c +1 -1
index 3dbbbfdeff71..901ffdaefb7f 100644
@@ -2106,7 +2106,7 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
 		if (damos_quota_is_set(quota) &&
 				quota->charged_sz >= quota->esz) {
 			quota->charge_target_from = t;
-			quota->charge_addr_from = r->ar.end + 1;
+			quota->charge_addr_from = r->ar.end;

Does this cause a resumption failure if the region covers the absolute end of the address space?

Quoted 4 lines

If r->ar.end wraps to 0 (e.g., ULONG_MAX + 1), charge_addr_from is assigned
0. Since DAMON uses 0 as a sentinel value for an uninitialized state, the checks for quota->charge_addr_from in damos_skip_charged_region() will evaluate to false.

There is no execution path that sets r->ar.end as the absolute end of the address space, to my best knowledge. If there is a path to make it happen, it is already an issue.

Quoted 3 lines

Will this completely fail to identify the resumption point, prematurely clear the state, and restart from the first region instead of resuming?

So this cannot happen to my best knowledge.

Quoted 21 lines

Additionally, this isn't a bug introduced by this patch, but does the resuming logic unconditionally skip the remainder of the last region of a target?

In damos_skip_charged_region(), the damon_last_region() check executes before evaluating charge_addr_from:

mm/damon/core.c:damos_skip_charged_region() {
    ...
	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;
		}
        ...
}
Quoted 5 lines

If a target's last region is partially charged and split during a window, the uncharged remainder becomes the new damon_last_region. In the next quota window, when the loop reaches this remainder, will it immediately match the last region check, clear the state, and permanently starve the end of targets by returning true (skip)?

Good finding. The user impact is quite negligible and anyway this is not introduced by this patch.

I will separtely work on this.

Quoted 17 lines

Finally, this isn't a bug introduced by this patch, but is the quota state lost when the remaining quota is less than min_region_sz?

Earlier in damos_apply_scheme(), if the remaining quota is small:

mm/damon/core.c:damos_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 8 lines

Does the goto update_stat bypass incrementing quota->charged_sz and saving charge_target_from and charge_addr_from?

Because charged_sz never reaches esz, the loop will uselessly evaluate all remaining regions. Because the resume state is never saved, will the next interval incorrectly restart from the very first target and region, breaking round-robin fairness and starving later regions whenever the dynamically calculated quota is not perfectly aligned?

Quota is charged in min_region_sz granularity, so the problem shouldn't happen.

FYI, this scenario can happen once failed region charge ratio feature is applied. Hence the failed region charge ratio patch series [1] contains a fix of this.

So, the issue is unreal now, and neither will be real in future.

[1] https://lore.kernel.org/[email protected]

Thanks,
SJ

[...]