Re: [PATCH v2 1/3] docs/mm/damon/design: accurate semantics of nr_snapshots

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

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] The commit updates documentation to match buggy code behavior rather than fixing the underlying logical flaws in the `nr_snapshots` counter. `nr_snapshots` incorrectly increments per target instead of per snapshot, and skips entirely when quotas are full.
> --
> 
> --- Patch [1]: [PATCH v2 1/3] docs/mm/damon/design: accurate semantics of nr_snapshots ---
> commit 11b4a9c760b881c063ff5e1eec0248b087d6a31f
> Author: Liew Rui Yan <[email protected]>
> 
> docs/mm/damon/design: accurate semantics of nr_snapshots
> 
> This commit updates the DAMON design documentation and header comments
> to align the description of nr_snapshots with the current behavior
> implemented in the code.
> 
> > diff --git a/Documentation/mm/damon/design.rst b/Documentation/mm/damon/design.rst
> > index aed6cb1cf4831..1739aeec6eb95 100644
> > --- a/Documentation/mm/damon/design.rst
> > +++ b/Documentation/mm/damon/design.rst
> > @@ -846,8 +846,8 @@ scheme's execution.
> [ ... ]
> >  - ``qt_exceeds``: Total number of times the quota of the scheme has exceeded.
> > -- ``nr_snapshots``: Total number of DAMON snapshots that the scheme is tried to
> > -  be applied.
> > +- ``nr_snapshots``: Total number of DAMON snapshots that the scheme is
> > +  completely tried to be applied.
> >  - ``max_nr_snapshots``: Upper limit of ``nr_snapshots``.
> 
> [Severity: High]
> This is a pre-existing issue, but does updating the documentation to match
> the code behavior paper over an underlying logical flaw in the nr_snapshots
> counter?
> 
> Looking at damon_do_apply_schemes() in mm/damon/core.c, the counter updates
> like this:
> 
> mm/damon/core.c:damon_do_apply_schemes() {
>     ...
>         /* Check the quota */
>         if (damos_quota_is_full(quota, c->min_region_sz))
>             continue;
>     ...
>         if (damon_is_last_region(r, t))
>             s->stat.nr_snapshots++;
>     ...
> }
> 
> Because this increments at the last region of a target, if multiple targets
> are present, does the counter incorrectly increment per target instead of
> per snapshot?
> 
> Additionally, if a restrictive quota fills up before the last region of a
> target is processed, the early continue skips the counter increment entirely.
> This could mean a scheme configured with a max_nr_snapshots limit might
> never reach its limit.
> 
> Should the underlying code be fixed to increment once per snapshot regardless
> of quotas and targets, rather than updating the documentation to reflect this
> behavior?

I show no problem at the current behavior.  Let's keep it.  If it really
confuses some real human, we could add clarifications to documents.

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


Thanks,
SJ