Re: [PATCH v2 2/3] docs/mm/damon/design: difference between watermarks and nr_snapshots

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but `nr_snapshots` is incorrectly incremented per target instead of per snapshot, and skips incrementing entirely if the scheme bypasses the final region of a target.
--

--- Patch [2]: [PATCH v2 2/3] docs/mm/damon/design: difference between watermarks and nr_snapshots ---
commit af16039fca7bbda3290262311f6015d553f6c65f
Author: Liew Rui Yan <[email protected]>

docs/mm/damon/design: difference between watermarks and nr_snapshots

Explain the difference between nr_snapshots reaches max_nr_snapshots and
watermarks.

> diff --git a/Documentation/mm/damon/design.rst b/Documentation/mm/damon/design.rst
> index 1739aeec6eb95..e7977f005ac06 100644
> --- a/Documentation/mm/damon/design.rst
> +++ b/Documentation/mm/damon/design.rst
> @@ -872,7 +872,8 @@ the action to the region will fail.
>  
>  Unlike normal stats, ``max_nr_snapshots`` is set by users.  If it is set as
>  non-zero and ``nr_snapshots`` be same to or greater than ``nr_snapshots``, the
> -scheme is deactivated.
> +scheme is deactivated.  Note that, unlike watermarks, even if a scheme's
> +``nr_snapshots`` reaches ``max_nr_snapshots``, monitoring will not stop.

[Severity: High]
This isn't a bug introduced by this patch, but while documenting the
nr_snapshots behavior, I noticed an issue in how nr_snapshots is incremented
in the core code.

Looking at damon_do_apply_schemes() in mm/damon/core.c, nr_snapshots is
incremented per target instead of per snapshot:

		if (damos_quota_is_full(quota, c->min_region_sz))
			continue;

		if (damos_skip_charged_region(t, r, s, c->min_region_sz))
			continue;
...
		if (damon_is_last_region(r, t))
			s->stat.nr_snapshots++;

Could this cause the scheme to incorrectly increment multiple times per
snapshot if there are multiple monitoring targets?

Also, if damos_quota_is_full() or damos_skip_charged_region() triggers a
continue on the target's last region, does this bypass the increment entirely?

If a scheme frequently exhausts its quota, it seems nr_snapshots might never
increment, causing the scheme to run forever without reaching
max_nr_snapshots.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2