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

[SJ Park <sj@kernel.org>]

On Tue, 07 Jul 2026 14:06:38 +0000 sashiko-bot@kernel.org wrote:

> 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?

Known low priority issue.

> 
> > +		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.

That's the intention.  This change is only for making the existing test not
broken.

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


Thanks,
SJ