[RFC PATCH 00/13] mm/damon: optimize out nr_accesses_bp

[RFC PATCH 00/13] mm/damon: optimize out nr_accesses_bp

[SeongJae Park]

TLDR: Replace damon_region->nr_accesses_bp, which is easy to be wrong,
with a simpler on-demand moving sum function, damon_nr_accesses_mvsum().

Background
==========

DAMON's monitoring output (access pattern snapshot, or more technically
speaking, damon_region->nr_accesses) is completed once per aggregation
interval, which is 100 ms by default.  Users can arbitrarily increase
the interval for demand.  Under the suggested intervals auto-tuning
setup, it can span up to 200 seconds.  If the aggregation interval is
too long, the snapshot users cannot use it in reasonable time.  To
mitigate this, we introduced a new field of damon_region, namely
nr_accesses_bp.  It contains a pseudo moving sum of nr_accesses in bp
units and is updated for each sampling interval.

It turned out keeping it correctly updated every sampling interval is
not that easy.  From online parameter update feature development and
more experimental hacks, we found it is easy to be corrupted.  Once it
is corrupted, DAMON's monitoring outputs become quite insane.  Hence we
added a few validation checks.  It is easy  to be corrupted because it
requires every update per sampling interval to be correct.

Solution
========

There is no real reason to keep it updated every sampling interval.  Due
to the simple pseudo-moving sum mechanism and existing helper field
(last_nr_accesses), we can also calculate the pseudo moving sum on
demand in a much simpler way.

Implement a function for getting the pseudo moving sum on demand, and
replace nr_accessses_bp uses with the new function.  Also remove no more
needed tests for nr_accesses_bp and the per-sampling interval update
functions.  Finally, remove the nr_accesses_bp.  The new function is
quite simple.

Discussion
==========

Depending on the use case, multiple nr_accesses readers could be
executed in the same kdamond_fn() main loop iteration, which is executed
once per sampling interval.  Such readers include DAMON region exporting
tracepoints (damon_[region_]aggregated and damos_before_apply), DAMOS,
and DAMON sysfs interface logic for update_schemes_tried_regions
command.  In this case, the new function will be called multiple times
and this could be overhead compared to the old logic, which simply reads
the field without any additional work.  Nonetheless, the new function is
quite simple.  And the new approach does nothing while there is no need
to read.  The old approach had to execute its update function for each
region for every sampling interval.  Hence the new approach is believed
to be even more lightweight in common case, and the overhead is anyway
negligible.

One more advantage of this change is that one field from the
damon_region struct is removed.  On setups that uses a high number of
DAMON regions, this could be a potential memory space benefit.

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

Patch 1 introduces the new function for getting the pseudo moving sum of
nr_accesses on demands.  Patch 2 implements a unit test for the new
function's internal logic.  Patches 3-5 replace uses of nr_accesses_bp
in DAMOS, tracepoints and DAMON sysfs interface with the new function,
respectively.  Patches 6-8 removes nr_accesses_bp validation functions
in DAMON core, one by one.  Patches 9 and 10 further remove tests and
test helper for nr_accesses_bp, respectively. Patches 11 removes the
setups and updates or nr_accesses_bp field.  Patch 12 removes the
function that was used for updating nr_accesses_bp field with its unit
test, which is the single remaining caller of the function.  Finally,
patch 13 removes damon_region->nr_accesses_bp field.

SeongJae Park (13):
  mm/damon: introduce damon_nr_accesses_mvsum()
  mm/damon/tests/core-kunit: test damon_mvsum()
  mm/damon/core: use damon_nr_accesses_mvsum() in __damos_valid_target()
  mm/damon/core: use damon_nr_accesses_mvsum() for damos region tracing
  mm/damon/sysfs-schemes: use damon_nr_accesses_mvsum() for damo regions
  mm/damon/core: remove damon_warn_fix_nr_accesses_corruption()
  mm/damon/core: remove damon_verify_reset_aggregated()
  mm/damon/core: remove damon_verify_merge_regions_of()
  mm/damon/tests/core-kunit: remove nr_accesses_bp setup and tests
  selftests/damon/drgn_dump_damon_status: do not dump nr_accesses_bp
  mm/damon/core: remove nr_accesses_bp setups and updates
  mm/damon/core: remove damon_moving_sum() and its unit test
  mm/damon: remove damon_region->nr_accesses_bp

 include/linux/damon.h                         |  12 +-
 include/trace/events/damon.h                  |   8 +-
 mm/damon/core.c                               | 171 +++++++-----------
 mm/damon/sysfs-schemes.c                      |   6 +-
 mm/damon/tests/core-kunit.h                   |  37 ++--
 .../selftests/damon/drgn_dump_damon_status.py |   1 -
 6 files changed, 91 insertions(+), 144 deletions(-)


base-commit: 6c6dd09066e98c4afcf61f404d5b2b87f35e3321
-- 
2.47.3

[RFC PATCH 01/13] mm/damon: introduce damon_nr_accesses_mvsum()

[SeongJae Park]

Introduce a new DAMON core function, damon_nr_accesses_mvsum().  It
returns a pseudo moving sum value of a given region's nr_accesses for
the last aggregation interval.  The internal logic is the same to
nr_accesses_bp.  The difference is that nr_accesses_bp is updated for
each sampling interval, while the new function needs to be executed only
when requested.  Hence the function's return value is the same as the
value of nr_accesses_bp.

Signed-off-by: SeongJae Park <sj@kernel.org>
---
 include/linux/damon.h |  2 ++
 mm/damon/core.c       | 55 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/include/linux/damon.h b/include/linux/damon.h
index 64d75c78f4df4..16e3e0910526b 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -1013,6 +1013,8 @@ struct damon_probe *damon_new_probe(void);
 void damon_add_probe(struct damon_ctx *ctx, struct damon_probe *probe);
 
 struct damon_region *damon_new_region(unsigned long start, unsigned long end);
+unsigned int damon_nr_accesses_mvsum(struct damon_region *r,
+		struct damon_ctx *ctx);
 
 int damon_set_regions(struct damon_target *t, struct damon_addr_range *ranges,
 		unsigned int nr_ranges, unsigned long min_region_sz);
diff --git a/mm/damon/core.c b/mm/damon/core.c
index 5b84b3ce3fcff..afb00f9bf7856 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -210,6 +210,61 @@ static struct damon_probe *damon_nth_probe(int n, struct damon_ctx *ctx)
 	return NULL;
 }
 
+/*
+ * damon_mvsum() - Returns pseudo moving sum value for a time window.
+ * @current_nr:		The value of the current aggregation window.
+ * @last_nr:		The value of the last aggregation window.
+ * @left_window_bp:	Left time of the current aggregation window.
+ *
+ * This function calculates a pseudo moving sum value of a counter that is
+ * aggregated for each time window.  @current_nr is the value of the counter
+ * that aggregated so far (maybe not yet complete), from the beginning of the
+ * current aggregation time window.  @last_nr is the value of the counter that
+ * has completely aggregated in the last aggregation time window.
+ * @left_window_bp represents how much time is left for the current aggregation
+ * time window in bp (1/10,000).  For example, the aggregation time window is
+ * for every 10 seconds and 7 seconds has passed since the beginning of the
+ * current window, this parameter will be 3000 ((10 - 7) / 10 * 10000).
+ *
+ * The logic assumes the aggregation in the last phase was made in a single
+ * speed.  Based on the assumption, the value from the last window that needs
+ * to be added to the current value is calculated as a portion of the last
+ * value based on the remaining time window.
+ */
+static unsigned long damon_mvsum(unsigned long current_nr,
+		unsigned long last_nr, unsigned long left_window_bp)
+{
+	return current_nr + mult_frac(last_nr, left_window_bp, 10000);
+}
+
+/**
+ * damon_nr_accesses_mvsum() - Returns moving sum access frequency score.
+ * @r:		Region to get the access frequency of.
+ * @ctx:	DAMON context of @r.
+ *
+ * This function returns for how many sampling iterations in the last
+ * aggregation interval (&damon_attrs->aggr_interval) the region was found to
+ * be accessed.  Hence the value can be interpreted as the relative access
+ * frequency score of the region (@r).  The value is calculated as a pseudo
+ * moving sum, and hence it is not an exact value but just a best-effort
+ * reasonable estimation.
+ *
+ * Return: the pseudo moving sum access frequency score.
+ */
+unsigned int damon_nr_accesses_mvsum(struct damon_region *r,
+		struct damon_ctx *ctx)
+{
+	unsigned long sample_interval, window_len, left_window, left_window_bp;
+
+	sample_interval = ctx->attrs.sample_interval ? : 1;
+	window_len = ctx->attrs.aggr_interval / sample_interval;
+	left_window = ctx->next_aggregation_sis - ctx->passed_sample_intervals;
+	left_window_bp = mult_frac(left_window, 10000, window_len);
+
+	return damon_mvsum(r->nr_accesses, r->last_nr_accesses,
+			left_window_bp);
+}
+
 #ifdef CONFIG_DAMON_DEBUG_SANITY
 static void damon_verify_new_region(unsigned long start, unsigned long end)
 {
-- 
2.47.3

[RFC PATCH 02/13] mm/damon/tests/core-kunit: test damon_mvsum()

[SeongJae Park]

Add a simple unit test for damon_nr_accesses_mvsum()'s internal core
logic, damon_mvsum().  The test contains cases for just-started windows,
partially completed windows, and just-completed windows.

Signed-off-by: SeongJae Park <sj@kernel.org>
---
 mm/damon/tests/core-kunit.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
index 4e448c08c724a..cdab14396250f 100644
--- a/mm/damon/tests/core-kunit.h
+++ b/mm/damon/tests/core-kunit.h
@@ -623,6 +623,30 @@ static void damon_test_moving_sum(struct kunit *test)
 	}
 }
 
