Re: [PATCH 6.1.y] mm/damon/core: make charge_addr_from aware of end-address exclusivity

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 <[email protected]>
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/[email protected]
Link: https://lore.kernel.org/[email protected] [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/[email protected]?part=1