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

[SeongJae Park <sj@kernel.org>]

On Tue, 28 Apr 2026 02:00:25 +0000 sashiko-bot@kernel.org wrote:

> 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.

Not a blocker for this patch.  Please read below for details.

> --
> 
> 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?

Good finding.  But the user impact is quite small, and anyway this bug is not
introduced by this patch.  I will separately work on fixing the bug.  But this
cannot be a blocker of this patch.


Thanks,
SJ

[...]