* Re: [PATCH 6.1.y] mm/damon/core: make charge_addr_from aware of end-address exclusivity
2026-07-21 4:58 ` [PATCH 6.1.y] mm/damon/core: make charge_addr_from aware of end-address exclusivity SJ Park
@ 2026-07-21 5:08 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-07-21 5:08 UTC (permalink / raw)
To: SJ Park; +Cc: damon
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [High] Modifying the local region pointer `r` inside the `damon_for_each_scheme` loop causes subsequent schemes to miss the first half of a split region.
- [High] Regions created by `damon_split_region_at()` are silently skipped by the caller's safe list iterator.
- [High] Failure to set `charge_target_from` when the remaining quota is less than `DAMON_MIN_REGION` breaks the round-robin mechanism and wastes CPU.
- [Medium] The scheme's resume logic unconditionally skips the uncharged remainder of a target's last region when resetting the charge state.
--
--- Patch [1]: [PATCH 6.1.y] mm/damon/core: make charge_addr_from aware of end-address exclusivity ---
commit 5e517614c6d1a61febddeb99e7bb671793abbca6
Author: SeongJae Park <sj@kernel.org>
Subject: mm/damon/core: make charge_addr_from aware of end-address exclusivity
This commit fixes a bug where charge_addr_from was assigned assuming the
end address is inclusive. It changes the assignment to respect the
exclusiveness of the DAMON region end address.
Link: https://lore.kernel.org/20260428042942.118230-1-sj@kernel.org
Link: https://lore.kernel.org/20260428032324.115663-1-sj@kernel.org [1]
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index d293200d824e9..4b434ebd37c53 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
[ ... ]
> @@ -803,7 +803,7 @@ static void damon_do_apply_schemes(struct damon_ctx *c,
> quota->charged_sz += sz;
> if (quota->esz && 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)
[Severity: High]
This is a pre-existing issue, but does the safe iterator in
kdamond_apply_schemes() skip newly inserted regions?
mm/damon/core.c:kdamond_apply_schemes() {
...
damon_for_each_region_safe(r, next_r, t) {
...
damon_do_apply_schemes(c, t, r);
}
}
If damon_do_apply_schemes() calls damon_split_region_at(), it inserts a new
region immediately after r. Because next_r is cached before the loop body,
the newly inserted right-hand region falls into a blind spot and appears to
be skipped for the current aggregation interval.
[Severity: High]
This is a pre-existing issue, but is it possible that reassigning the local
region pointer causes subsequent schemes to miss the first half of a split
region?
mm/damon/core.c:damon_do_apply_schemes() {
...
damon_for_each_scheme(s, c) {
...
if (quota->charge_addr_from && r->ar.start < quota->charge_addr_from) {
...
damon_split_region_at(t, r, sz);
r = damon_next_region(r);
sz = damon_sz_region(r);
}
...
}
}
Modifying the loop parameter r leaks to subsequent schemes in the iteration,
causing them to evaluate only the second half of the split region and
completely miss the first half.
[Severity: Medium]
This is a pre-existing issue, but could this logic unconditionally skip the
uncharged remainder of a target's last region?
mm/damon/core.c:damon_do_apply_schemes() {
...
if (r == damon_last_region(t)) {
quota->charge_target_from = NULL;
quota->charge_addr_from = 0;
continue;
}
...
}
When resuming from a previous charge window that exhausted its quota in the
middle of the last region of a target, it looks like this clears the charge
tracking and immediately executes continue, skipping the evaluation of the rest
of the region.
[Severity: High]
This is a pre-existing issue, but can this jump break the round-robin
mechanism and waste CPU when the remaining quota is less than
DAMON_MIN_REGION?
mm/damon/core.c:damon_do_apply_schemes() {
...
if (quota->esz && quota->charged_sz + sz > quota->esz) {
sz = ALIGN_DOWN(quota->esz - quota->charged_sz, DAMON_MIN_REGION);
if (!sz)
goto update_stat;
damon_split_region_at(t, r, sz);
}
...
}
When the remaining quota is less than DAMON_MIN_REGION, ALIGN_DOWN results in
sz == 0. The code then jumps to update_stat, bypassing the charged_sz update.
This prevents charged_sz from reaching esz, creating a pseudo-infinite
evaluation loop for the rest of the regions without saving the resume state.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260721045818.157773-1-sj@kernel.org?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread