Re: [PATCH 01/11] mm/damon/core: handle <min_region_sz remaining quota as empty

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [High] The patch unmasks an existing off-by-one bug in the DAMOS quota checkpoint logic, which permanently starves memory regions following a quota boundary.
--

commit 6b38ae7237b3de6636c6c420fb6a8b80b4e7eb87
Author: SeongJae Park <sj@kernel.org>

mm/damon/core: handle <min_region_sz remaining quota as empty
    
This commit updates the DAMOS quota logic to treat quotas with less than
min_region_sz remaining as fully charged. This ensures that charge_target_from
and charge_addr_from are updated appropriately to prevent DAMOS actions from
being applied too frequently to specific memory areas.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c

[ ... ]

> @@ -2103,8 +2117,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;

Does this reliably unmask an existing off-by-one bug here?

Since DAMON ranges [start, end) are exclusive at the end, r->ar.end is
exactly the start of the next region. By adding 1, the checkpoint
address advances 1 byte into the next uncharged region.

During the subsequent charge window, damos_skip_charged_region()
evaluates the next region and checks if it falls within the skipped area:

mm/damon/core.c:damos_skip_charged_region() {
    ...
    if (quota->charge_addr_from && r->ar.start <
            quota->charge_addr_from) {
        sz_to_skip = ALIGN_DOWN(quota->charge_addr_from -
                r->ar.start, min_region_sz);
        if (!sz_to_skip) {
            if (damon_sz_region(r) <= min_region_sz)
                return true;
            sz_to_skip = min_region_sz;
        }
    ...
}

Because the checkpoint is 1 byte inside the next region, sz_to_skip
evaluates to ALIGN_DOWN(1, min_region_sz), which becomes 0. The
fallback logic then forcefully skips min_region_sz bytes and returns true
without clearing the checkpoint.

Could this cause the min_region_sz bytes immediately following a quota
boundary to be permanently starved of DAMOS actions?

Should the assignment be changed to quota->charge_addr_from = r->ar.end
instead?

>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260428013402.115171-1-sj@kernel.org?part=1