[RFC PATCH v1.1 0/7] mm/damon: update probe hits for runtime parameter commits

[RFC PATCH v1.1 0/7] mm/damon: update probe hits for runtime parameter commits

[SJ Park]

DAMON users can update DAMON parameters such as sampling and aggregation
intervals at runtime.  For such changes, monitoring results that depend
on the intervals should be properly updated for better accuracy.  For
example, the access frequency counter (nr_accesses) is updated.  The
data attribute monitoring counter (probe_hits) is not being updated,
though.  Do the updates for new parameters.

Patches Sequence
================

Patch 1 removes obsolete comments and test code on a function that this
series will touch.  Patches 2-5 rename functions that are being used for
nr_accesses update, to be able to be used for probe_hits without
confusion.  Patch 6 does the probe_hits update.  Patch 7 update
damon_probe_hits_mvsum() to cover a corner case from the update for
better accuracy.

Changelog
=========

Changes from RFC
- RFC: https://lore.kernel.org/20260707043828.97900-1-sj@kernel.org
- Remove obsolete comment and code on the test.

SJ Park (7):
  mm/damon/core: remove obsolete comment for nr_to_bp() divide-by-zero
  mm/damon/core: s/damon_max_nr_accesses()/damon_nr_samples_per_aggr()/
  mm/damon/core: s/accesses_bp_to_nr_accesses/sample_bp_to_count/
  mm/damon/core: s/nr_accesses_to_accesses_bp/sample_count_to_bp/
  mm/damon/core: s/nr_accesses_for_new_attrs/nr_samples_for_new_attrs/
  mm/damon/core: update probe hits for new parameter commit
  mm/damon/core: handle unreset probe_hits in probe_hits_mvsum()

 include/linux/damon.h       | 10 +++---
 mm/damon/core.c             | 64 ++++++++++++++++++++++++-------------
 mm/damon/lru_sort.c         |  2 +-
 mm/damon/ops-common.c       |  2 +-
 mm/damon/tests/core-kunit.h | 31 ++++++++----------
 5 files changed, 62 insertions(+), 47 deletions(-)


base-commit: 86f45ad9d6667f344421729eec5a9367e37bfc5b
-- 
2.47.3

[RFC PATCH v1.1 1/7] mm/damon/core: remove obsolete comment for nr_to_bp() divide-by-zero

[SJ Park]

damon_nr_accesses_to_accesses_bp() warns it can trigger division-by-zero
when the aggregation interval is zero.  Commit 35d4a3cf70a8
("mm/damon/ops-common: handle extreme intervals in damon_hot_score()")
modified damon_nr_samples_per_aggr() to always return non-zero.  Hence
no division-by-zero of the note can happen.  Remove the obsolete comment
on the function and its test code.  Also remove the unnecessary guard
code in the test.

Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/core.c             |  8 +-------
 mm/damon/tests/core-kunit.h | 12 ------------
 2 files changed, 1 insertion(+), 19 deletions(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 390e00b3685ef..a15005db82899 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -883,13 +883,7 @@ static unsigned int damon_accesses_bp_to_nr_accesses(
 	return accesses_bp * damon_max_nr_accesses(attrs) / 10000;
 }
 
-/*
- * Convert nr_accesses to access ratio in bp (per 10,000).
- *
- * Callers should ensure attrs.aggr_interval is not zero, like
- * damon_update_monitoring_results() does .  Otherwise, divide-by-zero would
- * happen.
- */
+/* Convert nr_accesses to access ratio in bp (per 10,000) */
 static unsigned int damon_nr_accesses_to_accesses_bp(
 		unsigned int nr_accesses, struct damon_attrs *attrs)
 {
diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
index 0124f83b39b83..224a3ecaa0701 100644
--- a/mm/damon/tests/core-kunit.h
+++ b/mm/damon/tests/core-kunit.h
@@ -582,18 +582,6 @@ static void damon_test_nr_accesses_to_accesses_bp(struct kunit *test)
 		.aggr_interval = ((unsigned long)UINT_MAX + 1) * 10
 	};
 
-	/*
-	 * In some cases such as 32bit architectures where UINT_MAX is
-	 * ULONG_MAX, attrs.aggr_interval becomes zero.  Calling
-	 * damon_nr_accesses_to_accesses_bp() in the case will cause
-	 * divide-by-zero.  Such case is prohibited in normal execution since
-	 * the caution is documented on the comment for the function, and
-	 * damon_update_monitoring_results() does the check.  Skip the test in
-	 * the case.
-	 */
-	if (!attrs.aggr_interval)
-		kunit_skip(test, "aggr_interval is zero.");
-
 	KUNIT_EXPECT_EQ(test, damon_nr_accesses_to_accesses_bp(123, &attrs), 0);
 }
 
-- 
2.47.3

[RFC PATCH v1.1 2/7] mm/damon/core: s/damon_max_nr_accesses()/damon_nr_samples_per_aggr()/

[SJ Park]

damon_max_nr_accesses() actually returns the number of samples DAMON
checks for each region per each aggregation interval.  Rename it to
better describe what it really does and not confusing for more general
uses.

Signed-off-by: SJ Park <sj@kernel.org>
---
 include/linux/damon.h | 10 ++++++----
 mm/damon/core.c       |  4 ++--
 mm/damon/lru_sort.c   |  2 +-
 mm/damon/ops-common.c |  2 +-
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/include/linux/damon.h b/include/linux/damon.h
index 19b7e839bde0b..63f596957c1f1 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -1056,15 +1056,17 @@ static inline bool damon_target_has_pid(const struct damon_ctx *ctx)
 	return ctx->ops.id == DAMON_OPS_VADDR || ctx->ops.id == DAMON_OPS_FVADDR;
 }
 
