Re: [PATCH 3/3] mm/damon/sysfs-schemes: set probe hits as pseudo moving sums

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing left_window_bp == 10000 guard in damon_probe_hits_mvsum causes wrapped/doubled values at the aggregation interval boundary.
--

commit a039a69816e23f1f7d4a1680c2ab2713e7d81fe1
Author: SJ Park <sj@kernel.org>

mm/damon/sysfs-schemes: set probe hits as pseudo moving sums
    
DAMON sysfs interface exposes damon_region->probe_hits via probe hit
files.  Because the counters are completed only at the end of the
aggregation interval, users can show incomplete values if they requested
the file content update (update_schemes_tried_regions command) in the
middle of an aggregation interval.  Set the value as the pseudo moving
sum value of the counter, similar to that for nr_accesses.

> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> index dbf2b0515d582..32f495a96b17a 100644
> --- a/mm/damon/sysfs-schemes.c
> +++ b/mm/damon/sysfs-schemes.c
> @@ -110,7 +110,8 @@ static int damos_sysfs_probes_add_dirs(struct damos_sysfs_probes *probes,
>  		struct damos_sysfs_probe *sys_probe;
>  		int err;
>  
> -		sys_probe = damos_sysfs_probe_alloc(region->probe_hits[i]);
> +		sys_probe = damos_sysfs_probe_alloc(
> +				damon_probe_hits_mvsum(i, region, ctx));

[Severity: Medium]
Could this result in wrapped or doubled values at the aggregation interval
boundary?

Looking at damon_probe_hits_mvsum() in mm/damon/core.c, it seems to calculate
the moving sum without guarding against the boundary race condition, unlike
damon_nr_accesses_mvsum():

mm/damon/core.c:damon_probe_hits_mvsum() {
    ...
    left_window_bp = mult_frac(left_window, 10000, window_len);

    return damon_mvsum(r->probe_hits[probe_idx],
            r->last_probe_hits[probe_idx], left_window_bp);
}

If damon_set_attrs() updates ctx->next_aggregation_sis (e.g., via a sysfs
commit command) just before kdamond_apply_schemes() executes in the same loop
iteration, left_window_bp could evaluate to 10000.

If a user concurrently queues an update_schemes_tried_regions walk exactly at
this moment, the walk callback invokes damon_probe_hits_mvsum(). Because the
guard `if (left_window_bp == 10000)` is missing, the function evaluates to
probe_hits + last_probe_hits.

Since probe_hits retains its un-cleared values from before the attribute update
(as damon_update_monitoring_result() does not clear it), does this effectively
double the count and cause the 8-bit unsigned char to silently wrap around?

While this requires a tight race between sysfs writes (commit and
update_schemes_tried_regions), it appears structurally possible.

>  		if (!sys_probe) {
>  			damos_sysfs_probes_rm_dirs(probes);
>  			return -ENOMEM;

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