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

[SeongJae Park <sj@kernel.org>]

On Tue, 28 Apr 2026 05:49:03 +0000 sashiko-bot@kernel.org 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.

> --
> 
> commit 50bbd8d578080e27beac4fe1ad0cecbb013a8fd7
> Author: SeongJae Park <sj@kernel.org>
> 
> 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.
> 
> Link: https://lore.kernel.org/20260428032324.115663-1-sj@kernel.org
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 3dbbbfdeff71..901ffdaefb7f 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -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.

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.

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

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

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

I will separtely work on this.

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

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/20260428013402.115171-1-sj@kernel.org


Thanks,
SJ

[...]