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

On Wed, 12 Aug 2026 10:47:25 +0000 [email protected] wrote:

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

I don't find a problem of the current behavior.  Let me know if I'm missing
something.  Let's keep it as-is otherwise.  We could add clarifications to
documents if this really confuses humans.

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


Thanks,
SJ