mm/damon: update probe hits for runtime parameter commits

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 <[email protected]>

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.

Quoted 4 lines

mm/damon/tests/core-kunit.h +0 -0

index 0124f83b39b83..224a3ecaa0701 100644

[ ... ]

Quoted 7 lines

mm/damon/tests/core-kunit.h +0 -3

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

mm/damon/tests/core-kunit.h static void damon_test_nr_accesses_to_accesses_bp(struct kunit *test) · line 585

Quoted 10 lines

mm/damon/tests/core-kunit.h +0 -9

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

Quoted 2 lines

mm/damon/tests/core-kunit.h +0 -0

 }

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

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 <[email protected]>

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.

mm/damon/core.c static unsigned int damon_age_for_new_attrs(unsigned int age, · line 884

Quoted 11 lines

mm/damon/core.c +1 -1

index a15005db82899..177e5abc981ef 100644
@@ -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.

mm/damon/core.c static unsigned int damon_age_for_new_attrs(unsigned int age, · line 891

Quoted 8 lines

mm/damon/core.c +1 -1

 /* 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.

Signature · 2 lines

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

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 <[email protected]>
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.

Quoted 4 lines

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?

mm/damon/core.c static unsigned int damon_sample_bp_to_count( · line 884

Quoted 8 lines

mm/damon/core.c +0 -0

index dea63083917b0..718268df27fa7 100644
@@ -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.

mm/damon/core.c static unsigned int damon_sample_bp_to_count( · line 891

Quoted 12 lines

mm/damon/core.c +4 -4

-/* 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.

[ ... ]

Quoted 5 lines

mm/damon/tests/core-kunit.h +0 -0

index 224a3ecaa0701..9e631b19b184a 100644
@@ -575,14 +575,14 @@ static void damon_test_set_regions(struct kunit *test)

[ ... ]

mm/damon/tests/core-kunit.h static void damon_test_set_regions(struct kunit *test) · line 583

Quoted 7 lines

mm/damon/tests/core-kunit.h +1 -1

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

Quoted 1 line

mm/damon/tests/core-kunit.h static void damon_test_set_regions(struct kunit *test) · line 586

Quoted 3 lines

mm/damon/tests/core-kunit.h +1 -1

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

Signature · 2 lines

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4

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 <[email protected]>

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.

mm/damon/core.c static unsigned int damon_sample_count_to_bp( · line 892

Quoted 8 lines

mm/damon/core.c +0 -0

index 718268df27fa7..cda318806b65b 100644
@@ -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;

mm/damon/core.c static unsigned int damon_sample_count_to_bp( · line 895

Quoted 5 lines

mm/damon/core.c +1 -1

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

Quoted 2 lines

mm/damon/core.c +0 -0

 	return damon_sample_bp_to_count(

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5

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 <[email protected]>

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.

Quoted 8 lines

mm/damon/core.c +0 -0

index cda318806b65b..9a3ff0fe55a61 100644
@@ -898,10 +898,32 @@ static unsigned int damon_nr_samples_for_new_attrs(unsigned int nr_accesses,
 			new_attrs);
 }
 

mm/damon/core.c static unsigned int damon_nr_samples_for_new_attrs(unsigned int nr_accesses, · line 910

Quoted 10 lines

mm/damon/core.c +10 -0

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

Quoted 10 lines

mm/damon/core.c +9 -0

+		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++;
+	}
+}

[ ... ]

Quoted 7 lines

mm/damon/tests/core-kunit.h +0 -0

index 9e631b19b184a..b4feef1a460f4 100644
@@ -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;

mm/damon/tests/core-kunit.h static void damon_test_update_monitoring_result(struct kunit *test) · line 602

Quoted 10 lines

mm/damon/tests/core-kunit.h +6 -0

 	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.

Signature · 2 lines

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6