Re: [RFC PATCH v1.1 6/7] mm/damon/core: update probe hits for new parameter commit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Silent integer truncation when assigning scaled `probe_hits` from `unsigned int` to `unsigned char`.
- [Low] The KUnit test update adds the required `ctx` parameter but entirely fails to test the new `probe_hits` logic.
--

--- Patch [6]: [RFC PATCH v1.1 6/7] mm/damon/core: update probe hits for new parameter commit ---
commit 4d31f2a415d3352c6ff9b689ba6991c3606580b1
Author: SJ Park <sj@kernel.org>

mm/damon/core: update probe hits for new parameter commit

Users can update DAMON parameters at runtime. If the samples and/or
aggregation intervals are updated in this way, monitoring results depending on
the intervals should also be updated for a more accurate snapshot. This commit
updates probe_hits to reflect these changes.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index cda318806b65b..9a3ff0fe55a61 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -898,10 +898,32 @@ static unsigned int damon_nr_samples_for_new_attrs(unsigned int nr_accesses,
>  			new_attrs);
>  }
>  
> +static void damon_update_probe_hits(struct damon_region *r,
> +		struct damon_attrs *old_attrs, struct damon_attrs *new_attrs,
> +		bool aggregating, struct damon_ctx *ctx)
> +{
> +	struct damon_probe *p;
> +	int i = 0;
> +
> +	damon_for_each_probe(p, ctx) {
> +		r->last_probe_hits[i] = damon_nr_samples_for_new_attrs(
> +				r->last_probe_hits[i], old_attrs, new_attrs);

[Severity: Medium]
Does this code silently truncate the scaled sample counts?

The function damon_nr_samples_for_new_attrs() computes and returns an
unsigned int. However, r->last_probe_hits and r->probe_hits are arrays of
unsigned char.

If a privileged user updates DAMON parameters via sysfs such that the new
aggr_interval / sample_interval ratio scales the existing hits beyond 255, it
will silently truncate (modulo 256), losing the accuracy of the metric. 

Should the result be explicitly clamped via min_t(), or the underlying type
widened?

> +		if (!aggregating)
> +			r->probe_hits[i] = damon_nr_samples_for_new_attrs(
> +					r->probe_hits[i], old_attrs,
> +					new_attrs);
> +		else
> +			r->probe_hits[i] = 0;
> +		i++;
> +	}
> +}

[ ... ]

> diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
> index 9e631b19b184a..b4feef1a460f4 100644
> --- a/mm/damon/tests/core-kunit.h
> +++ b/mm/damon/tests/core-kunit.h
> @@ -591,32 +591,39 @@ static void damon_test_update_monitoring_result(struct kunit *test)
>  		.sample_interval = 10, .aggr_interval = 1000,};
>  	struct damon_attrs new_attrs;
>  	struct damon_region *r = damon_new_region(3, 7);
> +	struct damon_ctx *ctx;
>  
>  	if (!r)
>  		kunit_skip(test, "region alloc fail");
> +	ctx = damon_new_ctx();
> +	if (!ctx) {
> +		damon_free_region(r);
> +		kunit_skip(test, "ctx alloc fail");
> +	}

[Severity: Low]
Is the new probe_hits logic effectively dead code during this test?

The test allocates a new damon_ctx via damon_new_ctx(), but it doesn't add
any mock probes to it, nor does it initialize any probe_hits on the test
region. 

As a result, the new damon_for_each_probe() loop in
damon_update_probe_hits() exits immediately without testing the scaling logic.

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