Re: [PATCH 6.6.y v3 2/2] mm/damon/core: fix unconditionally skip last region

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mm/damon: dangling charge_target_from stalls schemes on target deletion
- [High] mm/damon: missing charge_target_from reset when target has no regions
--

commit 34b3a9a3bd49604e23a1fa56349694c2bc0db52b
Author: Liew Rui Yan <aethernet65535@gmail.com>

mm/damon/core: fix unconditionally skip last region

This commit modifies the quota charge skipping logic to only reset
charge_target_from and charge_addr_from when the last region is actually
reached. It addresses a minor issue where the last region of a tracked target
could be unconditionally skipped and reset even when it had not been processed
yet.

Link: https://lore.kernel.org/20260908134739.96919-1-sj@kernel.org

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index c86cac855b9b0..c459eae4bbaeb 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -884,36 +884,39 @@ static bool damos_skip_charged_region(struct damon_target *t,
>  {
>  	struct damos_quota *quota = &s->quota;
>  	unsigned long sz_to_skip;
> +	bool skip = false;
>  
>  	/* Skip previously charged regions */
>  	if (quota->charge_target_from) {
>  		if (t != quota->charge_target_from)
>  			return true;

[Severity: High]
Does this logic stall the scheme indefinitely if charge_target_from points to a
destroyed target?

If a target is removed during a sysfs commit, damon_sysfs_set_targets()
dynamically destroys it:

mm/damon/sysfs.c:damon_sysfs_set_targets() {
[ ... ]
		} else {
			if (damon_target_has_pid(ctx))
				put_pid(t->pid);
			damon_destroy_target(t);
		}
[ ... ]
}

However, damon_sysfs_set_schemes() merely updates existing schemes in-place and
fails to clear the quota->charge_target_from pointer if it points to the deleted
target.

Because this saved target was deleted and removed from the active target list,
would this condition evaluate to true for all remaining targets, permanently
skipping all regions and never reaching the logic to clear the dangling pointer?

[Severity: High]
Does this also cause a stall if a target loses all of its regions?

If quota->charge_target_from points to a target that loses all its regions (for
example, if all VMAs are unmapped and the task dies), damon_for_each_region()
will execute zero times for that target:

mm/damon/core.c:kdamond_apply_schemes() {
[ ... ]
	damon_for_each_target(t, c) {
		damon_for_each_region(r, t)
			damon_do_apply_schemes(c, t, r);
	}
[ ... ]
}

Would this bypass the invocation of damos_skip_charged_region() entirely for the
charged target, preventing charge_target_from from being cleared, and causing
all subsequent targets to be skipped indefinitely because they do not match the
uncleared charge_target_from pointer?

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