Re: [PATCH 1/3] mm/damon: add damon_region->last_probe_hits

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] `damos_apply_target()` merges regions without validating or averaging the newly added `last_probe_hits` array.
- [Medium] Missing reset of `probe_hits` when monitoring attributes change dynamically mid-aggregation corrupts the new `last_probe_hits`.

Pre-existing issues:
- [High] `last_nr_accesses` and newly added `last_probe_hits` are not updated or averaged when regions are merged in `damon_merge_two_regions()`.
--

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

mm/damon: add damon_region->last_probe_hits

This commit adds a new last_probe_hits array to the damon_region struct.
It maintains fully accumulated probe_hits values from the last aggregation
interval in this new field.

> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index 805e089ff4f2f..4f7e305b261c3 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -68,10 +68,10 @@ struct damon_region {
>  	unsigned int nr_accesses;
>  	unsigned char probe_hits[DAMON_MAX_PROBES];
>  	struct list_head list;
> -
>  	unsigned int age;
>  /* private: Internal value for age calculation. */
>  	unsigned int last_nr_accesses;
> +	unsigned char last_probe_hits[DAMON_MAX_PROBES];
>  };

[Severity: High]
This is a pre-existing issue, but does damon_merge_two_regions() also need
to average last_nr_accesses and this newly added last_probe_hits array?

In mm/damon/core.c:damon_merge_two_regions():
    l->nr_accesses = (l->nr_accesses * sz_l + r->nr_accesses * sz_r) /
                     (sz_l + sz_r);
    l->age = (l->age * sz_l + r->age * sz_r) / (sz_l + sz_r);
    l->ar.end = r->ar.end;

Because last_nr_accesses and last_probe_hits are left unmodified as the left
region's values, it seems this can cause spurious age resets in later loop
passes. If the left region's last_nr_accesses was 0, but the newly averaged
nr_accesses is non-zero, the condition ((r->nr_accesses == 0) !=
(r->last_nr_accesses == 0)) evaluates to true and spuriously resets r->age
to 0.

[Severity: Medium]
Should this newly added field also be validated in damos_apply_target()?

When damos_apply_target() iterates over regions, it merges adjacent ones if
their age, nr_accesses, and last_nr_accesses match:

    if (prev_r->last_nr_accesses != r->last_nr_accesses)
        continue;
    if (prev_r->nr_accesses != r->nr_accesses)
        continue;

Because last_probe_hits isn't compared, regions with different probe statistics
might be erroneously merged, and the right region's probe hits would be
silently discarded.

[ ... ]

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index c6fc35be633b3..60be3e9d87887 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
[ ... ]
> @@ -2040,8 +2042,10 @@ static void kdamond_reset_aggregated(struct damon_ctx *c)
>  					damon_nr_regions(t), nr_probes);
>  			r->last_nr_accesses = r->nr_accesses;
>  			r->nr_accesses = 0;
> -			for (i = 0; i < DAMON_MAX_PROBES; i++)
> +			for (i = 0; i < DAMON_MAX_PROBES; i++) {
> +				r->last_probe_hits[i] = r->probe_hits[i];
>  				r->probe_hits[i] = 0;
> +			}
>  		}
>  		ti++;
>  	}

[Severity: Medium]
Could this copy un-reset, partial-interval probe_hits into last_probe_hits
when monitoring attributes change dynamically?

When damon_set_attrs() dynamically changes intervals mid-aggregation,
damon_update_monitoring_result() explicitly resets nr_accesses to drop the
partial state:

    if (!aggregating)
        r->nr_accesses = damon_nr_accesses_for_new_attrs(r->nr_accesses,
                                                         old_attrs, new_attrs);
    else
        r->nr_accesses = 0;

However, probe_hits is not reset. When kdamond_reset_aggregated() fires next,
will it populate last_probe_hits with mixed-interval garbage, violating the
fully accumulated semantic guarantee?

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