-static inline unsigned int damon_max_nr_accesses(const struct damon_attrs *attrs)
+/* Returns number of samples per aggregation interval */
+static inline unsigned int damon_nr_samples_per_aggr(
+		const struct damon_attrs *attrs)
 {
 	unsigned long sample_interval;
-	unsigned long max_nr_accesses;
+	unsigned long nr_samples;
 
 	sample_interval = attrs->sample_interval ? : 1;
-	max_nr_accesses = min(attrs->aggr_interval / sample_interval,
+	nr_samples = min(attrs->aggr_interval / sample_interval,
 			(unsigned long)UINT_MAX);
-	return max_nr_accesses ? : 1;
+	return nr_samples ? : 1;
 }
 
 
diff --git a/mm/damon/core.c b/mm/damon/core.c
index a15005db82899..177e5abc981ef 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -880,14 +880,14 @@ static unsigned int damon_age_for_new_attrs(unsigned int age,
 static unsigned int damon_accesses_bp_to_nr_accesses(
 		unsigned int accesses_bp, struct damon_attrs *attrs)
 {
-	return accesses_bp * damon_max_nr_accesses(attrs) / 10000;
+	return accesses_bp * damon_nr_samples_per_aggr(attrs) / 10000;
 }
 
 /* Convert nr_accesses to access ratio in bp (per 10,000) */
 static unsigned int damon_nr_accesses_to_accesses_bp(
 		unsigned int nr_accesses, struct damon_attrs *attrs)
 {
-	return mult_frac(nr_accesses, 10000, damon_max_nr_accesses(attrs));
+	return mult_frac(nr_accesses, 10000, damon_nr_samples_per_aggr(attrs));
 }
 
 static unsigned int damon_nr_accesses_for_new_attrs(unsigned int nr_accesses,
diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
index e8c389ad3226f..7e077084cb03a 100644
--- a/mm/damon/lru_sort.c
+++ b/mm/damon/lru_sort.c
@@ -303,7 +303,7 @@ static int damon_lru_sort_apply_parameters(void)
 		goto out;
 
 	err = -ENOMEM;
-	hot_thres = damon_max_nr_accesses(&attrs) *
+	hot_thres = damon_nr_samples_per_aggr(&attrs) *
 		hot_thres_access_freq / 1000;
 	hot_scheme = damon_lru_sort_new_hot_scheme(hot_thres);
 	if (!hot_scheme)
diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
index 6bdd1cfd3863a..89c694f6e4fa8 100644
--- a/mm/damon/ops-common.c
+++ b/mm/damon/ops-common.c
@@ -112,7 +112,7 @@ int damon_hot_score(struct damon_ctx *c, struct damon_region *r,
 	int hotness;
 
 	freq_subscore = r->nr_accesses * DAMON_MAX_SUBSCORE /
-		damon_max_nr_accesses(&c->attrs);
+		damon_nr_samples_per_aggr(&c->attrs);
 
 	age_in_sec = (unsigned long)r->age * c->attrs.aggr_interval / 1000000;
 	if (age_in_sec)
-- 
2.47.3

[RFC PATCH v1.1 3/7] mm/damon/core: s/accesses_bp_to_nr_accesses/sample_bp_to_count/

[SJ Park]

accesses_bp_to_nr_accesses() actually converts a positive samples ratio
to the count.  Rename it to better describe what it really does and not
confusing for more general uses.

Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 177e5abc981ef..dea63083917b0 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -876,11 +876,11 @@ static unsigned int damon_age_for_new_attrs(unsigned int age,
 	return age * old_attrs->aggr_interval / new_attrs->aggr_interval;
 }
 
-/* convert access ratio in bp (per 10,000) to nr_accesses */
-static unsigned int damon_accesses_bp_to_nr_accesses(
-		unsigned int accesses_bp, struct damon_attrs *attrs)
+/* convert sample ratio in bp (per 10,000) to count */
+static unsigned int damon_sample_bp_to_count(
+		unsigned int bp, struct damon_attrs *attrs)
 {
-	return accesses_bp * damon_nr_samples_per_aggr(attrs) / 10000;
+	return bp * damon_nr_samples_per_aggr(attrs) / 10000;
 }
 
 /* Convert nr_accesses to access ratio in bp (per 10,000) */
@@ -893,7 +893,7 @@ static unsigned int damon_nr_accesses_to_accesses_bp(
 static unsigned int damon_nr_accesses_for_new_attrs(unsigned int nr_accesses,
 		struct damon_attrs *old_attrs, struct damon_attrs *new_attrs)
 {
-	return damon_accesses_bp_to_nr_accesses(
+	return damon_sample_bp_to_count(
 			damon_nr_accesses_to_accesses_bp(
 				nr_accesses, old_attrs),
 			new_attrs);
-- 
2.47.3

[RFC PATCH v1.1 4/7] mm/damon/core: s/nr_accesses_to_accesses_bp/sample_count_to_bp/

[SJ Park]

damon_nr_accesses_to_accesses_bp() actually converts a positive sample
count to the ratio.  Rename it to better describe what it really does
and not confusing for more general uses.

Also remove the obsolete comment about division-by-zero.  Commit
35d4a3cf70a8 ("mm/damon/ops-common: handle extreme intervals in
damon_hot_score()") modified damon_nr_samples_per_aggr() to always
return non-zero.  Hence no division-by-zero of the note can happen.

Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/core.c             | 11 +++++------
 mm/damon/tests/core-kunit.h |  6 +++---
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index dea63083917b0..718268df27fa7 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -883,19 +883,18 @@ static unsigned int damon_sample_bp_to_count(
 	return bp * damon_nr_samples_per_aggr(attrs) / 10000;
 }
 
-/* Convert nr_accesses to access ratio in bp (per 10,000) */
-static unsigned int damon_nr_accesses_to_accesses_bp(
-		unsigned int nr_accesses, struct damon_attrs *attrs)
+/* convert sample count to ratio in bp (per 10,000) */
+static unsigned int damon_sample_count_to_bp(
+		unsigned int count, struct damon_attrs *attrs)
 {
-	return mult_frac(nr_accesses, 10000, damon_nr_samples_per_aggr(attrs));
+	return mult_frac(count, 10000, damon_nr_samples_per_aggr(attrs));
 }
 
 static unsigned int damon_nr_accesses_for_new_attrs(unsigned int nr_accesses,
 		struct damon_attrs *old_attrs, struct damon_attrs *new_attrs)
 {
 	return damon_sample_bp_to_count(
-			damon_nr_accesses_to_accesses_bp(
-				nr_accesses, old_attrs),
+			damon_sample_count_to_bp(nr_accesses, old_attrs),
 			new_attrs);
 }
 
diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
index 224a3ecaa0701..9e631b19b184a 100644
--- a/mm/damon/tests/core-kunit.h
+++ b/mm/damon/tests/core-kunit.h
@@ -575,14 +575,14 @@ static void damon_test_set_regions(struct kunit *test)
 			}, 3);
 }
 
-static void damon_test_nr_accesses_to_accesses_bp(struct kunit *test)
+static void damon_test_sample_count_to_bp(struct kunit *test)
 {
 	struct damon_attrs attrs = {
 		.sample_interval = 10,
 		.aggr_interval = ((unsigned long)UINT_MAX + 1) * 10
 	};
 
-	KUNIT_EXPECT_EQ(test, damon_nr_accesses_to_accesses_bp(123, &attrs), 0);
+	KUNIT_EXPECT_EQ(test, damon_sample_count_to_bp(123, &attrs), 0);
 }
 
 static void damon_test_update_monitoring_result(struct kunit *test)
@@ -1560,7 +1560,7 @@ static struct kunit_case damon_test_cases[] = {
 	KUNIT_CASE(damon_test_split_above_half_progresses),
 	KUNIT_CASE(damon_test_ops_registration),
 	KUNIT_CASE(damon_test_set_regions),
-	KUNIT_CASE(damon_test_nr_accesses_to_accesses_bp),
+	KUNIT_CASE(damon_test_sample_count_to_bp),
 	KUNIT_CASE(damon_test_update_monitoring_result),
 	KUNIT_CASE(damon_test_set_attrs),
 	KUNIT_CASE(damon_test_mvsum),
-- 
2.47.3

[RFC PATCH v1.1 5/7] mm/damon/core: s/nr_accesses_for_new_attrs/nr_samples_for_new_attrs/

[SJ Park]

damon_nr_accesses_for_new_attrs() can be used for not only nr_accesses
but also any positive sample count, like probe_hits.  Rename to be able
to be used for such general uses without confusion.

Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 718268df27fa7..cda318806b65b 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -890,7 +890,7 @@ static unsigned int damon_sample_count_to_bp(
 	return mult_frac(count, 10000, damon_nr_samples_per_aggr(attrs));
 }
 
-static unsigned int damon_nr_accesses_for_new_attrs(unsigned int nr_accesses,
+static unsigned int damon_nr_samples_for_new_attrs(unsigned int nr_accesses,
 		struct damon_attrs *old_attrs, struct damon_attrs *new_attrs)
 {
 	return damon_sample_bp_to_count(
@@ -902,10 +902,10 @@ 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 = damon_nr_samples_for_new_attrs(
 			r->last_nr_accesses, old_attrs, new_attrs);
 	if (!aggregating)
-		r->nr_accesses = damon_nr_accesses_for_new_attrs(
+		r->nr_accesses = damon_nr_samples_for_new_attrs(
 				r->nr_accesses, old_attrs, new_attrs);
 	else
 		/*
-- 
2.47.3

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

[SJ Park]

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.  The age and nr_accesses are properly updated, while
probe_hits are not updated in the way.  Do the update.

Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/core.c             | 28 +++++++++++++++++++++++++---
 mm/damon/tests/core-kunit.h | 13 ++++++++++---
 2 files changed, 35 insertions(+), 6 deletions(-)

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);
+		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++;
+	}
+}
+
 static void damon_update_monitoring_result(struct damon_region *r,
 		struct damon_attrs *old_attrs, struct damon_attrs *new_attrs,
-		bool aggregating)
+		bool aggregating, struct damon_ctx *ctx)
 {
+	damon_update_probe_hits(r, old_attrs, new_attrs, aggregating, ctx);
+
 	r->last_nr_accesses = damon_nr_samples_for_new_attrs(
 			r->last_nr_accesses, old_attrs, new_attrs);
 	if (!aggregating)
@@ -941,8 +963,8 @@ static void damon_update_monitoring_results(struct damon_ctx *ctx,
 
 	damon_for_each_target(t, ctx)
 		damon_for_each_region(r, t)
-			damon_update_monitoring_result(
-					r, old_attrs, new_attrs, aggregating);
+			damon_update_monitoring_result(r, old_attrs, new_attrs,
+					aggregating, ctx);
 }
 
 /*
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");
+	}
 
 	r->nr_accesses = 15;
 	r->age = 20;
 
 	new_attrs = (struct damon_attrs){
 		.sample_interval = 100, .aggr_interval = 10000,};
-	damon_update_monitoring_result(r, &old_attrs, &new_attrs, false);
+	damon_update_monitoring_result(r, &old_attrs, &new_attrs, false, ctx);
 	KUNIT_EXPECT_EQ(test, r->nr_accesses, 15);
 	KUNIT_EXPECT_EQ(test, r->age, 2);
 
 	new_attrs = (struct damon_attrs){
 		.sample_interval = 1, .aggr_interval = 1000};
-	damon_update_monitoring_result(r, &old_attrs, &new_attrs, false);
+	damon_update_monitoring_result(r, &old_attrs, &new_attrs, false, ctx);
 	KUNIT_EXPECT_EQ(test, r->nr_accesses, 150);
 	KUNIT_EXPECT_EQ(test, r->age, 2);
 
 	new_attrs = (struct damon_attrs){
 		.sample_interval = 1, .aggr_interval = 100};
-	damon_update_monitoring_result(r, &old_attrs, &new_attrs, false);
+	damon_update_monitoring_result(r, &old_attrs, &new_attrs, false, ctx);
 	KUNIT_EXPECT_EQ(test, r->nr_accesses, 150);
 	KUNIT_EXPECT_EQ(test, r->age, 20);
 
 	damon_free_region(r);
+	damon_destroy_ctx(ctx);
 }
 
 static void damon_test_set_attrs(struct kunit *test)
-- 
2.47.3

[RFC PATCH v1.1 7/7] mm/damon/core: handle unreset probe_hits in probe_hits_mvsum()

[SJ Park]

If damon_update_monitoring_result() is called at the end of the
aggregation interval, probe_hits is not reset.  That's because the value
will be exposed to the user via damon_region_aggregated trace event.

Meanwhile, damon_probe_hits_mvsum() can be called in this state.  Due to
its logic, it will return a value that is incorrectly high.  This could
happen  if the user requested DAMOS schemes applied regions sysfs files
update exactly in the time sequence.

The impact is minor, but better to avoid.  Check the timing and simply
return the fully aggregated last_probe_hits, like
damon_nr_accesses_mvsum() also does.  It is not 100% accurate since it
is the last interval's aggregation.  But better than the value that is
completely reset.

Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 9a3ff0fe55a61..093da5c0b3693 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -290,6 +290,9 @@ unsigned char damon_probe_hits_mvsum(int probe_idx, 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_probe_hits[probe_idx];
+
 	return damon_mvsum(r->probe_hits[probe_idx],
 			r->last_probe_hits[probe_idx], left_window_bp);
 }
-- 
2.47.3

Re: [RFC PATCH v1.1 1/7] mm/damon/core: remove obsolete comment for nr_to_bp() divide-by-zero

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Removing the `!attrs.aggr_interval` skip guard causes `damon_test_nr_accesses_to_accesses_bp` to fail deterministically on 32-bit architectures.
--

--- Patch [1]: [RFC PATCH v1.1 1/7] mm/damon/core: remove obsolete comment for nr_to_bp() divide-by-zero ---
commit a31fcddef9e05dc323b42f64143ffe58dc702dee
Author: SJ Park <sj@kernel.org>

mm/damon/core: remove obsolete comment for nr_to_bp() divide-by-zero

This commit removes obsolete comments about division-by-zero in
damon_nr_accesses_to_accesses_bp() and its related test code. A previous
commit modified the underlying calculation to always return a non-zero value,
preventing the division-by-zero issue.

> diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
> index 0124f83b39b83..224a3ecaa0701 100644
> --- a/mm/damon/tests/core-kunit.h
> +++ b/mm/damon/tests/core-kunit.h
[ ... ]
> @@ -582,18 +582,6 @@ static void damon_test_nr_accesses_to_accesses_bp(struct kunit *test)
>  		.aggr_interval = ((unsigned long)UINT_MAX + 1) * 10
>  	};
>  
> -	/*
> -	 * In some cases such as 32bit architectures where UINT_MAX is
> -	 * ULONG_MAX, attrs.aggr_interval becomes zero.  Calling
> -	 * damon_nr_accesses_to_accesses_bp() in the case will cause
> -	 * divide-by-zero.  Such case is prohibited in normal execution since
> -	 * the caution is documented on the comment for the function, and
> -	 * damon_update_monitoring_results() does the check.  Skip the test in
> -	 * the case.
> -	 */
> -	if (!attrs.aggr_interval)
> -		kunit_skip(test, "aggr_interval is zero.");
> -
>  	KUNIT_EXPECT_EQ(test, damon_nr_accesses_to_accesses_bp(123, &attrs), 0);

[Severity: Medium]
Will removing this skip guard cause a deterministic test failure on 32-bit
architectures?

When the test runs on a 32-bit architecture, attrs.aggr_interval is initialized
to ((unsigned long)UINT_MAX + 1) * 10, which overflows to 0. With the division
by zero fix in place, damon_max_nr_accesses(&attrs) will return 1 instead of 0.

This causes damon_nr_accesses_to_accesses_bp(123, &attrs) to evaluate to
(123 * 10000) / 1 = 1230000. Since the test unconditionally expects the result
to be 0, the KUNIT_EXPECT_EQ assertion will fail.

>  }

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

Re: [RFC PATCH v1.1 2/7] mm/damon/core: s/damon_max_nr_accesses()/damon_nr_samples_per_aggr()/

[sashiko-bot]

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

Pre-existing issues:
- [High] Multiplications by 10000 in DAMON basis-point conversion functions overflow 32-bit unsigned integers when the number of samples per aggregation interval is large.
--

--- Patch [2]: [RFC PATCH v1.1 2/7] mm/damon/core: s/damon_max_nr_accesses()/damon_nr_samples_per_aggr()/ ---
commit f63888911d3e9e966f4abf35c7d87cb96cc0e08b
Author: SJ Park <sj@kernel.org>

mm/damon/core: s/damon_max_nr_accesses()/damon_nr_samples_per_aggr()/

This commit renames damon_max_nr_accesses() to damon_nr_samples_per_aggr()
to better describe that it returns the number of samples DAMON checks for
each region per aggregation interval.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index a15005db82899..177e5abc981ef 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -880,14 +880,14 @@ static unsigned int damon_age_for_new_attrs(unsigned int age,
>  static unsigned int damon_accesses_bp_to_nr_accesses(
>  		unsigned int accesses_bp, struct damon_attrs *attrs)
>  {
> -	return accesses_bp * damon_max_nr_accesses(attrs) / 10000;
> +	return accesses_bp * damon_nr_samples_per_aggr(attrs) / 10000;
>  }

[Severity: High]
This is a pre-existing issue, but does this calculation overflow 32-bit
unsigned integers when the number of samples per aggregation interval is
large?

In damon_accesses_bp_to_nr_accesses() in mm/damon/core.c, both accesses_bp
and the return value of damon_nr_samples_per_aggr() are 32-bit unsigned
integers. If an administrator configures DAMON with a large ratio between
aggr_interval and sample_interval (e.g., > 429,496, achievable via sysfs
with a 1us sample and 500ms aggregation), the multiplication will silently
overflow UINT_MAX, yielding much smaller thresholds than intended.

>  /* Convert nr_accesses to access ratio in bp (per 10,000) */
>  static unsigned int damon_nr_accesses_to_accesses_bp(
>  		unsigned int nr_accesses, struct damon_attrs *attrs)
>  {
> -	return mult_frac(nr_accesses, 10000, damon_max_nr_accesses(attrs));
> +	return mult_frac(nr_accesses, 10000, damon_nr_samples_per_aggr(attrs));
>  }

[Severity: High]
This isn't a bug introduced by this patch, but can this macro expansion also
suffer from a 32-bit overflow?

The mult_frac() macro adopts the type of its first argument, nr_accesses,
which is an unsigned int. Its internal calculation of the remainder
multiplied by the numerator (r * n_) evaluates as an unsigned int.

When the remainder r (which is bounded by samples_per_aggr) exceeds 429,496,
this multiplication by 10000 will overflow 32-bit boundaries, leading to
incorrect basis-point ratio calculations.

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

Re: [RFC PATCH v1.1 4/7] mm/damon/core: s/nr_accesses_to_accesses_bp/sample_count_to_bp/

[sashiko-bot]

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

New issues:
- [Medium] The KUnit test `damon_test_sample_count_to_bp` unconditionally fails on 32-bit architectures due to integer overflow in `aggr_interval` initialization.
- [Low] The commit message erroneously claims to remove an obsolete division-by-zero comment that is not present in the patch diff.

Pre-existing issues:
- [High] Arithmetic overflow in `damon_sample_bp_to_count()` and `damon_sample_count_to_bp()` on all architectures when `nr_samples_per_aggr` is large.
--

--- Patch [4]: [RFC PATCH v1.1 4/7] mm/damon/core: s/nr_accesses_to_accesses_bp/sample_count_to_bp/ ---
commit 3660ea5f51f638ba678520c22cb275f27a0966b1
Author: SJ Park <sj@kernel.org>
mm/damon/core: s/nr_accesses_to_accesses_bp/sample_count_to_bp/

This commit renames damon_nr_accesses_to_accesses_bp() to
damon_sample_count_to_bp() to better reflect its behavior. It also mentions
removing an obsolete division-by-zero comment.

>     Also remove the obsolete comment about division-by-zero.  Commit
>     35d4a3cf70a8 ("mm/damon/ops-common: handle extreme intervals in
>     damon_hot_score()") modified damon_nr_samples_per_aggr() to always
>     return non-zero.  Hence no division-by-zero of the note can happen.

[Severity: Low]
This isn't a bug, but the commit message states it removes an obsolete
division-by-zero comment. However, this comment does not appear to be
removed in the diff below. Was it already removed by an earlier commit
in the series?

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index dea63083917b0..718268df27fa7 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -883,19 +883,18 @@ static unsigned int damon_sample_bp_to_count(
>  	return bp * damon_nr_samples_per_aggr(attrs) / 10000;
>  }

[Severity: High]
This is a pre-existing issue, but can this multiplication silently
overflow on architectures where unsigned int is 32-bit?

If a privileged user configures DAMON attributes via sysfs such that
aggr_interval / sample_interval > 429,496, damon_nr_samples_per_aggr()
will return a large 32-bit unsigned integer. The multiplication
bp * damon_nr_samples_per_aggr(attrs) could then exceed 32 bits, resulting
in incorrect sample counts and potentially flawed memory management policies.

>  
> -/* Convert nr_accesses to access ratio in bp (per 10,000) */
> -static unsigned int damon_nr_accesses_to_accesses_bp(
> -		unsigned int nr_accesses, struct damon_attrs *attrs)
> +/* convert sample count to ratio in bp (per 10,000) */
> +static unsigned int damon_sample_count_to_bp(
> +		unsigned int count, struct damon_attrs *attrs)
>  {
> -	return mult_frac(nr_accesses, 10000, damon_nr_samples_per_aggr(attrs));
> +	return mult_frac(count, 10000, damon_nr_samples_per_aggr(attrs));
>  }

[Severity: High]
This is also a pre-existing issue, but does mult_frac() run into a similar
32-bit overflow?

The mult_frac macro evaluates the remainder multiplication (count * 10000)
as a 32-bit unsigned int. If count >= 429,497, this could overflow, leading
to a flawed ratio calculation.

[ ... ]

> diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
> index 224a3ecaa0701..9e631b19b184a 100644
> --- a/mm/damon/tests/core-kunit.h
> +++ b/mm/damon/tests/core-kunit.h
> @@ -575,14 +575,14 @@ static void damon_test_set_regions(struct kunit *test)
[ ... ]
> -static void damon_test_nr_accesses_to_accesses_bp(struct kunit *test)
> +static void damon_test_sample_count_to_bp(struct kunit *test)
>  {
>  	struct damon_attrs attrs = {
>  		.sample_interval = 10,
>  		.aggr_interval = ((unsigned long)UINT_MAX + 1) * 10
>  	};

[Severity: Medium]
Does this test initialization unconditionally fail on 32-bit architectures?

On 32-bit systems, unsigned long is 32 bits, causing ((unsigned long)UINT_MAX + 1)
to wrap to 0. This makes aggr_interval = 0.

>  
> -	KUNIT_EXPECT_EQ(test, damon_nr_accesses_to_accesses_bp(123, &attrs), 0);
> +	KUNIT_EXPECT_EQ(test, damon_sample_count_to_bp(123, &attrs), 0);
>  }

Because aggr_interval is 0, damon_nr_samples_per_aggr() safely returns 1.
The call to damon_sample_count_to_bp(123, &attrs) then computes
123 * 10000 / 1, which evaluates to 1,230,000. This would fail the
KUNIT_EXPECT_EQ assertion that expects 0.

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

Re: [RFC PATCH v1.1 5/7] mm/damon/core: s/nr_accesses_for_new_attrs/nr_samples_for_new_attrs/

[sashiko-bot]

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

New issues:
- [Low] Incomplete refactoring: the parameter name `nr_accesses` was not updated to reflect the new generic purpose.

Pre-existing issues:
- [High] 32-bit arithmetic overflow in `damon_sample_bp_to_count` and `damon_sample_count_to_bp` corrupts interval conversions for large aggregation windows.
--

--- Patch [5]: [RFC PATCH v1.1 5/7] mm/damon/core: s/nr_accesses_for_new_attrs/nr_samples_for_new_attrs/ ---
commit 17e2132821e6ed9a2213f8cf4fa6bdeba54db3b6
Author: SJ Park <sj@kernel.org>

mm/damon/core: s/nr_accesses_for_new_attrs/nr_samples_for_new_attrs/

damon_nr_accesses_for_new_attrs() can be used for not only nr_accesses
but also any positive sample count, like probe_hits.  Rename to be able
to be used for such general uses without confusion.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 718268df27fa7..cda318806b65b 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -890,7 +890,7 @@ static unsigned int damon_sample_count_to_bp(
>  	return mult_frac(count, 10000, damon_nr_samples_per_aggr(attrs));
>  }
>  

[Severity: High]
This is a pre-existing issue, but does this calculation silently overflow for
large aggregation intervals?

When users configure large aggregation intervals (or very small sample
intervals) such that samples_per_aggr exceeds 429,496, calculating the access
frequency ratio overflows 32-bit math.

The mult_frac() macro calculates (count % samples) * 10000 using the 32-bit
type of count. If samples_per_aggr > 429,496, the remainder can be large
enough that multiplying by 10000 causes an overflow.

A similar 32-bit arithmetic overflow can happen in damon_sample_bp_to_count():

    return bp * damon_nr_samples_per_aggr(attrs) / 10000;

> -static unsigned int damon_nr_accesses_for_new_attrs(unsigned int nr_accesses,
> +static unsigned int damon_nr_samples_for_new_attrs(unsigned int nr_accesses,
>  		struct damon_attrs *old_attrs, struct damon_attrs *new_attrs)
>  {

[Severity: Low]
Is the nr_accesses parameter name still appropriate here?

Since the function is being renamed to be used for general sample counts
(like probe_hits) without confusion, leaving the first parameter named
nr_accesses creates a semantic mismatch that limits the readability benefits
of this refactoring.

>  	return damon_sample_bp_to_count(

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

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

[sashiko-bot]

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

Re: [RFC PATCH v1.1 1/7] mm/damon/core: remove obsolete comment for nr_to_bp() divide-by-zero

[SJ Park]

On Tue, 07 Jul 2026 13:57:50 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Removing the `!attrs.aggr_interval` skip guard causes `damon_test_nr_accesses_to_accesses_bp` to fail deterministically on 32-bit architectures.
> --
> 
> --- Patch [1]: [RFC PATCH v1.1 1/7] mm/damon/core: remove obsolete comment for nr_to_bp() divide-by-zero ---
> commit a31fcddef9e05dc323b42f64143ffe58dc702dee
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/core: remove obsolete comment for nr_to_bp() divide-by-zero
> 
> This commit removes obsolete comments about division-by-zero in
> damon_nr_accesses_to_accesses_bp() and its related test code. A previous
> commit modified the underlying calculation to always return a non-zero value,
> preventing the division-by-zero issue.
> 
> > diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
> > index 0124f83b39b83..224a3ecaa0701 100644
> > --- a/mm/damon/tests/core-kunit.h
> > +++ b/mm/damon/tests/core-kunit.h
> [ ... ]
> > @@ -582,18 +582,6 @@ static void damon_test_nr_accesses_to_accesses_bp(struct kunit *test)
> >  		.aggr_interval = ((unsigned long)UINT_MAX + 1) * 10
> >  	};
> >  
> > -	/*
> > -	 * In some cases such as 32bit architectures where UINT_MAX is
> > -	 * ULONG_MAX, attrs.aggr_interval becomes zero.  Calling
> > -	 * damon_nr_accesses_to_accesses_bp() in the case will cause
> > -	 * divide-by-zero.  Such case is prohibited in normal execution since
> > -	 * the caution is documented on the comment for the function, and
> > -	 * damon_update_monitoring_results() does the check.  Skip the test in
> > -	 * the case.
> > -	 */
> > -	if (!attrs.aggr_interval)
> > -		kunit_skip(test, "aggr_interval is zero.");
> > -
> >  	KUNIT_EXPECT_EQ(test, damon_nr_accesses_to_accesses_bp(123, &attrs), 0);
> 
> [Severity: Medium]
> Will removing this skip guard cause a deterministic test failure on 32-bit
> architectures?
> 
> When the test runs on a 32-bit architecture, attrs.aggr_interval is initialized
> to ((unsigned long)UINT_MAX + 1) * 10, which overflows to 0. With the division
> by zero fix in place, damon_max_nr_accesses(&attrs) will return 1 instead of 0.
> 
> This causes damon_nr_accesses_to_accesses_bp(123, &attrs) to evaluate to
> (123 * 10000) / 1 = 1230000. Since the test unconditionally expects the result
> to be 0, the KUNIT_EXPECT_EQ assertion will fail.

Good point.  Actually, this test was only for division by zero, and now it
seems it is just broken.  Maybe a better way is testing if
damon_max_nr_accesses() returns zero or not in the extreme cases.

I will simply drop the entire test from the next revision.

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

Thanks,
SJ

Re: [RFC PATCH v1.1 2/7] mm/damon/core: s/damon_max_nr_accesses()/damon_nr_samples_per_aggr()/

[SJ Park]

On Tue, 07 Jul 2026 13:58:22 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Multiplications by 10000 in DAMON basis-point conversion functions overflow 32-bit unsigned integers when the number of samples per aggregation interval is large.
> --
> 
> --- Patch [2]: [RFC PATCH v1.1 2/7] mm/damon/core: s/damon_max_nr_accesses()/damon_nr_samples_per_aggr()/ ---
> commit f63888911d3e9e966f4abf35c7d87cb96cc0e08b
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/core: s/damon_max_nr_accesses()/damon_nr_samples_per_aggr()/
> 
> This commit renames damon_max_nr_accesses() to damon_nr_samples_per_aggr()
> to better describe that it returns the number of samples DAMON checks for
> each region per aggregation interval.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index a15005db82899..177e5abc981ef 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -880,14 +880,14 @@ static unsigned int damon_age_for_new_attrs(unsigned int age,
> >  static unsigned int damon_accesses_bp_to_nr_accesses(
> >  		unsigned int accesses_bp, struct damon_attrs *attrs)
> >  {
> > -	return accesses_bp * damon_max_nr_accesses(attrs) / 10000;
> > +	return accesses_bp * damon_nr_samples_per_aggr(attrs) / 10000;
> >  }
> 
> [Severity: High]
> This is a pre-existing issue, but does this calculation overflow 32-bit
> unsigned integers when the number of samples per aggregation interval is
> large?
> 
> In damon_accesses_bp_to_nr_accesses() in mm/damon/core.c, both accesses_bp
> and the return value of damon_nr_samples_per_aggr() are 32-bit unsigned
> integers. If an administrator configures DAMON with a large ratio between
> aggr_interval and sample_interval (e.g., > 429,496, achievable via sysfs
> with a 1us sample and 500ms aggregation), the multiplication will silently
> overflow UINT_MAX, yielding much smaller thresholds than intended.

Known low priority issue.

> 
> >  /* Convert nr_accesses to access ratio in bp (per 10,000) */
> >  static unsigned int damon_nr_accesses_to_accesses_bp(
> >  		unsigned int nr_accesses, struct damon_attrs *attrs)
> >  {
> > -	return mult_frac(nr_accesses, 10000, damon_max_nr_accesses(attrs));
> > +	return mult_frac(nr_accesses, 10000, damon_nr_samples_per_aggr(attrs));
> >  }
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but can this macro expansion also
> suffer from a 32-bit overflow?
> 
> The mult_frac() macro adopts the type of its first argument, nr_accesses,
> which is an unsigned int. Its internal calculation of the remainder
> multiplied by the numerator (r * n_) evaluates as an unsigned int.
> 
> When the remainder r (which is bounded by samples_per_aggr) exceeds 429,496,
> this multiplication by 10000 will overflow 32-bit boundaries, leading to
> incorrect basis-point ratio calculations.

Known low priority issue.

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



Thanks,
SJ

Re: [RFC PATCH v1.1 4/7] mm/damon/core: s/nr_accesses_to_accesses_bp/sample_count_to_bp/

[SJ Park]

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

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [Medium] The KUnit test `damon_test_sample_count_to_bp` unconditionally fails on 32-bit architectures due to integer overflow in `aggr_interval` initialization.
> - [Low] The commit message erroneously claims to remove an obsolete division-by-zero comment that is not present in the patch diff.
> 
> Pre-existing issues:
> - [High] Arithmetic overflow in `damon_sample_bp_to_count()` and `damon_sample_count_to_bp()` on all architectures when `nr_samples_per_aggr` is large.
> --
> 
> --- Patch [4]: [RFC PATCH v1.1 4/7] mm/damon/core: s/nr_accesses_to_accesses_bp/sample_count_to_bp/ ---
> commit 3660ea5f51f638ba678520c22cb275f27a0966b1
> Author: SJ Park <sj@kernel.org>
> mm/damon/core: s/nr_accesses_to_accesses_bp/sample_count_to_bp/
> 
> This commit renames damon_nr_accesses_to_accesses_bp() to
> damon_sample_count_to_bp() to better reflect its behavior. It also mentions
> removing an obsolete division-by-zero comment.
> 
> >     Also remove the obsolete comment about division-by-zero.  Commit
> >     35d4a3cf70a8 ("mm/damon/ops-common: handle extreme intervals in
> >     damon_hot_score()") modified damon_nr_samples_per_aggr() to always
> >     return non-zero.  Hence no division-by-zero of the note can happen.
> 
> [Severity: Low]
> This isn't a bug, but the commit message states it removes an obsolete
> division-by-zero comment. However, this comment does not appear to be
> removed in the diff below. Was it already removed by an earlier commit
> in the series?

Good catch.  I will remove that part from the commit message.

> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index dea63083917b0..718268df27fa7 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -883,19 +883,18 @@ static unsigned int damon_sample_bp_to_count(
> >  	return bp * damon_nr_samples_per_aggr(attrs) / 10000;
> >  }
> 
> [Severity: High]
> This is a pre-existing issue, but can this multiplication silently
> overflow on architectures where unsigned int is 32-bit?
> 
> If a privileged user configures DAMON attributes via sysfs such that
> aggr_interval / sample_interval > 429,496, damon_nr_samples_per_aggr()
> will return a large 32-bit unsigned integer. The multiplication
> bp * damon_nr_samples_per_aggr(attrs) could then exceed 32 bits, resulting
> in incorrect sample counts and potentially flawed memory management policies.

Known low priority issue.

> 
> >  
> > -/* Convert nr_accesses to access ratio in bp (per 10,000) */
> > -static unsigned int damon_nr_accesses_to_accesses_bp(
> > -		unsigned int nr_accesses, struct damon_attrs *attrs)
> > +/* convert sample count to ratio in bp (per 10,000) */
> > +static unsigned int damon_sample_count_to_bp(
> > +		unsigned int count, struct damon_attrs *attrs)
> >  {
> > -	return mult_frac(nr_accesses, 10000, damon_nr_samples_per_aggr(attrs));
> > +	return mult_frac(count, 10000, damon_nr_samples_per_aggr(attrs));
> >  }
> 
> [Severity: High]
> This is also a pre-existing issue, but does mult_frac() run into a similar
> 32-bit overflow?
> 
> The mult_frac macro evaluates the remainder multiplication (count * 10000)
> as a 32-bit unsigned int. If count >= 429,497, this could overflow, leading
> to a flawed ratio calculation.

Known low priority issue.

> 
> [ ... ]
> 
> > diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
> > index 224a3ecaa0701..9e631b19b184a 100644
> > --- a/mm/damon/tests/core-kunit.h
> > +++ b/mm/damon/tests/core-kunit.h
> > @@ -575,14 +575,14 @@ static void damon_test_set_regions(struct kunit *test)
> [ ... ]
> > -static void damon_test_nr_accesses_to_accesses_bp(struct kunit *test)
> > +static void damon_test_sample_count_to_bp(struct kunit *test)
> >  {
> >  	struct damon_attrs attrs = {
> >  		.sample_interval = 10,
> >  		.aggr_interval = ((unsigned long)UINT_MAX + 1) * 10
> >  	};
> 
> [Severity: Medium]
> Does this test initialization unconditionally fail on 32-bit architectures?
> 
> On 32-bit systems, unsigned long is 32 bits, causing ((unsigned long)UINT_MAX + 1)
> to wrap to 0. This makes aggr_interval = 0.
> 
> >  
> > -	KUNIT_EXPECT_EQ(test, damon_nr_accesses_to_accesses_bp(123, &attrs), 0);
> > +	KUNIT_EXPECT_EQ(test, damon_sample_count_to_bp(123, &attrs), 0);
> >  }
> 
> Because aggr_interval is 0, damon_nr_samples_per_aggr() safely returns 1.
> The call to damon_sample_count_to_bp(123, &attrs) then computes
> 123 * 10000 / 1, which evaluates to 1,230,000. This would fail the
> KUNIT_EXPECT_EQ assertion that expects 0.

As I replied to earlier review, I will remove the test entirely.

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


Thanks,
SJ

Re: [RFC PATCH v1.1 5/7] mm/damon/core: s/nr_accesses_for_new_attrs/nr_samples_for_new_attrs/

[SJ Park]

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

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Low] Incomplete refactoring: the parameter name `nr_accesses` was not updated to reflect the new generic purpose.
> 
> Pre-existing issues:
> - [High] 32-bit arithmetic overflow in `damon_sample_bp_to_count` and `damon_sample_count_to_bp` corrupts interval conversions for large aggregation windows.
> --
> 
> --- Patch [5]: [RFC PATCH v1.1 5/7] mm/damon/core: s/nr_accesses_for_new_attrs/nr_samples_for_new_attrs/ ---
> commit 17e2132821e6ed9a2213f8cf4fa6bdeba54db3b6
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/core: s/nr_accesses_for_new_attrs/nr_samples_for_new_attrs/
> 
> damon_nr_accesses_for_new_attrs() can be used for not only nr_accesses
> but also any positive sample count, like probe_hits.  Rename to be able
> to be used for such general uses without confusion.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 718268df27fa7..cda318806b65b 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -890,7 +890,7 @@ static unsigned int damon_sample_count_to_bp(
> >  	return mult_frac(count, 10000, damon_nr_samples_per_aggr(attrs));
> >  }
> >  
> 
> [Severity: High]
> This is a pre-existing issue, but does this calculation silently overflow for
> large aggregation intervals?
> 
> When users configure large aggregation intervals (or very small sample
> intervals) such that samples_per_aggr exceeds 429,496, calculating the access
> frequency ratio overflows 32-bit math.
> 
> The mult_frac() macro calculates (count % samples) * 10000 using the 32-bit
> type of count. If samples_per_aggr > 429,496, the remainder can be large
> enough that multiplying by 10000 causes an overflow.
> 
> A similar 32-bit arithmetic overflow can happen in damon_sample_bp_to_count():
> 
>     return bp * damon_nr_samples_per_aggr(attrs) / 10000;

Known low priority issue.

> 
> > -static unsigned int damon_nr_accesses_for_new_attrs(unsigned int nr_accesses,
> > +static unsigned int damon_nr_samples_for_new_attrs(unsigned int nr_accesses,
> >  		struct damon_attrs *old_attrs, struct damon_attrs *new_attrs)
> >  {
> 
> [Severity: Low]
> Is the nr_accesses parameter name still appropriate here?
> 
> Since the function is being renamed to be used for general sample counts
> (like probe_hits) without confusion, leaving the first parameter named
> nr_accesses creates a semantic mismatch that limits the readability benefits
> of this refactoring.

Good catch.  I will fix in the next revision.

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


Thanks,
SJ

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

[SJ Park]

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