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

[sashiko-bot@kernel.org]

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.

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.

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?

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