Re: [RFC PATCH v1.2 03/17] mm/damon/core: always update ->last_nr_accesses for intervals change

[SeongJae Park <sj@kernel.org>]

On Sun, 21 Jun 2026 16:11:04 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Arithmetic overflow in DAMON access scaling functions when interval ratio is large.
> - [High] The patch attempts to fix a 'wrong value' returned by `damon_nr_accesses_mvsum()`, but misdiagnoses the root cause and leaves a major double-counting bug unresolved.
> --
> 
> commit 6636564dd42488eb260d7102569887a00bdd3d71
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/core: always update ->last_nr_accesses for intervals change
> 
> This commit modifies damon_update_monitoring_result() to unconditionally
> update last_nr_accesses when aggregation intervals change. It aims to
> prevent incorrect calculations in damon_nr_accesses_mvsum() that could occur
> if called with a stale last_nr_accesses value alongside a fresh aggregation
> timestamp.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index addd14e4e70bf..bc40a6d10f277 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -873,6 +873,8 @@ static void damon_update_monitoring_result(struct damon_region *r,
> >  		struct damon_attrs *old_attrs, struct damon_attrs *new_attrs,
> >  		bool aggregating)
> >  {
> > +	r->last_nr_accesses = damon_nr_accesses_for_new_attrs(
> > +			r->last_nr_accesses, old_attrs, new_attrs);
> 
> [Severity: High]
> This is a pre-existing issue, but does this scaling trigger an arithmetic
> overflow when the interval ratio is large?
> 
> When damon_nr_accesses_for_new_attrs() is called, it performs 32-bit
> arithmetic in damon_nr_accesses_to_accesses_bp() and
> damon_accesses_bp_to_nr_accesses().
> 
> Specifically:
> 
> mm/damon/core.c:damon_nr_accesses_to_accesses_bp() {
>     ...
>     return mult_frac(nr_accesses, 10000, damon_max_nr_accesses(attrs));
> }
> 
> mm/damon/core.c:damon_accesses_bp_to_nr_accesses() {
>     ...
>     return accesses_bp * damon_max_nr_accesses(attrs) / 10000;
> }
> 
> If the ratio of aggr_interval to sample_interval exceeds 429,496 (for
> example, a 1s aggregation and 1us sample), the intermediate multiplications
> like r * 10000 inside mult_frac() are computed as 32-bit unsigned int,
> which overflows the 32-bit maximum.
> 
> Can this silently corrupt the region access counts upon attribute updates,
> leading to completely incorrect monitoring results?

Known low priority issue.  Not a blocker of this patch.

> 
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does scaling
> last_nr_accesses fully resolve the incorrect values returned by
> damon_nr_accesses_mvsum(), or does it leave a double-counting flaw intact?
> 
> The commit message notes that damon_nr_accesses_mvsum() can return the
> wrong value. However, the root cause appears to be a state desynchronization
> rather than just a stale last_nr_accesses.
> 
> When damon_set_attrs() is called, it advances ctx->next_aggregation_sis
> into the future:
> 
> mm/damon/core.c:damon_set_attrs() {
>     ...
>     ctx->next_aggregation_sis = ctx->passed_sample_intervals +
>                                 attrs->aggr_interval / sample_interval;
> }
> 
> If a subsequent call to damon_nr_accesses_mvsum() evaluates this future
> timestamp, it assumes it is at the start of a new interval, meaning
> left_window_bp evaluates to 10000.
> 
> mm/damon/core.c:damon_nr_accesses_mvsum() {
>     ...
>     return damon_mvsum(r->nr_accesses, r->last_nr_accesses, left_window_bp);
> }
> 
> Because kdamond_reset_aggregated() has not yet executed, r->nr_accesses
> still holds the fully accumulated count of the just-completed interval.
> This means damon_mvsum() adds two full intervals' worth of access counts
> together, effectively calculating:
> r->nr_accesses + (r->last_nr_accesses * 10000 / 10000).
> 
> Does unconditionally scaling last_nr_accesses merely adjust the second term
> of this sum while preserving the underlying double-counting logic flaw?

Good point.  In the long term, damon_update_monitoring_result() should modify
nr_accesses.  We cannot do that at this moment, since we have to support both
nr_accesses_bp and damon_nr_accesses_mvsum().   I will add a temporal
preparatory hack to damonn_nr_accesses_mvsum() for this issue, like below.

'''
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -268,6 +268,8 @@ unsigned int damon_nr_accesses_mvsum(struct damon_region *r,
                        ctx->passed_sample_intervals;
        left_window_bp = mult_frac(left_window, 10000, window_len);

+       if (left_window_bp == 10000)
+               return r->last_nr_accesses;
        return damon_mvsum(r->nr_accesses, r->last_nr_accesses,
                        left_window_bp);
 }
'''

At the end of the series, we will updte damon_update_monitoring_result() to
reset nr_accesses always,and remove this temporal hack.


Thanks,
SJ

[...]