Re: [RFC PATCH 1/6] mm/damon/core: handle invalid target in damos_fix_stale_charge_pointers()

[SJ Park <sj@kernel.org>]

On Sat, 31 Oct 2026 20:36:25 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] mm/damon/core: stale charge_target_from and skip regression on invalid target
> --
> 
> commit 3b1cac25b68197d5c2819d4a16dbde02446b2d07
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/core: handle invalid target in damos_fix_stale_charge_pointers()
> 
> This commit moves the DAMOS quota charge pointer fixup for invalid targets
> from kdamond_apply_schemes() into damos_fix_stale_charge_pointers().
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 73b571b429cb3..ce75db510a9d3 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -3394,7 +3394,11 @@ static void damos_fix_stale_charge_pointers(struct damon_ctx *c,
> >  		return;
> >  	damon_for_each_target(t, c) {
> >  		if (t == quota->charge_target_from) {
> > -			if (damon_nr_regions(t) > 0)
> > +			bool valid_target = true;
> > +
> > +			if (c->ops.target_valid && !c->ops.target_valid(t))
> > +				valid_target = false;
> > +			if (valid_target && damon_nr_regions(t) > 0)
> >  				return;
> >  			break;
> >  		}
> > @@ -3513,15 +3517,8 @@ static noinline_for_stack void kdamond_apply_schemes(struct damon_ctx *c)
> >  	max_region_sz = damon_region_sz_limit(c);
> >  	mutex_lock(&c->walk_control_lock);
> >  	damon_for_each_target(t, c) {
> > -		if (c->ops.target_valid && c->ops.target_valid(t) == false) {
> > -			damon_for_each_scheme(s, c) {
> > -				if (s->quota.charge_target_from != t)
> > -					continue;
> > -				s->quota.charge_target_from = NULL;
> > -				s->quota.charge_addr_from = 0;
> > -			}
> > +		if (c->ops.target_valid && c->ops.target_valid(t) == false)
> >  			continue;
> > -		}
> 
> [Severity: Medium]
> Can moving the invalid-target reset from kdamond_apply_schemes() into
> damos_fix_stale_charge_pointers() cause stale quota->charge_target_from
> pointers and alter round-robin target skipping across multiple targets?
> 
> First, in kdamond_apply_schemes(), damos_adjust_quota() (which calls
> damos_fix_stale_charge_pointers()) runs for all schemes before the
> damon_for_each_target() application loop.  For virtual address space
> monitoring, c->ops.target_valid(t) is damon_va_target_valid(), which
> checks whether the target PID still has a live task_struct.
> 
> If the target referenced by s->quota.charge_target_from is valid when
> damos_fix_stale_charge_pointers() checks it, but the process exits while
> damos_adjust_quota() builds the score histogram or while
> damos_apply_target() applies schemes to earlier targets, the target loop
> in kdamond_apply_schemes() will see c->ops.target_valid(t) == false and
> skip t without clearing s->quota.charge_target_from.  For all remaining
> targets after t in c->adaptive_targets, damon_do_apply_schemes() calls
> damos_skip_charged_region():
> 
> mm/damon/core.c:damos_skip_charged_region() {
> 	if (quota->charge_target_from) {
> 		if (t != quota->charge_target_from)
> 			return true;
> ...
> }
> 
> which will skip every subsequent target for that apply interval.

This race can happen even in the current code.  That is, the target could be
terminated between the current check and damos_apply_target() call.  The race
can happen more easily, but I don't think this is a regression.

> 
> Second, damos_adjust_quota() returns early before calling
> damos_fix_stale_charge_pointers() whenever damos_quota_is_set(quota) is
> false:
> 
> mm/damon/core.c:damos_adjust_quota() {
> 	if (!damos_quota_is_set(quota))
> 		return;
> 	...
> 	damos_fix_stale_charge_pointers(c, s);
> }
> 
> whereas damon_do_apply_schemes() calls damos_skip_charged_region()
> unconditionally.  If damos_quota_is_set(quota) becomes false while
> quota->charge_target_from is still set to a target t (for example, when a
> quota is disabled or its goal is removed via online commit in
> damos_commit_quota(), which preserves quota->charge_target_from, and
> quota->esz becomes 0), and t exits before kdamond_apply_schemes() reaches
> t, neither damos_adjust_quota() nor kdamond_apply_schemes() will ever
> clear quota->charge_target_from, causing damos_skip_charged_region() to
> skip all other targets permanently.

Good finding.  I will drop this patch from the next version of this series for
now.

> 
> Finally, when monitoring multiple targets [t0, t1, t2] where the previous
> quota window stopped at charge_target_from = t1 and t1 has since exited,
> kdamond_apply_schemes() previously skipped t0 via
> damos_skip_charged_region(), cleared charge_target_from upon reaching t1,
> and resumed applying the scheme at t2.  With charge_target_from cleared
> upfront in damos_fix_stale_charge_pointers() before the target loop
> starts, won't kdamond_apply_schemes() restart charging from t0 instead of
> advancing to t2?

Again, good finding.  This is only a corner case that happen only for one
cycle.  I think it is a reasonable tradeoff for code simplicity.  But since I
decided to drop this patch for now, I will think about this again, too.

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


Thanks,
SJ