+static void damon_test_mvsum(struct kunit *test)
+{
+	unsigned long input_expects[] = {
+		/* current value, last value, remaining window (bp) */
+		0, 49, 10000, 49,	/* 0 + 49 * 1 */
+		3, 10, 7000, 10,	/* 3 + 10 * 0.7 */
+		3, 10, 5000, 8,		/* 3 + 10 * 0.5 */
+		32, 100, 1000, 42,	/* 32 + 100 * 0.1 */
+		42, 49, 0, 42,		/* 42 + 49 * 0 */
+	};
+
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(input_expects); i += 4) {
+		unsigned long current_nr = input_expects[i];
+		unsigned long last_nr = input_expects[i + 1];
+		unsigned long left_window_bp = input_expects[i + 2];
+		unsigned long expect = input_expects[i + 3];
+
+		KUNIT_EXPECT_EQ(test, damon_mvsum(current_nr, last_nr,
+					left_window_bp), expect);
+	}
+}
+
 static void damos_test_new_filter(struct kunit *test)
 {
 	struct damos_filter *filter;
@@ -1501,6 +1525,7 @@ static struct kunit_case damon_test_cases[] = {
 	KUNIT_CASE(damon_test_update_monitoring_result),
 	KUNIT_CASE(damon_test_set_attrs),
 	KUNIT_CASE(damon_test_moving_sum),
+	KUNIT_CASE(damon_test_mvsum),
 	KUNIT_CASE(damos_test_new_filter),
 	KUNIT_CASE(damos_test_commit_quota_goal),
 	KUNIT_CASE(damos_test_commit_quota_goals),
-- 
2.47.3

[RFC PATCH 03/13] mm/damon/core: use damon_nr_accesses_mvsum() in __damos_valid_target()

[SeongJae Park]

damon_nr_accesses_mvsum() returns a value same to nr_accesses_bp.  Also
the function is more simple and therefore more tolerant to errors.
Execution of the function would be more expensive than the simple read
of the field, but because the function is quite simple, the overhead
should be negligible.  Use it in __damos_valid_target() instead of the
nr_accesses_bp.

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

diff --git a/mm/damon/core.c b/mm/damon/core.c
index afb00f9bf7856..3a6df10ba8548 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2118,10 +2118,11 @@ static noinline_for_stack void kdamond_tune_intervals(struct damon_ctx *c)
 	damon_set_attrs(c, &new_attrs);
 }
 
-static bool __damos_valid_target(struct damon_region *r, struct damos *s)
+static bool __damos_valid_target(struct damon_region *r, struct damos *s,
+		struct damon_ctx *c)
 {
 	unsigned long sz;
-	unsigned int nr_accesses = r->nr_accesses_bp / 10000;
+	unsigned int nr_accesses = damon_nr_accesses_mvsum(r, c);
 
 	sz = damon_sz_region(r);
 	return s->pattern.min_sz_region <= sz &&
@@ -2147,7 +2148,7 @@ static bool damos_quota_is_set(struct damos_quota *quota)
 static bool damos_valid_target(struct damon_ctx *c, struct damon_region *r,
 		struct damos *s)
 {
-	bool ret = __damos_valid_target(r, s);
+	bool ret = __damos_valid_target(r, s, c);
 
 	if (!ret || !damos_quota_is_set(&s->quota) || !c->ops.get_scheme_score)
 		return ret;
@@ -2733,7 +2734,7 @@ static phys_addr_t damos_calc_eligible_bytes(struct damon_ctx *c,
 		damon_for_each_region(r, t) {
 			phys_addr_t addr, end_addr;
 
-			if (!__damos_valid_target(r, s))
+			if (!__damos_valid_target(r, s, c))
 				continue;
 
 			/* Convert from core address units to physical bytes */
@@ -3022,7 +3023,7 @@ static void damos_adjust_quota(struct damon_ctx *c, struct damos *s)
 			(DAMOS_MAX_SCORE + 1));
 	damon_for_each_target(t, c) {
 		damon_for_each_region(r, t) {
-			if (!__damos_valid_target(r, s))
+			if (!__damos_valid_target(r, s, c))
 				continue;
 			if (damos_core_filter_out(c, t, r, s))
 				continue;
-- 
2.47.3

[RFC PATCH 04/13] mm/damon/core: use damon_nr_accesses_mvsum() for damos region tracing

[SeongJae Park]

damon_nr_accesses_mvsum() returns a value same to nr_accesses_bp.  Also
the function is more simple and therefore more tolerant to errors.
Execution of the function would be more expensive than the simple read
of the field, but because the function is quite simple, the overhead
should be negligible.  Use it in the DAMON region exporting trace points
instead of the nr_accesses_bp.

Signed-off-by: SeongJae Park <sj@kernel.org>
---
 include/trace/events/damon.h | 8 +++++---
 mm/damon/core.c              | 1 +
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/trace/events/damon.h b/include/trace/events/damon.h
index 78388538acf44..8851727ae1627 100644
--- a/include/trace/events/damon.h
+++ b/include/trace/events/damon.h
@@ -78,9 +78,11 @@ TRACE_EVENT_CONDITION(damos_before_apply,
 
 	TP_PROTO(unsigned int context_idx, unsigned int scheme_idx,
 		unsigned int target_idx, struct damon_region *r,
-		unsigned int nr_regions, bool do_trace),
+		unsigned int nr_accesses, unsigned int nr_regions,
+		bool do_trace),
 
-	TP_ARGS(context_idx, scheme_idx, target_idx, r, nr_regions, do_trace),
+	TP_ARGS(context_idx, scheme_idx, target_idx, r, nr_accesses,
+		nr_regions, do_trace),
 
 	TP_CONDITION(do_trace),
 
@@ -101,7 +103,7 @@ TRACE_EVENT_CONDITION(damos_before_apply,
 		__entry->target_idx = target_idx;
 		__entry->start = r->ar.start;
 		__entry->end = r->ar.end;
-		__entry->nr_accesses = r->nr_accesses_bp / 10000;
+		__entry->nr_accesses = nr_accesses;
 		__entry->age = r->age;
 		__entry->nr_regions = nr_regions;
 	),
diff --git a/mm/damon/core.c b/mm/damon/core.c
index 3a6df10ba8548..2eddf2674f4fa 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2463,6 +2463,7 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
 			return;
 		ktime_get_coarse_ts64(&begin);
 		trace_damos_before_apply(cidx, sidx, tidx, r,
+				damon_nr_accesses_mvsum(r, c),
 				damon_nr_regions(t), do_trace);
 		sz_applied = c->ops.apply_scheme(c, t, r, s,
 				&sz_ops_filter_passed);
-- 
2.47.3

[RFC PATCH 05/13] mm/damon/sysfs-schemes: use damon_nr_accesses_mvsum() for damo regions

[SeongJae Park]

damon_nr_accesses_mvsum() returns a value same to nr_accesses_bp.  Also
the function is more simple and therefore more tolerant to errors.
Execution of the function would be more expensive than the simple read
of the field, but because the function is quite simple, the overhead
should be negligible.  Use it in the DAMON sysfs interface for
scheme-tried regions, instead of the nr_accesses_bp.

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

diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
index 9d8fab32b80f7..30a007bcf82f4 100644
--- a/mm/damon/sysfs-schemes.c
+++ b/mm/damon/sysfs-schemes.c
@@ -159,7 +159,7 @@ struct damon_sysfs_scheme_region {
 };
 
 static struct damon_sysfs_scheme_region *damon_sysfs_scheme_region_alloc(
-		struct damon_region *region)
+		struct damon_region *region, struct damon_ctx *ctx)
 {
 	struct damon_sysfs_scheme_region *sysfs_region = kmalloc_obj(*sysfs_region);
 
@@ -167,7 +167,7 @@ static struct damon_sysfs_scheme_region *damon_sysfs_scheme_region_alloc(
 		return NULL;
 	sysfs_region->kobj = (struct kobject){};
 	sysfs_region->ar = region->ar;
-	sysfs_region->nr_accesses = region->nr_accesses_bp / 10000;
+	sysfs_region->nr_accesses = damon_nr_accesses_mvsum(region, ctx);
 	sysfs_region->age = region->age;
 	sysfs_region->probes = NULL;
 	INIT_LIST_HEAD(&sysfs_region->list);
@@ -3124,7 +3124,7 @@ void damos_sysfs_populate_region_dir(struct damon_sysfs_schemes *sysfs_schemes,
 	if (total_bytes_only)
 		return;
 
-	region = damon_sysfs_scheme_region_alloc(r);
+	region = damon_sysfs_scheme_region_alloc(r, ctx);
 	if (!region)
 		return;
 	region->sz_filter_passed = sz_filter_passed;
-- 
2.47.3

[RFC PATCH 06/13] mm/damon/core: remove damon_warn_fix_nr_accesses_corruption()

[SeongJae Park]

nr_accesses_bp is delicate.  Once it is corrupted, the consequence is
the bad madness of DAMON monitoring results.  From developments of
features of size, we historically found nr_accesses_bp can be corrupted
by complicated bugs that are not easy to debug.  Hence we added a
function for finding the corruption and fixing it right away.

There are no more uses of nr_accesses_bp.  Hence the function for
corruption detection and fix is no more needed.  Rip it out.

Signed-off-by: SeongJae Park <sj@kernel.org>
---
 mm/damon/core.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 2eddf2674f4fa..12d09d2f7ea1e 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -1992,19 +1992,6 @@ int damos_walk(struct damon_ctx *ctx, struct damos_walk_control *control)
 	return 0;
 }
 
-/*
- * Warn and fix corrupted ->nr_accesses[_bp] for investigations and preventing
- * the problem being propagated.
- */
-static void damon_warn_fix_nr_accesses_corruption(struct damon_region *r)
-{
-	if (r->nr_accesses_bp == r->nr_accesses * 10000)
-		return;
-	WARN_ONCE(true, "invalid nr_accesses_bp at reset: %u %u\n",
-			r->nr_accesses_bp, r->nr_accesses);
-	r->nr_accesses_bp = r->nr_accesses * 10000;
-}
-
 #ifdef CONFIG_DAMON_DEBUG_SANITY
 static void damon_verify_reset_aggregated(struct damon_region *r,
 		struct damon_ctx *c)
@@ -2046,7 +2033,6 @@ static void kdamond_reset_aggregated(struct damon_ctx *c)
 			trace_damon_aggregated(ti, r, damon_nr_regions(t));
 			trace_damon_region_aggregated(ti, r,
 					damon_nr_regions(t), nr_probes);
-			damon_warn_fix_nr_accesses_corruption(r);
 			r->last_nr_accesses = r->nr_accesses;
 			r->nr_accesses = 0;
 			for (i = 0; i < DAMON_MAX_PROBES; i++)
-- 
2.47.3

[RFC PATCH 07/13] mm/damon/core: remove damon_verify_reset_aggregated()

[SeongJae Park]

nr_accesse_bp is no longer being used in real use cases.  Remove its
validation function.

Signed-off-by: SeongJae Park <sj@kernel.org>
---
 mm/damon/core.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 12d09d2f7ea1e..9b8423056d879 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -1992,23 +1992,6 @@ int damos_walk(struct damon_ctx *ctx, struct damos_walk_control *control)
 	return 0;
 }
 
-#ifdef CONFIG_DAMON_DEBUG_SANITY
-static void damon_verify_reset_aggregated(struct damon_region *r,
-		struct damon_ctx *c)
-{
-	WARN_ONCE(r->nr_accesses_bp != r->last_nr_accesses * 10000,
-			"nr_accesses_bp %u last_nr_accesses %u sis %lu %lu\n",
-			r->nr_accesses_bp, r->last_nr_accesses,
-			c->passed_sample_intervals, c->next_aggregation_sis);
-}
-#else
-static void damon_verify_reset_aggregated(struct damon_region *r,
-		struct damon_ctx *c)
-{
-}
-#endif
-
-
 /*
  * Reset the aggregated monitoring results ('nr_accesses' of each region).
  */
@@ -2037,7 +2020,6 @@ static void kdamond_reset_aggregated(struct damon_ctx *c)
 			r->nr_accesses = 0;
 			for (i = 0; i < DAMON_MAX_PROBES; i++)
 				r->probe_hits[i] = 0;
-			damon_verify_reset_aggregated(r, c);
 		}
 		ti++;
 	}
-- 
2.47.3

[RFC PATCH 08/13] mm/damon/core: remove damon_verify_merge_regions_of()

[SeongJae Park]

damon_verify_merge_regions_of() is only for nr_accesses_bp validation.
But nr_accesses_bp is no more being used for a real purpose.  Remove the
validation.

Signed-off-by: SeongJae Park <sj@kernel.org>
---
 mm/damon/core.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 9b8423056d879..f2db39aa34f4c 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -3107,20 +3107,6 @@ static void damon_merge_two_regions(struct damon_target *t,
 	damon_destroy_region(r, t);
 }
 
-#ifdef CONFIG_DAMON_DEBUG_SANITY
-static void damon_verify_merge_regions_of(struct damon_region *r)
-{
-	WARN_ONCE(r->nr_accesses != r->nr_accesses_bp / 10000,
-			"nr_accesses (%u) != nr_accesses_bp (%u)\n",
-			r->nr_accesses, r->nr_accesses_bp);
-}
-#else
-static void damon_verify_merge_regions_of(struct damon_region *r)
-{
-}
-#endif
-
-
 /*
  * Merge adjacent regions having similar access frequencies
  *
@@ -3134,7 +3120,6 @@ static void damon_merge_regions_of(struct damon_target *t, unsigned int thres,
 	struct damon_region *r, *prev = NULL, *next;
 
 	damon_for_each_region_safe(r, next, t) {
-		damon_verify_merge_regions_of(r);
 		if (abs(r->nr_accesses - r->last_nr_accesses) > thres)
 			r->age = 0;
 		else if ((r->nr_accesses == 0) != (r->last_nr_accesses == 0))
-- 
2.47.3

[RFC PATCH 09/13] mm/damon/tests/core-kunit: remove nr_accesses_bp setup and tests

[SeongJae Park]

DAMON core unit tests set up nr_accesses_bp for representing realistic
damon_region, and also test the field.  nr_acceses_bp is no longer being
used for a real use case.  Remove the setup and tests.

Signed-off-by: SeongJae Park <sj@kernel.org>
---
 mm/damon/tests/core-kunit.h | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
index cdab14396250f..d1f019ab6fc25 100644
--- a/mm/damon/tests/core-kunit.h
+++ b/mm/damon/tests/core-kunit.h
@@ -118,7 +118,6 @@ static void damon_test_aggregate(struct kunit *test)
 				kunit_skip(test, "region alloc fail");
 			}
 			r->nr_accesses = accesses[it][ir];
-			r->nr_accesses_bp = accesses[it][ir] * 10000;
 			damon_add_region(r, t);
 		}
 		it++;
@@ -155,7 +154,6 @@ static void damon_test_split_at(struct kunit *test)
 		damon_free_target(t);
 		kunit_skip(test, "region alloc fail");
 	}
-	r->nr_accesses_bp = 420000;
 	r->nr_accesses = 42;
 	r->last_nr_accesses = 15;
 	r->age = 10;
@@ -168,7 +166,6 @@ static void damon_test_split_at(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, r_new->ar.start, 25ul);
 	KUNIT_EXPECT_EQ(test, r_new->ar.end, 100ul);
 
-	KUNIT_EXPECT_EQ(test, r->nr_accesses_bp, r_new->nr_accesses_bp);
 	KUNIT_EXPECT_EQ(test, r->nr_accesses, r_new->nr_accesses);
 	KUNIT_EXPECT_EQ(test, r->last_nr_accesses, r_new->last_nr_accesses);
 	KUNIT_EXPECT_EQ(test, r->age, r_new->age);
@@ -191,7 +188,6 @@ static void damon_test_merge_two(struct kunit *test)
 		kunit_skip(test, "region alloc fail");
 	}
 	r->nr_accesses = 10;
-	r->nr_accesses_bp = 100000;
 	r->age = 9;
 	damon_add_region(r, t);
 	r2 = damon_new_region(100, 300);
@@ -200,7 +196,6 @@ static void damon_test_merge_two(struct kunit *test)
 		kunit_skip(test, "second region alloc fail");
 	}
 	r2->nr_accesses = 20;
-	r2->nr_accesses_bp = 200000;
 	r2->age = 21;
 	damon_add_region(r2, t);
 
@@ -208,7 +203,6 @@ static void damon_test_merge_two(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, r->ar.start, 0ul);
 	KUNIT_EXPECT_EQ(test, r->ar.end, 300ul);
 	KUNIT_EXPECT_EQ(test, r->nr_accesses, 16u);
-	KUNIT_EXPECT_EQ(test, r->nr_accesses_bp, 160000u);
 	KUNIT_EXPECT_EQ(test, r->age, 17u);
 
 	i = 0;
@@ -256,7 +250,6 @@ static void damon_test_merge_regions_of(struct kunit *test)
 			kunit_skip(test, "region alloc fail");
 		}
 		r->nr_accesses = nrs[i];
-		r->nr_accesses_bp = nrs[i] * 10000;
 		damon_add_region(r, t);
 	}
 
@@ -556,7 +549,6 @@ static void damon_test_update_monitoring_result(struct kunit *test)
 		kunit_skip(test, "region alloc fail");
 
 	r->nr_accesses = 15;
-	r->nr_accesses_bp = 150000;
 	r->age = 20;
 
 	new_attrs = (struct damon_attrs){
-- 
2.47.3

[RFC PATCH 10/13] selftests/damon/drgn_dump_damon_status: do not dump nr_accesses_bp

[SeongJae Park]

drgn_dump_damon_status is dumping nr_accesses_bp field for future use
case.  nr_accesses_bp is not being used for a real purpose, though.
Hence there will be no future test for it.  Do not dump it.

Signed-off-by: SeongJae Park <sj@kernel.org>
---
 tools/testing/selftests/damon/drgn_dump_damon_status.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/testing/selftests/damon/drgn_dump_damon_status.py b/tools/testing/selftests/damon/drgn_dump_damon_status.py
index 26b207e44268d..09552e91bc782 100755
--- a/tools/testing/selftests/damon/drgn_dump_damon_status.py
+++ b/tools/testing/selftests/damon/drgn_dump_damon_status.py
@@ -59,7 +59,6 @@ def region_to_dict(region):
         ['ar', addr_range_to_dict],
         ['sampling_addr', int],
         ['nr_accesses', int],
-        ['nr_accesses_bp', int],
         ['age', int],
         ])
 
-- 
2.47.3

[RFC PATCH 11/13] mm/damon/core: remove nr_accesses_bp setups and updates

[SeongJae Park]

DAMON core sets and updates nr_accesses_bp in multiple places.  It
explains how delicate it is.  The field is no more being used for any
real purpose, and replaced by a simpler function.  Remove the setups and
updates.

Signed-off-by: SeongJae Park <sj@kernel.org>
---
 mm/damon/core.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index f2db39aa34f4c..eb129e3f7029d 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -294,7 +294,6 @@ struct damon_region *damon_new_region(unsigned long start, unsigned long end)
 	region->ar.start = start;
 	region->ar.end = end;
 	region->nr_accesses = 0;
-	region->nr_accesses_bp = 0;
 	for (i = 0; i < DAMON_MAX_PROBES; i++)
 		region->probe_hits[i] = 0;
 	INIT_LIST_HEAD(&region->list);
@@ -869,7 +868,6 @@ static void damon_update_monitoring_result(struct damon_region *r,
 	if (!aggregating) {
 		r->nr_accesses = damon_nr_accesses_for_new_attrs(
 				r->nr_accesses, old_attrs, new_attrs);
-		r->nr_accesses_bp = r->nr_accesses * 10000;
 	} else {
 		/*
 		 * if this is called in the middle of the aggregation, reset
@@ -879,7 +877,6 @@ static void damon_update_monitoring_result(struct damon_region *r,
 		 */
 		r->last_nr_accesses = damon_nr_accesses_for_new_attrs(
 				r->last_nr_accesses, old_attrs, new_attrs);
-		r->nr_accesses_bp = r->last_nr_accesses * 10000;
 		r->nr_accesses = 0;
 	}
 	r->age = damon_age_for_new_attrs(r->age, old_attrs, new_attrs);
@@ -3096,7 +3093,6 @@ static void damon_merge_two_regions(struct damon_target *t,
 
 	l->nr_accesses = (l->nr_accesses * sz_l + r->nr_accesses * sz_r) /
 			(sz_l + sz_r);
-	l->nr_accesses_bp = l->nr_accesses * 10000;
 	l->age = (l->age * sz_l + r->age * sz_r) / (sz_l + sz_r);
 	l->ar.end = r->ar.end;
 	/* todo: do this for only installed probes */
@@ -3208,7 +3204,6 @@ static void damon_split_region_at(struct damon_target *t,
 
 	new->age = r->age;
 	new->last_nr_accesses = r->last_nr_accesses;
-	new->nr_accesses_bp = r->nr_accesses_bp;
 	new->nr_accesses = r->nr_accesses;
 	/* todo: do this for only installed probes */
 	memcpy(new->probe_hits, r->probe_hits, sizeof(r->probe_hits));
@@ -3777,18 +3772,6 @@ static unsigned int damon_moving_sum(unsigned int mvsum, unsigned int nomvsum,
 void damon_update_region_access_rate(struct damon_region *r, bool accessed,
 		struct damon_attrs *attrs)
 {
-	unsigned int len_window = 1;
-
-	/*
-	 * sample_interval can be zero, but cannot be larger than
-	 * aggr_interval, owing to validation of damon_set_attrs().
-	 */
-	if (attrs->sample_interval)
-		len_window = damon_max_nr_accesses(attrs);
-	r->nr_accesses_bp = damon_moving_sum(r->nr_accesses_bp,
-			r->last_nr_accesses * 10000, len_window,
-			accessed ? 10000 : 0);
-
 	if (accessed)
 		r->nr_accesses++;
 }
-- 
2.47.3

[RFC PATCH 12/13] mm/damon/core: remove damon_moving_sum() and its unit test

[SeongJae Park]

damon_moving_sum() is no longer being called for real purpose but its
unit test.  Testing a function that is not being used for real users
makes no sense.  Remove the test and the function.

Signed-off-by: SeongJae Park <sj@kernel.org>
---
 mm/damon/core.c             | 40 -------------------------------------
 mm/damon/tests/core-kunit.h | 16 ---------------
 2 files changed, 56 deletions(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index eb129e3f7029d..1b78b2eb99e49 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -3718,46 +3718,6 @@ int damon_set_region_system_rams_default(struct damon_target *t,
 	return damon_set_regions(t, &addr_range, 1, min_region_sz);
 }
 
-/*
- * damon_moving_sum() - Calculate an inferred moving sum value.
- * @mvsum:	Inferred sum of the last @len_window values.
- * @nomvsum:	Non-moving sum of the last discrete @len_window window values.
- * @len_window:	The number of last values to take care of.
- * @new_value:	New value that will be added to the pseudo moving sum.
- *
- * Moving sum (moving average * window size) is good for handling noise, but
- * the cost of keeping past values can be high for arbitrary window size.  This
- * function implements a lightweight pseudo moving sum function that doesn't
- * keep the past window values.
- *
- * It simply assumes there was no noise in the past, and get the no-noise
- * assumed past value to drop from @nomvsum and @len_window.  @nomvsum is a
- * non-moving sum of the last window.  For example, if @len_window is 10 and we
- * have 25 values, @nomvsum is the sum of the 11th to 20th values of the 25
- * values.  Hence, this function simply drops @nomvsum / @len_window from
- * given @mvsum and add @new_value.
- *
- * For example, if @len_window is 10 and @nomvsum is 50, the last 10 values for
- * the last window could be vary, e.g., 0, 10, 0, 10, 0, 10, 0, 0, 0, 20.  For
- * calculating next moving sum with a new value, we should drop 0 from 50 and
- * add the new value.  However, this function assumes it got value 5 for each
- * of the last ten times.  Based on the assumption, when the next value is
- * measured, it drops the assumed past value, 5 from the current sum, and add
- * the new value to get the updated pseduo-moving average.
- *
- * This means the value could have errors, but the errors will be disappeared
- * for every @len_window aligned calls.  For example, if @len_window is 10, the
- * pseudo moving sum with 11th value to 19th value would have an error.  But
- * the sum with 20th value will not have the error.
- *
- * Return: Pseudo-moving average after getting the @new_value.
- */
-static unsigned int damon_moving_sum(unsigned int mvsum, unsigned int nomvsum,
-		unsigned int len_window, unsigned int new_value)
-{
-	return mvsum - nomvsum / len_window + new_value;
-}
-
 /**
  * damon_update_region_access_rate() - Update the access rate of a region.
  * @r:		The DAMON region to update for its access check result.
diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
index d1f019ab6fc25..df16cc070eeb0 100644
--- a/mm/damon/tests/core-kunit.h
+++ b/mm/damon/tests/core-kunit.h
@@ -600,21 +600,6 @@ static void damon_test_set_attrs(struct kunit *test)
 	damon_destroy_ctx(c);
 }
 
-static void damon_test_moving_sum(struct kunit *test)
-{
-	unsigned int mvsum = 50000, nomvsum = 50000, len_window = 10;
-	unsigned int new_values[] = {10000, 0, 10000, 0, 0, 0, 10000, 0, 0, 0};
-	unsigned int expects[] = {55000, 50000, 55000, 50000, 45000, 40000,
-		45000, 40000, 35000, 30000};
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(new_values); i++) {
-		mvsum = damon_moving_sum(mvsum, nomvsum, len_window,
-				new_values[i]);
-		KUNIT_EXPECT_EQ(test, mvsum, expects[i]);
-	}
-}
-
 static void damon_test_mvsum(struct kunit *test)
 {
 	unsigned long input_expects[] = {
@@ -1516,7 +1501,6 @@ static struct kunit_case damon_test_cases[] = {
 	KUNIT_CASE(damon_test_nr_accesses_to_accesses_bp),
 	KUNIT_CASE(damon_test_update_monitoring_result),
 	KUNIT_CASE(damon_test_set_attrs),
-	KUNIT_CASE(damon_test_moving_sum),
 	KUNIT_CASE(damon_test_mvsum),
 	KUNIT_CASE(damos_test_new_filter),
 	KUNIT_CASE(damos_test_commit_quota_goal),
-- 
2.47.3

[RFC PATCH 13/13] mm/damon: remove damon_region->nr_accesses_bp

[SeongJae Park]

No code touches damon_region->nr_accesses_bp field.  Remove it.

Signed-off-by: SeongJae Park <sj@kernel.org>
---
 include/linux/damon.h | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/include/linux/damon.h b/include/linux/damon.h
index 16e3e0910526b..cfd653c3e88cd 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -47,8 +47,6 @@ struct damon_size_range {
  * @ar:			The address range of the region.
  * @sampling_addr:	Address of the sample for the next access check.
  * @nr_accesses:	Access frequency of this region.
- * @nr_accesses_bp:	@nr_accesses in basis point (0.01%) that updated for
- *			each sampling interval.
  * @probe_hits:		Number of probe-positive region samples.
  * @list:		List head for siblings.
  * @age:		Age of this region.
@@ -61,13 +59,6 @@ struct damon_size_range {
  * not be done with direct access but with the helper function,
  * damon_update_region_access_rate().
  *
- * @nr_accesses_bp is another representation of @nr_accesses in basis point
- * (1 in 10,000) that updated for every &damon_attrs->sample_interval in a
- * manner similar to moving sum.  By the algorithm, this value becomes
- * @nr_accesses * 10000 for every &struct damon_attrs->aggr_interval.  This can
- * be used when the aggregation interval is too huge and therefore cannot wait
- * for it before getting the access monitoring results.
- *
  * @age is initially zero, increased for each aggregation interval, and reset
  * to zero again if the access frequency is significantly changed.  If two
  * regions are merged into a new region, both @nr_accesses and @age of the new
@@ -77,7 +68,6 @@ struct damon_region {
 	struct damon_addr_range ar;
 	unsigned long sampling_addr;
 	unsigned int nr_accesses;
-	unsigned int nr_accesses_bp;
 	unsigned char probe_hits[DAMON_MAX_PROBES];
 	struct list_head list;
 
-- 
2.47.3

Re: [RFC PATCH 01/13] mm/damon: introduce damon_nr_accesses_mvsum()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Kernel panic due to division by zero in `damon_nr_accesses_mvsum()` when `aggr_interval` is configured to 0.
- [Medium] Integer overflow in `mult_frac()` leading to severely incorrect access scoring on 32-bit systems when `window_len` is large.
--

commit 0c6de91631a66fe41c1e81f68011cf31d86b4825
Author: SeongJae Park <sj@kernel.org>

mm/damon: introduce damon_nr_accesses_mvsum()

This commit introduces damon_nr_accesses_mvsum() to calculate a pseudo
moving sum of a given region's nr_accesses for the last aggregation
interval. The function is designed to be executed on-demand rather than
at every sampling interval.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
[ ... ]
> +unsigned int damon_nr_accesses_mvsum(struct damon_region *r,
> +		struct damon_ctx *ctx)
> +{
> +	unsigned long sample_interval, window_len, left_window, left_window_bp;
> +
> +	sample_interval = ctx->attrs.sample_interval ? : 1;
> +	window_len = ctx->attrs.aggr_interval / sample_interval;
> +	left_window = ctx->next_aggregation_sis - ctx->passed_sample_intervals;
> +	left_window_bp = mult_frac(left_window, 10000, window_len);

[Severity: High]
What happens here if aggr_interval is configured to 0?

If userspace configures both aggr_interval and sample_interval to 0
via sysfs, sample_interval evaluates to 1 through the fallback, but
window_len becomes 0.

When window_len is passed as the denominator to mult_frac(), does this
result in a divide-by-zero kernel panic?

[Severity: Medium]
Is there a risk of an integer overflow here on 32-bit systems?

The mult_frac(x, n, d) macro evaluates the fractional part using the type
of x, which is an unsigned long here. On 32-bit architectures, this has
a maximum value of ~4.29 billion.

If a 5-second aggregation interval and 1-microsecond sample interval are
configured, window_len and left_window could reach 5,000,000.

The intermediate multiplication in mult_frac():
(left_window % window_len) * 10000

could evaluate to around 50,000,000,000, which overflows a 32-bit
unsigned long. Could this lead to inaccurate access frequency calculations?

> +
> +	return damon_mvsum(r->nr_accesses, r->last_nr_accesses,
> +			left_window_bp);
> +}

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

Re: [RFC PATCH 06/13] mm/damon/core: remove damon_warn_fix_nr_accesses_corruption()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] damon_nr_accesses_mvsum() causes a divide-by-zero kernel panic if aggr_interval is configured to 0.
--

commit 6a4a453ed1f9a5204defaea4d03c5096a0240cc8
Author: SeongJae Park <sj@kernel.org>

mm/damon/core: remove damon_warn_fix_nr_accesses_corruption()

This commit removes the damon_warn_fix_nr_accesses_corruption function
and its call site in kdamond_reset_aggregated. This removes the
corruption detection and fix logic for nr_accesses_bp since it is no
longer used.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index a7b45b3cad508..35a4f22a35f7e 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c

[ ... ]

> @@ -2046,7 +2033,6 @@ static void kdamond_reset_aggregated(struct damon_ctx *c)
>  			trace_damon_aggregated(ti, r, damon_nr_regions(t));
>  			trace_damon_region_aggregated(ti, r,
>  					damon_nr_regions(t), nr_probes);
> -			damon_warn_fix_nr_accesses_corruption(r);
>  			r->last_nr_accesses = r->nr_accesses;
>  			r->nr_accesses = 0;
>  			for (i = 0; i < DAMON_MAX_PROBES; i++)

[Severity: Critical]
This isn't a bug introduced by this patch, but by a previous patch in the
series (mm/damon: introduce damon_nr_accesses_mvsum). Does the
damon_nr_accesses_mvsum() function cause a divide-by-zero if aggr_interval is
configured to 0?

In damon_nr_accesses_mvsum(), window_len is calculated as:

    window_len = ctx->attrs.aggr_interval / sample_interval;
    left_window = ctx->next_aggregation_sis - ctx->passed_sample_intervals;
    left_window_bp = mult_frac(left_window, 10000, window_len);

If a user configures aggr_interval to 0 (and sample_interval to 0, which
passes the sample_interval > aggr_interval validation in damon_set_attrs()),
window_len evaluates to 0.

Because mult_frac() directly performs a division by this parameter, wouldn't
this result in a divide-by-zero exception and a kernel panic?

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

Re: [RFC PATCH 05/13] mm/damon/sysfs-schemes: use damon_nr_accesses_mvsum() for damo regions

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Calling `damon_nr_accesses_mvsum()` triggers a division by zero kernel panic when `aggr_interval` and `sample_interval` are configured to 0.
- [Medium] Integer overflow in `damon_nr_accesses_mvsum()` on 32-bit architectures when `window_len` is large.
--

commit 66c2d8d0e11cf5ef0032d41b0f9dcf8d2fd41a3b
Author: SeongJae Park <sj@kernel.org>

mm/damon/sysfs-schemes: use damon_nr_accesses_mvsum() for damo regions

This commit updates the DAMON sysfs interface to use 
damon_nr_accesses_mvsum() for scheme-tried regions instead of reading 
nr_accesses_bp directly.

> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> index 329cfd0bbe9f3..e648b0f809332 100644
> --- a/mm/damon/sysfs-schemes.c
> +++ b/mm/damon/sysfs-schemes.c
> @@ -159,7 +159,7 @@ struct damon_sysfs_scheme_region {
>  };
>  
>  static struct damon_sysfs_scheme_region *damon_sysfs_scheme_region_alloc(
> -		struct damon_region *region)
> +		struct damon_region *region, struct damon_ctx *ctx)
>  {
>  	struct damon_sysfs_scheme_region *sysfs_region = kmalloc_obj(*sysfs_region);
>  
> @@ -167,7 +167,7 @@ static struct damon_sysfs_scheme_region *damon_sysfs_scheme_region_alloc(
>  		return NULL;
>  	sysfs_region->kobj = (struct kobject){};
>  	sysfs_region->ar = region->ar;
> -	sysfs_region->nr_accesses = region->nr_accesses_bp / 10000;
> +	sysfs_region->nr_accesses = damon_nr_accesses_mvsum(region, ctx);

[Severity: High]
Could this new call trigger a divide-by-zero kernel panic?

If a user configures both aggr_interval and sample_interval to 0 via the
DAMON sysfs interface, damon_set_attrs() accepts these values. During the
calculation of the moving sum in damon_nr_accesses_mvsum(), window_len
evaluates to 0:

mm/damon/core.c:damon_nr_accesses_mvsum() {
    sample_interval = ctx->attrs.sample_interval ? : 1;
    window_len = ctx->attrs.aggr_interval / sample_interval;
    ...
    left_window_bp = mult_frac(left_window, 10000, window_len);
    ...
}

Since window_len becomes 0/1 = 0, the subsequent call to mult_frac()
performs a division by window_len, which would crash the system.

[Severity: Medium]
Also, does this calculation cause an integer overflow on 32-bit architectures?

The mult_frac macro expands to (rem * 10000) / window_len. On 32-bit
systems where unsigned long is 32 bits, if the aggregation interval is large
and sample interval is small, window_len can exceed 429,496.

As rem approaches window_len, the intermediate multiplication (rem * 10000)
exceeds ULONG_MAX and wraps around, corrupting the moving sum access
statistics.

>  	sysfs_region->age = region->age;
>  	sysfs_region->probes = NULL;
>  	INIT_LIST_HEAD(&sysfs_region->list);

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

Re: [RFC PATCH 13/13] mm/damon: remove damon_region->nr_accesses_bp

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Stale comment in kdamond_fn() references the removed nr_accesses_bp field.
--

commit 7fe8c1a02c74da4804760b45a04f15189947a500
Author: SeongJae Park <sj@kernel.org>

mm/damon: remove damon_region->nr_accesses_bp

This commit removes the unused nr_accesses_bp field from struct damon_region,
as well as its associated kernel-doc comments.

> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index 4f73d27979f0b..a18d905f28e14 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
[ ... ]
> @@ -77,7 +68,6 @@ struct damon_region {
>  	struct damon_addr_range ar;
>  	unsigned long sampling_addr;
>  	unsigned int nr_accesses;
> -	unsigned int nr_accesses_bp;

[Severity: Low]
Is this a minor regression that leaves a stale comment in kdamond_fn()?

Looking at mm/damon/core.c, kdamond_fn() still explicitly references the
nr_accesses_bp field:

mm/damon/core.c:kdamond_fn() {
    ...
				 * information reset for all regions.  Then,
				 * following kdamond_reset_aggregated() call
				 * will make the region information invalid,
				 * particularly for ->nr_accesses_bp.
    ...
}

Should this comment in mm/damon/core.c be updated or removed to reflect the
field's deletion?

>  	unsigned char probe_hits[DAMON_MAX_PROBES];
>  	struct list_head list;

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

Re: [RFC PATCH 03/13] mm/damon/core: use damon_nr_accesses_mvsum() in __damos_valid_target()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Divide-by-zero kernel panic in damon_nr_accesses_mvsum() when aggr_interval is configured to 0.
--

commit 9675c26c9e91a5b3df67520a05b7e2a8a6a589a7
Author: SeongJae Park <sj@kernel.org>

mm/damon/core: use damon_nr_accesses_mvsum() in __damos_valid_target()

This commit modifies __damos_valid_target() to use the helper function
damon_nr_accesses_mvsum() instead of directly reading the nr_accesses_bp
field.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index bc8b102d700a7..f45f030a72d82 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -2118,10 +2118,11 @@ static void kdamond_tune_intervals(struct damon_ctx *c)
>  	damon_set_attrs(c, &new_attrs);
>  }
>  
> -static bool __damos_valid_target(struct damon_region *r, struct damos *s)
> +static bool __damos_valid_target(struct damon_region *r, struct damos *s,
> +		struct damon_ctx *c)
>  {
>  	unsigned long sz;
> -	unsigned int nr_accesses = r->nr_accesses_bp / 10000;
> +	unsigned int nr_accesses = damon_nr_accesses_mvsum(r, c);

[Severity: High]
Does this introduce a divide-by-zero risk if aggr_interval is configured to 0?

If an administrator writes 0 to both sample_us and aggr_us via the
DAMON sysfs interface, damon_set_attrs() allows it because the
validation check (attrs->sample_interval > attrs->aggr_interval)
evaluates to false (0 > 0).

When __damos_valid_target() evaluates active schemes, it now executes
damon_nr_accesses_mvsum(). Inside that function, window_len is calculated
like this:

damon_nr_accesses_mvsum() {
	...
	sample_interval = ctx->attrs.sample_interval ? : 1;
	window_len = ctx->attrs.aggr_interval / sample_interval;
	left_window = ctx->next_aggregation_sis - ctx->passed_sample_intervals;
	left_window_bp = mult_frac(left_window, 10000, window_len);
	...
}

If aggr_interval is 0, window_len evaluates to 0. It is then passed
as the denominator to mult_frac().

Could this result in an exact integer division by zero panic, and should
the zero-values be protected against here or in the configuration interfaces?

[ ... ]

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

Re: [RFC PATCH 04/13] mm/damon/core: use damon_nr_accesses_mvsum() for damos region tracing

[sashiko-bot]

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

New issues:
- [Medium] Unconditional evaluation of an expensive function (`damon_nr_accesses_mvsum`) as an argument to a tracepoint, causing performance overhead when tracing is disabled.

Pre-existing issues:
- [Critical] Pre-existing vulnerability where setting aggr_interval to 0 triggers a divide-by-zero kernel panic in damon_nr_accesses_mvsum().
--

commit a32ac0de542ee6f63cc5519f32be214de8a6347f
Author: SeongJae Park <sj@kernel.org>

mm/damon/core: use damon_nr_accesses_mvsum() for damos region tracing

damon_nr_accesses_mvsum() returns a value same to nr_accesses_bp.  Also
the function is more simple and therefore more tolerant to errors.
Execution of the function would be more expensive than the simple read
of the field, but because the function is quite simple, the overhead
should be negligible.  Use it in the DAMON region exporting trace points
instead of the nr_accesses_bp.

Signed-off-by: SeongJae Park <sj@kernel.org>

> diff --git a/include/trace/events/damon.h b/include/trace/events/damon.h
> index 78388538acf44..8851727ae1627 100644
> --- a/include/trace/events/damon.h
> +++ b/include/trace/events/damon.h
> @@ -78,9 +78,11 @@ TRACE_EVENT_CONDITION(damos_before_apply,
>  
>  	TP_PROTO(unsigned int context_idx, unsigned int scheme_idx,
>  		unsigned int target_idx, struct damon_region *r,
> -		unsigned int nr_regions, bool do_trace),
> +		unsigned int nr_accesses, unsigned int nr_regions,
> +		bool do_trace),

[ ... ]

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index f45f030a72d82..a7b45b3cad508 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -2463,6 +2463,7 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
>  			return;
>  		ktime_get_coarse_ts64(&begin);
>  		trace_damos_before_apply(cidx, sidx, tidx, r,
> +				damon_nr_accesses_mvsum(r, c),
>  				damon_nr_regions(t), do_trace);

[Severity: Medium]
Will this unconditionally evaluate damon_nr_accesses_mvsum() even when
tracing is disabled?

Because trace_damos_before_apply() is an inline function wrapper, its
arguments are evaluated before the function is called. This means
damon_nr_accesses_mvsum() will execute on the hot path for every valid
region, regardless of whether the tracepoint is active. It also causes
the function to be evaluated a second time per region, since it was
already called in __damos_valid_target().

Would it be better to pass the DAMON context pointer c into the tracepoint
prototype and defer the execution of damon_nr_accesses_mvsum() entirely to
TP_fast_assign() to avoid this overhead?


[Severity: Critical]
This is a pre-existing issue, but does damon_nr_accesses_mvsum() have a
divide-by-zero vulnerability when userspace configures both sample_interval
and aggr_interval to 0?

Userspace can configure DAMON via sysfs. In damon_set_attrs(), the validation
logic accepts configurations where both intervals are 0:

mm/damon/core.c:damon_set_attrs() {
    ...
	if (attrs->sample_interval > attrs->aggr_interval)
		return -EINVAL;
    ...
}

Since 0 > 0 evaluates to false, this configuration is accepted.

When damon_nr_accesses_mvsum() is subsequently called, it computes
window_len = ctx->attrs.aggr_interval / sample_interval, which evaluates to 0.
It then calls mult_frac(left_window, 10000, window_len), which divides
by window_len and will cause an immediate kernel panic.

>  		sz_applied = c->ops.apply_scheme(c, t, r, s,
>  				&sz_ops_filter_passed);

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

Re: [RFC PATCH 09/13] mm/damon/tests/core-kunit: remove nr_accesses_bp setup and tests

[sashiko-bot]

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

New issues:
- [High] Divide-by-zero in `damon_nr_accesses_mvsum()` when `aggr_interval` is 0.

Pre-existing issues:
- [High] Divide-by-zero in `damon_max_nr_accesses()` when `sample_interval` is 0.
--

commit 6519e9a3b09dd0793b79fa00b085d57ba68bc353
Author: SeongJae Park <sj@kernel.org>

mm/damon/tests/core-kunit: remove nr_accesses_bp setup and tests
    
DAMON core unit tests set up nr_accesses_bp for representing realistic
damon_region, and also test the field.  nr_acceses_bp is no longer being
used for a real use case.  Remove the setup and tests.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -210,6 +210,61 @@ static struct damon_probe *damon_nth_probe(int n, struct damon_ctx *ctx)
[ ... ]
> +unsigned int damon_nr_accesses_mvsum(struct damon_region *r,
> +		struct damon_ctx *ctx)
> +{
> +	unsigned long sample_interval, window_len, left_window, left_window_bp;
> +
> +	sample_interval = ctx->attrs.sample_interval ? : 1;
> +	window_len = ctx->attrs.aggr_interval / sample_interval;
> +	left_window = ctx->next_aggregation_sis - ctx->passed_sample_intervals;
> +	left_window_bp = mult_frac(left_window, 10000, window_len);
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: High]
If `ctx->attrs.aggr_interval` is 0, `window_len` evaluates to 0.

Since `damon_set_attrs()` accepts configurations where both `sample_us` and
`aggr_us` are 0, does this lead to a divide-by-zero here?

When the kdamond thread evaluates user-defined schemes, it appears to
unconditionally call __damos_valid_target() -> damon_nr_accesses_mvsum().

Could this regression be triggered via the DAMON sysfs interface to cause
a crash?

> +
> +	return damon_mvsum(r->nr_accesses, r->last_nr_accesses,
> +			left_window_bp);
> +}

include/linux/damon.h:damon_max_nr_accesses() {
    ...
    static inline unsigned int damon_max_nr_accesses(const struct damon_attrs *attrs)
    {
        /* {aggr,sample}_interval are unsigned long, hence could overflow */
        return min(attrs->aggr_interval / attrs->sample_interval,
                (unsigned long)UINT_MAX);
    }
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but there is a pre-existing
divide-by-zero risk if `sample_interval` is 0.

Does this calculation cause an immediate divide-by-zero exception?

Userspace can configure `sample_us` to 0 via sysfs, which avoids validation
in `damon_set_attrs()` if `aggr_us` is also 0.

Since `damon_get_scheme_score()` unconditionally invokes
`damon_max_nr_accesses()` when evaluating user-defined schemes, could this
allow a user to intentionally crash the kernel?

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

Re: [RFC PATCH 01/13] mm/damon: introduce damon_nr_accesses_mvsum()

[SeongJae Park]

On Fri, 19 Jun 2026 19:44:28 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Kernel panic due to division by zero in `damon_nr_accesses_mvsum()` when `aggr_interval` is configured to 0.
> - [Medium] Integer overflow in `mult_frac()` leading to severely incorrect access scoring on 32-bit systems when `window_len` is large.
> --
> 
> commit 0c6de91631a66fe41c1e81f68011cf31d86b4825
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon: introduce damon_nr_accesses_mvsum()
> 
> This commit introduces damon_nr_accesses_mvsum() to calculate a pseudo
> moving sum of a given region's nr_accesses for the last aggregation
> interval. The function is designed to be executed on-demand rather than
> at every sampling interval.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> [ ... ]
> > +unsigned int damon_nr_accesses_mvsum(struct damon_region *r,
> > +		struct damon_ctx *ctx)
> > +{
> > +	unsigned long sample_interval, window_len, left_window, left_window_bp;
> > +
> > +	sample_interval = ctx->attrs.sample_interval ? : 1;
> > +	window_len = ctx->attrs.aggr_interval / sample_interval;
> > +	left_window = ctx->next_aggregation_sis - ctx->passed_sample_intervals;
> > +	left_window_bp = mult_frac(left_window, 10000, window_len);
> 
> [Severity: High]
> What happens here if aggr_interval is configured to 0?
> 
> If userspace configures both aggr_interval and sample_interval to 0
> via sysfs, sample_interval evaluates to 1 through the fallback, but
> window_len becomes 0.
> 
> When window_len is passed as the denominator to mult_frac(), does this
> result in a divide-by-zero kernel panic?

Good catch.  I will add a handling of the case in the next revision.

> 
> [Severity: Medium]
> Is there a risk of an integer overflow here on 32-bit systems?
> 
> The mult_frac(x, n, d) macro evaluates the fractional part using the type
> of x, which is an unsigned long here. On 32-bit architectures, this has
> a maximum value of ~4.29 billion.
> 
> If a 5-second aggregation interval and 1-microsecond sample interval are
> configured, window_len and left_window could reach 5,000,000.
> 
> The intermediate multiplication in mult_frac():
> (left_window % window_len) * 10000
> 
> could evaluate to around 50,000,000,000, which overflows a 32-bit
> unsigned long. Could this lead to inaccurate access frequency calculations?

I don't think that's a sane input that need a support.  We can revisit if a
real user report from this is made.


Thanks,
SJ

[...]

Re: [RFC PATCH 05/13] mm/damon/sysfs-schemes: use damon_nr_accesses_mvsum() for damo regions

[SeongJae Park]

On Fri, 19 Jun 2026 19:47:50 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Calling `damon_nr_accesses_mvsum()` triggers a division by zero kernel panic when `aggr_interval` and `sample_interval` are configured to 0.

I will fix this in the next revision.

> - [Medium] Integer overflow in `damon_nr_accesses_mvsum()` on 32-bit architectures when `window_len` is large.

I will ignore this until a user claims this as an issue.

For more details: https://lore.kernel.org/20260619195232.26053-1-sj@kernel.org


Thanks,
SJ

[...]

Re: [RFC PATCH 06/13] mm/damon/core: remove damon_warn_fix_nr_accesses_corruption()

[SeongJae Park]

On Fri, 19 Jun 2026 19:47:44 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Critical] damon_nr_accesses_mvsum() causes a divide-by-zero kernel panic if aggr_interval is configured to 0.

I will fix this in the next revision.  For more details:
https://lore.kernel.org/20260619195232.26053-1-sj@kernel.org


Thanks,
SJ

[...]

Re: [RFC PATCH 04/13] mm/damon/core: use damon_nr_accesses_mvsum() for damos region tracing

[SeongJae Park]

On Fri, 19 Jun 2026 19:51:40 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Medium] Unconditional evaluation of an expensive function (`damon_nr_accesses_mvsum`) as an argument to a tracepoint, causing performance overhead when tracing is disabled.
> 
> Pre-existing issues:
> - [Critical] Pre-existing vulnerability where setting aggr_interval to 0 triggers a divide-by-zero kernel panic in damon_nr_accesses_mvsum().
> --
> 
> commit a32ac0de542ee6f63cc5519f32be214de8a6347f
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/core: use damon_nr_accesses_mvsum() for damos region tracing
> 
> damon_nr_accesses_mvsum() returns a value same to nr_accesses_bp.  Also
> the function is more simple and therefore more tolerant to errors.
> Execution of the function would be more expensive than the simple read
> of the field, but because the function is quite simple, the overhead
> should be negligible.  Use it in the DAMON region exporting trace points
> instead of the nr_accesses_bp.
> 
> Signed-off-by: SeongJae Park <sj@kernel.org>
> 
> > diff --git a/include/trace/events/damon.h b/include/trace/events/damon.h
> > index 78388538acf44..8851727ae1627 100644
> > --- a/include/trace/events/damon.h
> > +++ b/include/trace/events/damon.h
> > @@ -78,9 +78,11 @@ TRACE_EVENT_CONDITION(damos_before_apply,
> >  
> >  	TP_PROTO(unsigned int context_idx, unsigned int scheme_idx,
> >  		unsigned int target_idx, struct damon_region *r,
> > -		unsigned int nr_regions, bool do_trace),
> > +		unsigned int nr_accesses, unsigned int nr_regions,
> > +		bool do_trace),
> 
> [ ... ]
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index f45f030a72d82..a7b45b3cad508 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -2463,6 +2463,7 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
> >  			return;
> >  		ktime_get_coarse_ts64(&begin);
> >  		trace_damos_before_apply(cidx, sidx, tidx, r,
> > +				damon_nr_accesses_mvsum(r, c),
> >  				damon_nr_regions(t), do_trace);
> 
> [Severity: Medium]
> Will this unconditionally evaluate damon_nr_accesses_mvsum() even when
> tracing is disabled?
> 
> Because trace_damos_before_apply() is an inline function wrapper, its
> arguments are evaluated before the function is called. This means
> damon_nr_accesses_mvsum() will execute on the hot path for every valid
> region, regardless of whether the tracepoint is active. It also causes
> the function to be evaluated a second time per region, since it was
> already called in __damos_valid_target().
> 
> Would it be better to pass the DAMON context pointer c into the tracepoint
> prototype and defer the execution of damon_nr_accesses_mvsum() entirely to
> TP_fast_assign() to avoid this overhead?

Nice catch.  I will execute the function in trace_damos_before_apply_enabled()
scope of the function, like below.

'''
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2711,7 +2711,7 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
        struct damos *siter;            /* schemes iterator */
        unsigned int sidx = 0;
        struct damon_target *titer;     /* targets iterator */
-       unsigned int tidx = 0;
+       unsigned int tidx = 0, nr_accesses = 0;
        bool do_trace = false;

        /* get indices for trace_damos_before_apply() */
@@ -2726,6 +2726,7 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
                                break;
                        tidx++;
                }
+               nr_accesses = damon_nr_accesses_mvsum(r, c);
                do_trace = true;
        }

@@ -2741,8 +2742,7 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
                if (damos_core_filter_out(c, t, r, s))
                        return;
                ktime_get_coarse_ts64(&begin);
-               trace_damos_before_apply(cidx, sidx, tidx, r,
-                               damon_nr_accesses_mvsum(r, c),
+               trace_damos_before_apply(cidx, sidx, tidx, r, nr_accesses,
                                damon_nr_regions(t), do_trace);
                sz_applied = c->ops.apply_scheme(c, t, r, s,
                                &sz_ops_filter_passed);
'''

> 
> 
> [Severity: Critical]
> This is a pre-existing issue, but does damon_nr_accesses_mvsum() have a
> divide-by-zero vulnerability when userspace configures both sample_interval
> and aggr_interval to 0?
> 
> Userspace can configure DAMON via sysfs. In damon_set_attrs(), the validation
> logic accepts configurations where both intervals are 0:
> 
> mm/damon/core.c:damon_set_attrs() {
>     ...
> 	if (attrs->sample_interval > attrs->aggr_interval)
> 		return -EINVAL;
>     ...
> }
> 
> Since 0 > 0 evaluates to false, this configuration is accepted.
> 
> When damon_nr_accesses_mvsum() is subsequently called, it computes
> window_len = ctx->attrs.aggr_interval / sample_interval, which evaluates to 0.
> It then calls mult_frac(left_window, 10000, window_len), which divides
> by window_len and will cause an immediate kernel panic.

I will fix this in the next revision.


Thanks,
SJ

[...]

Re: [RFC PATCH 09/13] mm/damon/tests/core-kunit: remove nr_accesses_bp setup and tests

[SeongJae Park]

On Fri, 19 Jun 2026 19:52:31 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] Divide-by-zero in `damon_nr_accesses_mvsum()` when `aggr_interval` is 0.
> 
> Pre-existing issues:
> - [High] Divide-by-zero in `damon_max_nr_accesses()` when `sample_interval` is 0.
> --
> 
> commit 6519e9a3b09dd0793b79fa00b085d57ba68bc353
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/tests/core-kunit: remove nr_accesses_bp setup and tests
>     
> DAMON core unit tests set up nr_accesses_bp for representing realistic
> damon_region, and also test the field.  nr_acceses_bp is no longer being
> used for a real use case.  Remove the setup and tests.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -210,6 +210,61 @@ static struct damon_probe *damon_nth_probe(int n, struct damon_ctx *ctx)
> [ ... ]
> > +unsigned int damon_nr_accesses_mvsum(struct damon_region *r,
> > +		struct damon_ctx *ctx)
> > +{
> > +	unsigned long sample_interval, window_len, left_window, left_window_bp;
> > +
> > +	sample_interval = ctx->attrs.sample_interval ? : 1;
> > +	window_len = ctx->attrs.aggr_interval / sample_interval;
> > +	left_window = ctx->next_aggregation_sis - ctx->passed_sample_intervals;
> > +	left_window_bp = mult_frac(left_window, 10000, window_len);
>                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> [Severity: High]
> If `ctx->attrs.aggr_interval` is 0, `window_len` evaluates to 0.
> 
> Since `damon_set_attrs()` accepts configurations where both `sample_us` and
> `aggr_us` are 0, does this lead to a divide-by-zero here?
> 
> When the kdamond thread evaluates user-defined schemes, it appears to
> unconditionally call __damos_valid_target() -> damon_nr_accesses_mvsum().
> 
> Could this regression be triggered via the DAMON sysfs interface to cause
> a crash?

Yes, I will fix this in the next revision.

> 
> > +
> > +	return damon_mvsum(r->nr_accesses, r->last_nr_accesses,
> > +			left_window_bp);
> > +}
> 
> include/linux/damon.h:damon_max_nr_accesses() {
>     ...
>     static inline unsigned int damon_max_nr_accesses(const struct damon_attrs *attrs)
>     {
>         /* {aggr,sample}_interval are unsigned long, hence could overflow */
>         return min(attrs->aggr_interval / attrs->sample_interval,
>                 (unsigned long)UINT_MAX);
>     }
>     ...
> }
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but there is a pre-existing
> divide-by-zero risk if `sample_interval` is 0.
> 
> Does this calculation cause an immediate divide-by-zero exception?
> 
> Userspace can configure `sample_us` to 0 via sysfs, which avoids validation
> in `damon_set_attrs()` if `aggr_us` is also 0.
> 
> Since `damon_get_scheme_score()` unconditionally invokes
> `damon_max_nr_accesses()` when evaluating user-defined schemes, could this
> allow a user to intentionally crash the kernel?

Nice finding.  I will separately fix this.


Thanks,
SJ

[...]

Re: [RFC PATCH 13/13] mm/damon: remove damon_region->nr_accesses_bp

[SeongJae Park]

On Fri, 19 Jun 2026 19:49:02 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] Stale comment in kdamond_fn() references the removed nr_accesses_bp field.
> --
> 
> commit 7fe8c1a02c74da4804760b45a04f15189947a500
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon: remove damon_region->nr_accesses_bp
> 
> This commit removes the unused nr_accesses_bp field from struct damon_region,
> as well as its associated kernel-doc comments.
> 
> > diff --git a/include/linux/damon.h b/include/linux/damon.h
> > index 4f73d27979f0b..a18d905f28e14 100644
> > --- a/include/linux/damon.h
> > +++ b/include/linux/damon.h
> [ ... ]
> > @@ -77,7 +68,6 @@ struct damon_region {
> >  	struct damon_addr_range ar;
> >  	unsigned long sampling_addr;
> >  	unsigned int nr_accesses;
> > -	unsigned int nr_accesses_bp;
> 
> [Severity: Low]
> Is this a minor regression that leaves a stale comment in kdamond_fn()?
> 
> Looking at mm/damon/core.c, kdamond_fn() still explicitly references the
> nr_accesses_bp field:
> 
> mm/damon/core.c:kdamond_fn() {
>     ...
> 				 * information reset for all regions.  Then,
> 				 * following kdamond_reset_aggregated() call
> 				 * will make the region information invalid,
> 				 * particularly for ->nr_accesses_bp.
>     ...
> }
> 
> Should this comment in mm/damon/core.c be updated or removed to reflect the
> field's deletion?

Nice catch.  I will remove the mention of the field in the next revision.


Thanks,
SJ

[...]