[RFC PATCH 00/10] mm/damon: cleanup code, add test cases, and update guidances in docs

[RFC PATCH 00/10] mm/damon: cleanup code, add test cases, and update guidances in docs

[SJ Park]

Misc cleanup, improvements and updates of code, test, and documents.

Patches 1-3 cleanup DAMON code.  Patches 4-8 adds kunit and selftest
test cases for recently fixed bugs and a new feature.  Patches 9 and 10
update guidelines for AI review and what document to read, on DAMON
documents.

SJ Park (10):
  mm/damon/core: use damon_nr_samples_per_aggr() for max merge threshold
  mm/damon/core: remove debug messages
  mm/damon/vaddr: remove a debug message
  mm/damon/tests/core-kunit: extend set_regions() test for error case
  mm/damon/tests/core-kunit: test <=0 size damon_set_regions() inputs
  mm/damon/tests/core-kunit: test overlapping ranges for set_regions()
  mm/damon/tests/core-kunit: test damon_nr_samples_per_aggr()
  selftests/damon/sysfs.sh: test hugepage_mem_bp quota goal
  Docs/mm/damon/maintainer-profile: update AI review for Sashiko replies
  Docs/ABI/damon: recommend subsystem doc instead of admin-guide

 .../ABI/testing/sysfs-kernel-mm-damon         |  2 +-
 Documentation/mm/damon/maintainer-profile.rst | 19 ++----
 mm/damon/core.c                               | 12 +---
 mm/damon/tests/core-kunit.h                   | 60 ++++++++++++++++---
 mm/damon/vaddr.c                              | 11 +---
 tools/testing/selftests/damon/sysfs.sh        |  1 +
 6 files changed, 61 insertions(+), 44 deletions(-)


base-commit: f5d0a1c210a9bec33ee6cc888f4cc6dd1f56eda7
-- 
2.47.3

[RFC PATCH 01/10] mm/damon/core: use damon_nr_samples_per_aggr() for max merge threshold

[SJ Park]

kdamond_merge_regions() open-codes max region merge threshold
calculation.  What it does is fundamentally the same as
damon_nr_samples_per_aggr() but missing a few corner cases.  The
unhandled corner cases should be rare and make only a negligible level
of monitoring results degradation.  But having the inconsistency could
increase future maintenance burden.  Use the dedicated function.

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

diff --git a/mm/damon/core.c b/mm/damon/core.c
index f7c9b94b6436b..4169268d94416 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -3470,8 +3470,7 @@ static void kdamond_merge_regions(struct damon_ctx *c, unsigned int threshold,
 	unsigned int max_thres;
 	bool count_age = true;
 
-	max_thres = c->attrs.aggr_interval /
-		(c->attrs.sample_interval ?  c->attrs.sample_interval : 1);
+	max_thres = damon_nr_samples_per_aggr(&c->attrs);
 	while (true) {
 		nr_regions = 0;
 		damon_for_each_target(t, c) {
-- 
2.47.3

[RFC PATCH 02/10] mm/damon/core: remove debug messages

[SJ Park]

There are a few debug messages in DAMON core.  Those have not really
been used in a meaningful way for the last few years, though.  Remove
those.

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

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 4169268d94416..1b0f12d17de3a 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -3693,10 +3693,6 @@ static unsigned long damos_wmark_wait_us(struct damos *scheme)
 
 	/* higher than high watermark or lower than low watermark */
 	if (metric > scheme->wmarks.high || scheme->wmarks.low > metric) {
-		if (scheme->wmarks.activated)
-			pr_debug("deactivate a scheme (%d) for %s wmark\n",
-				 scheme->action,
-				 str_high_low(metric > scheme->wmarks.high));
 		scheme->wmarks.activated = false;
 		return scheme->wmarks.interval;
 	}
@@ -3706,8 +3702,6 @@ static unsigned long damos_wmark_wait_us(struct damos *scheme)
 			!scheme->wmarks.activated)
 		return scheme->wmarks.interval;
 
-	if (!scheme->wmarks.activated)
-		pr_debug("activate a scheme (%d)\n", scheme->action);
 	scheme->wmarks.activated = true;
 	return 0;
 }
@@ -3850,8 +3844,6 @@ static int kdamond_fn(void *data)
 	struct damon_ctx *ctx = data;
 	unsigned long sz_limit = 0;
 
-	pr_debug("kdamond (%d) starts\n", current->pid);
-
 	mutex_lock(&ctx->call_controls_lock);
 	ctx->call_controls_obsolete = false;
 	mutex_unlock(&ctx->call_controls_lock);
@@ -4005,7 +3997,6 @@ static int kdamond_fn(void *data)
 	mutex_unlock(&ctx->walk_control_lock);
 	damos_walk_cancel(ctx);
 
-	pr_debug("kdamond (%d) finishes\n", current->pid);
 	mutex_lock(&ctx->kdamond_lock);
 	ctx->kdamond = NULL;
 	mutex_unlock(&ctx->kdamond_lock);
-- 
2.47.3

[RFC PATCH 03/10] mm/damon/vaddr: remove a debug message

[SJ Park]

There is a debug message in the DAMON virtual address space operation
set.  It has not really been used in a meaningful way for the last few
years, though.  Remove it.

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

diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
index 0648400b2d65b..95899b217b2af 100644
--- a/mm/damon/vaddr.c
+++ b/mm/damon/vaddr.c
@@ -192,19 +192,10 @@ static int damon_va_three_regions(struct damon_target *t,
 static void __damon_va_init_regions(struct damon_ctx *ctx,
 				     struct damon_target *t)
 {
-	struct damon_target *ti;
 	struct damon_addr_range regions[3];
-	int tidx = 0;
 
-	if (damon_va_three_regions(t, regions)) {
-		damon_for_each_target(ti, ctx) {
-			if (ti == t)
-				break;
-			tidx++;
-		}
-		pr_debug("Failed to get three regions of %dth target\n", tidx);
+	if (damon_va_three_regions(t, regions))
 		return;
-	}
 
 	damon_set_regions(t, regions, 3, DAMON_MIN_REGION_SZ);
 }
-- 
2.47.3

[RFC PATCH 04/10] mm/damon/tests/core-kunit: extend set_regions() test for error case

[SJ Park]

damon_test_set_regions_for() is designed to test only success-expected
damon_set_regions() calls.  Extend it to cover error-expected calls,
too.

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

diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
index 4a536d41cdb2d..befc1196d6d60 100644
--- a/mm/damon/tests/core-kunit.h
+++ b/mm/damon/tests/core-kunit.h
@@ -463,11 +463,12 @@ static void damon_test_set_regions_for(struct kunit *test,
 		struct damon_addr_range *old_ranges, int sz_old_ranges,
 		struct damon_addr_range *new_ranges, int sz_new_ranges,
 		unsigned long min_region_sz,
-		struct damon_addr_range *expect_ranges, int sz_expect_ranges)
+		struct damon_addr_range *expect_ranges, int sz_expect_ranges,
+		int expect_err)
 {
 	struct damon_target *t;
 	struct damon_region *r;
-	int i;
+	int i, err;
 
 	t = damon_new_target();
 	if (!t)
@@ -481,7 +482,8 @@ static void damon_test_set_regions_for(struct kunit *test,
 		damon_add_region(r, t);
 	}
 
-	damon_set_regions(t, new_ranges, sz_new_ranges, min_region_sz);
+	err = damon_set_regions(t, new_ranges, sz_new_ranges, min_region_sz);
+	KUNIT_EXPECT_EQ(test, err, expect_err);
 
 	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), sz_expect_ranges);
 	if (damon_nr_regions(t) != sz_expect_ranges) {
@@ -510,7 +512,7 @@ static void damon_test_set_regions(struct kunit *test)
 			(struct damon_addr_range[]){
 			{.start = 5, .end = 15},
 			{.start = 15, .end = 25},
-			}, 2);
+			}, 2, 0);
 	/* Un-intersecting regions should be removed. */
 	damon_test_set_regions_for(test,
 			(struct damon_addr_range[]){
@@ -523,7 +525,7 @@ static void damon_test_set_regions(struct kunit *test)
 			1,
 			(struct damon_addr_range[]){
 			{.start = 18, .end = 23},
-			}, 1);
+			}, 1, 0);
 	/*
 	 * Holes should be filled up with new regions.
 	 *
@@ -544,7 +546,7 @@ static void damon_test_set_regions(struct kunit *test)
 			{.start = 8, .end = 16},
 			{.start = 16, .end = 24},
 			{.start = 24, .end = 28},
-			}, 3);
+			}, 3, 0);
 	/*
 	 * New regions should be able to be appended.
 	 *
@@ -566,7 +568,7 @@ static void damon_test_set_regions(struct kunit *test)
 			{.start = 0, .end = 4},
 			{.start = 4, .end = 15},
 			{.start = 25, .end = 40},
-			}, 3);
+			}, 3, 0);
 	/*
 	 * New regions should be able to be inserted.
 	 *
@@ -589,7 +591,7 @@ static void damon_test_set_regions(struct kunit *test)
 			{.start = 0, .end = 15},
 			{.start = 25, .end = 40},
 			{.start = 44, .end = 50},
-			}, 3);
+			}, 3, 0);
 }
 
 static void damon_test_update_monitoring_result(struct kunit *test)
-- 
2.47.3

[RFC PATCH 05/10] mm/damon/tests/core-kunit: test <=0 size damon_set_regions() inputs

[SJ Park]

Commit 1292c0ecb1ca ("mm/damon/core: validate ranges in
damon_set_regions()") disallowed passing zero or negative size input
ranges to damon_set_regions().  Add kunit test cases for those inputs.

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

diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
index befc1196d6d60..ac15393a7935b 100644
--- a/mm/damon/tests/core-kunit.h
+++ b/mm/damon/tests/core-kunit.h
@@ -592,6 +592,20 @@ static void damon_test_set_regions(struct kunit *test)
 			{.start = 25, .end = 40},
 			{.start = 44, .end = 50},
 			}, 3, 0);
+	/* Zero size regions should return -EINVAL. */
+	damon_test_set_regions_for(test,
+			(struct damon_addr_range[]){}, 0,
+			(struct damon_addr_range[]){
+			{.start = 42, .end = 42},
+			}, 1, 1,
+			(struct damon_addr_range[]){}, 0, -EINVAL);
+	/* Negative size regions should return -EINVAL. */
+	damon_test_set_regions_for(test,
+			(struct damon_addr_range[]){}, 0,
+			(struct damon_addr_range[]){
+			{.start = 42, .end = 21},
+			}, 1, 1,
+			(struct damon_addr_range[]){}, 0, -EINVAL);
 }
 
 static void damon_test_update_monitoring_result(struct kunit *test)
-- 
2.47.3

[RFC PATCH 06/10] mm/damon/tests/core-kunit: test overlapping ranges for set_regions()

[SJ Park]

Commit 954157679ec3 ("mm/damon/core: disallow overlapping input ranges
for damon_set_regions()") disallowed passing overlapping input ranges to
damon_set_regions().  Add a kunit test case for the overlapping input.

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

diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
index ac15393a7935b..3041b8288ec26 100644
--- a/mm/damon/tests/core-kunit.h
+++ b/mm/damon/tests/core-kunit.h
@@ -606,6 +606,17 @@ static void damon_test_set_regions(struct kunit *test)
 			{.start = 42, .end = 21},
 			}, 1, 1,
 			(struct damon_addr_range[]){}, 0, -EINVAL);
+	/*
+	 * Regions resulting in same region after alingment should return
+	 * -EINVAL.
+	 */
+	damon_test_set_regions_for(test,
+			(struct damon_addr_range[]){}, 0,
+			(struct damon_addr_range[]){
+			{.start = 10, .end = 20},
+			{.start = 20, .end = 30},
+			}, 2, 4096,
+			(struct damon_addr_range[]){}, 0, -EINVAL);
 }
 
 static void damon_test_update_monitoring_result(struct kunit *test)
-- 
2.47.3

[RFC PATCH 07/10] mm/damon/tests/core-kunit: test damon_nr_samples_per_aggr()

[SJ Park]

damon_max_nr_accesses(), which is a previous version of
damon_nr_samples_per_aggr() before the renaming, was wrongly returning
zero or random overflowed values for extreme intervals setup.  Commit
35d4a3cf70a8 ("mm/damon/ops-common: handle extreme intervals in
damon_hot_score()") updated the function to return correct or more valid
values.  Add a kunit test to ensure it is working as expected.

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

diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
index 3041b8288ec26..511f2856d343c 100644
--- a/mm/damon/tests/core-kunit.h
+++ b/mm/damon/tests/core-kunit.h
@@ -619,6 +619,22 @@ static void damon_test_set_regions(struct kunit *test)
 			(struct damon_addr_range[]){}, 0, -EINVAL);
 }
 
+static void damon_test_nr_samples_per_aggr(struct kunit *test)
+{
+	struct damon_attrs attrs = {
+		.sample_interval = 0,
+		.aggr_interval = 0,
+	};
+
+	/* Zero aggregation interval doesn't cause division by zero */
+	KUNIT_EXPECT_EQ(test, damon_nr_samples_per_aggr(&attrs), 1);
+
+	/* Too large aggregation interval doesn't cause overflow */
+	attrs.aggr_interval = (unsigned long)UINT_MAX + 1;
+	KUNIT_EXPECT_GE(test, damon_nr_samples_per_aggr(&attrs), 0);
+	KUNIT_EXPECT_LE(test, damon_nr_samples_per_aggr(&attrs), UINT_MAX);
+}
+
 static void damon_test_update_monitoring_result(struct kunit *test)
 {
 	struct damon_attrs old_attrs = {
@@ -1670,6 +1686,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_samples_per_aggr),
 	KUNIT_CASE(damon_test_update_monitoring_result),
 	KUNIT_CASE(damon_test_set_attrs),
 	KUNIT_CASE(damon_test_mvsum),
-- 
2.47.3

[RFC PATCH 08/10] selftests/damon/sysfs.sh: test hugepage_mem_bp quota goal

[SJ Park]

DAMON sysfs quota goal target_metric file now accepts 'hugepage_mem_bp'
input.  Test it is accepted in fundamental DAMON sysfs file operation
selftest.

Signed-off-by: SJ Park <sj@kernel.org>
---
 tools/testing/selftests/damon/sysfs.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/damon/sysfs.sh b/tools/testing/selftests/damon/sysfs.sh
index ddebde6edabe4..b66593c9ac471 100755
--- a/tools/testing/selftests/damon/sysfs.sh
+++ b/tools/testing/selftests/damon/sysfs.sh
@@ -210,6 +210,7 @@ test_goal()
 	ensure_write_succ "$fpath" "active_mem_bp" "valid input"
 	ensure_write_succ "$fpath" "inactive_mem_bp" "valid input"
 	ensure_write_succ "$fpath" "node_eligible_mem_bp" "valid input"
+	ensure_write_succ "$fpath" "hugepage_mem_bp" "valid input"
 	ensure_write_fail "$fpath" "foo" "invalid input"
 	ensure_file "$goal_dir/nid" "exist" "600"
 	ensure_file "$goal_dir/path" "exist" "600"
-- 
2.47.3

[RFC PATCH 09/10] Docs/mm/damon/maintainer-profile: update AI review for Sashiko replies

[SJ Park]

In the past, sharing Sashiko review results was tedious.  Also it was
suggested to minimize the recipients on the sharing mails.  Hence the AI
review section of DAMON maintainer profile document was updated to give
guidance about available tools for making the sharing easier, and how
the recipients list should be managed.

Now DAMON is onboarded [1] to Sashiko's automatic review replies
feature.  Sashiko directly sends its reviews as replies to the patch
mail thread. It also reduces the recipients list to deliver the
reporting to only the author and the mailing list.

Remove the old guidance that is no more necessary but only confusing.

[1] https://github.com/sashiko-dev/sashiko/commit/b554c7b6e733

Signed-off-by: SJ Park <sj@kernel.org>
---
 Documentation/mm/damon/maintainer-profile.rst | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/Documentation/mm/damon/maintainer-profile.rst b/Documentation/mm/damon/maintainer-profile.rst
index fb2fa00cc9aa1..a7c1352339378 100644
--- a/Documentation/mm/damon/maintainer-profile.rst
+++ b/Documentation/mm/damon/maintainer-profile.rst
@@ -106,18 +106,9 @@ AI Review
 
 For patches that are publicly posted to DAMON mailing list
 (damon@lists.linux.dev), AI reviews of the patches will be available at
-sashiko.dev.  The reviews could also be sent as mails to the author of the
-patch.
-
-Patch authors are encouraged to check the AI reviews and share their opinions.
-The sharing could be done as a reply to the mail thread.  Consider reducing the
-recipients list for such sharing, since some people are not really interested
-in AI reviews.  As a rule of thumb, drop stable@vger.kernel.org and individuals
-except DAMON maintainer.
-
-`hkml` also provides a `feature
-<https://github.com/sjp38/hackermail/blob/master/USAGE.md#forwarding-sashikodev-statuscomments-to-mailing-list>`_
-for such sharing.  Please feel free to use the feature.
+sashiko.dev.  The reviews will also be sent as replies to the author of the
+patch and the mailing list.
 
-It is only an optional recommendation.  DAMON maintainer could also ask any
-question about the AI reviews, though.
+Patch authors are encouraged to check the AI reviews and share their opinions
+by replying on the mail thread.  It is only an optional recommendation.  DAMON
+maintainer could also ask any question about the AI reviews, though.
-- 
2.47.3

[RFC PATCH 10/10] Docs/ABI/damon: recommend subsystem doc instead of admin-guide

[SJ Park]

DAMON ABI doc is recommending DAMON admin-guide for people who are
willing to know further about DAMON.  Nowadays the subsystem doc
(Documentation/mm/damon) is a more recommended place for even beginners.
Recommend the subsystem doc over admin-guide.

Signed-off-by: SJ Park <sj@kernel.org>
---
 Documentation/ABI/testing/sysfs-kernel-mm-damon | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-kernel-mm-damon b/Documentation/ABI/testing/sysfs-kernel-mm-damon
index f8d2601e82904..ad21f58f3c912 100644
--- a/Documentation/ABI/testing/sysfs-kernel-mm-damon
+++ b/Documentation/ABI/testing/sysfs-kernel-mm-damon
@@ -3,7 +3,7 @@ Date:		Mar 2022
 Contact:	SJ Park <sj@kernel.org>
 Description:	Interface for Data Access MONitoring (DAMON).  Contains files
 		for controlling DAMON.  For more details on DAMON itself,
-		please refer to Documentation/admin-guide/mm/damon/index.rst.
+		please refer to Documentation/mm/damon/index.rst.
 
 What:		/sys/kernel/mm/damon/admin/
 Date:		Mar 2022
-- 
2.47.3

Re: [RFC PATCH 03/10] mm/damon/vaddr: remove a debug message

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The 'ctx' parameter in '__damon_va_init_regions()' is now completely unused and should be removed.
--

--- Patch [3]: [RFC PATCH 03/10] mm/damon/vaddr: remove a debug message ---
commit 6f7aa725872ec3ac7c5da1213c336d706d99ebb1
Author: SJ Park <sj@kernel.org>

mm/damon/vaddr: remove a debug message

This commit removes an old debug message in the DAMON virtual address
space operation set that hasn't been used meaningfully for the last
few years.

> diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> index 4976775fe2762..5ccf70dc8a30b 100644
> --- a/mm/damon/vaddr.c
> +++ b/mm/damon/vaddr.c
> @@ -192,19 +192,10 @@ static int damon_va_three_regions(struct damon_target *t,
>  static void __damon_va_init_regions(struct damon_ctx *ctx,
>  				     struct damon_target *t)

[Severity: Low]
Since the removal of the debug message also removes the only usage of ctx
in __damon_va_init_regions(), should we also remove the ctx parameter from
this static function's signature and update its caller to avoid unused
parameter warnings?

>  {
> -	struct damon_target *ti;
>  	struct damon_addr_range regions[3];
> -	int tidx = 0;
>  
> -	if (damon_va_three_regions(t, regions)) {
> -		damon_for_each_target(ti, ctx) {
> -			if (ti == t)
> -				break;
> -			tidx++;
> -		}
> -		pr_debug("Failed to get three regions of %dth target\n", tidx);
> +	if (damon_va_three_regions(t, regions))
>  		return;
> -	}
>  
>  	damon_set_regions(t, regions, 3, DAMON_MIN_REGION_SZ);
>  }

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

Re: [RFC PATCH 07/10] mm/damon/tests/core-kunit: test damon_nr_samples_per_aggr()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The newly added `damon_test_nr_samples_per_aggr` test uses tautological assertions that will always pass, failing to detect the regressions it was intended to prevent. Additionally, the test setup overflows to zero on 32-bit architectures.
--

--- Patch [7]: [RFC PATCH 07/10] mm/damon/tests/core-kunit: test damon_nr_samples_per_aggr() ---
commit 50a7320dda40ca169bc23924caa1421599f78314
Author: SJ Park <sj@kernel.org>

mm/damon/tests/core-kunit: test damon_nr_samples_per_aggr()

This commit adds a kunit test for damon_nr_samples_per_aggr() to ensure
that extreme interval setups are handled properly without returning zero
or random overflowed values.

> diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
> index 3041b8288ec26..511f2856d343c 100644
> --- a/mm/damon/tests/core-kunit.h
> +++ b/mm/damon/tests/core-kunit.h

[ ... ]

> +static void damon_test_nr_samples_per_aggr(struct kunit *test)
> +{
> +	struct damon_attrs attrs = {
> +		.sample_interval = 0,
> +		.aggr_interval = 0,
> +	};
> +
> +	/* Zero aggregation interval doesn't cause division by zero */
> +	KUNIT_EXPECT_EQ(test, damon_nr_samples_per_aggr(&attrs), 1);
> +
> +	/* Too large aggregation interval doesn't cause overflow */
> +	attrs.aggr_interval = (unsigned long)UINT_MAX + 1;

[Severity: Medium]
On 32-bit systems where unsigned long is 32-bits wide, does this assignment
overflow to 0?

If it overflows, the test would configure an interval of 0 instead of a value
larger than UINT_MAX, failing to test the large-interval scenario.

> +	KUNIT_EXPECT_GE(test, damon_nr_samples_per_aggr(&attrs), 0);
> +	KUNIT_EXPECT_LE(test, damon_nr_samples_per_aggr(&attrs), UINT_MAX);
> +}

[Severity: Medium]
Since damon_nr_samples_per_aggr() returns an unsigned int, are these
assertions tautological?

An unsigned int is inherently always >= 0 and <= UINT_MAX. This means the
test will pass regardless of the function's return value, and it wouldn't
catch if the function unexpectedly returned 0.

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

Re: [RFC PATCH 08/10] selftests/damon/sysfs.sh: test hugepage_mem_bp quota goal

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The selftest unconditionally fails because the kernel implementation for the 'hugepage_mem_bp' quota goal target metric is missing.
--

--- Patch [8]: [RFC PATCH 08/10] selftests/damon/sysfs.sh: test hugepage_mem_bp quota goal ---
commit 033ea051e9d9dfb2787f833f5569853c4af61194
Author: SJ Park <sj@kernel.org>

selftests/damon/sysfs.sh: test hugepage_mem_bp quota goal

DAMON sysfs quota goal target_metric file now accepts 'hugepage_mem_bp'
input. Test it is accepted in fundamental DAMON sysfs file operation
selftest.

> diff --git a/tools/testing/selftests/damon/sysfs.sh b/tools/testing/selftests/damon/sysfs.sh
> --- a/tools/testing/selftests/damon/sysfs.sh
> +++ b/tools/testing/selftests/damon/sysfs.sh
> @@ -210,6 +210,7 @@ test_goal()
>  	ensure_write_succ "$fpath" "active_mem_bp" "valid input"
>  	ensure_write_succ "$fpath" "inactive_mem_bp" "valid input"
>  	ensure_write_succ "$fpath" "node_eligible_mem_bp" "valid input"
> +	ensure_write_succ "$fpath" "hugepage_mem_bp" "valid input"

[Severity: Medium]
Will this selftest fail when executed?

Looking at target_metric_store() in mm/damon/sysfs-schemes.c, the kernel
validation loop returns -EINVAL because "hugepage_mem_bp" is not present in
the damos_sysfs_qgoal_metric_names array:

mm/damon/sysfs-schemes.c:target_metric_store() {
	...
	for (i = 0; i < ARRAY_SIZE(damos_sysfs_qgoal_metric_names); i++) {
		struct damos_sysfs_qgoal_metric_name *metric_name;

		metric_name = &damos_sysfs_qgoal_metric_names[i];
		if (sysfs_streq(buf, metric_name->name)) {
			goal->metric = metric_name->metric;
			return count;
		}
	}
	return -EINVAL;
}

Because the test assumes the write will succeed, the missing kernel
implementation for this quota goal target metric will cause the write to be
rejected and trigger a selftest failure.

>  	ensure_write_fail "$fpath" "foo" "invalid input"
>  	ensure_file "$goal_dir/nid" "exist" "600"
>  	ensure_file "$goal_dir/path" "exist" "600"

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

Re: [RFC PATCH 03/10] mm/damon/vaddr: remove a debug message

[SJ Park]

On Tue, 28 Jul 2026 05:32:43 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] The 'ctx' parameter in '__damon_va_init_regions()' is now completely unused and should be removed.
> --
> 
> --- Patch [3]: [RFC PATCH 03/10] mm/damon/vaddr: remove a debug message ---
> commit 6f7aa725872ec3ac7c5da1213c336d706d99ebb1
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/vaddr: remove a debug message
> 
> This commit removes an old debug message in the DAMON virtual address
> space operation set that hasn't been used meaningfully for the last
> few years.
> 
> > diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> > index 4976775fe2762..5ccf70dc8a30b 100644
> > --- a/mm/damon/vaddr.c
> > +++ b/mm/damon/vaddr.c
> > @@ -192,19 +192,10 @@ static int damon_va_three_regions(struct damon_target *t,
> >  static void __damon_va_init_regions(struct damon_ctx *ctx,
> >  				     struct damon_target *t)
> 
> [Severity: Low]
> Since the removal of the debug message also removes the only usage of ctx
> in __damon_va_init_regions(), should we also remove the ctx parameter from
> this static function's signature and update its caller to avoid unused
> parameter warnings?

Good suggestion, I will do so.

> 
> >  {
> > -	struct damon_target *ti;
> >  	struct damon_addr_range regions[3];
> > -	int tidx = 0;
> >  
> > -	if (damon_va_three_regions(t, regions)) {
> > -		damon_for_each_target(ti, ctx) {
> > -			if (ti == t)
> > -				break;
> > -			tidx++;
> > -		}
> > -		pr_debug("Failed to get three regions of %dth target\n", tidx);
> > +	if (damon_va_three_regions(t, regions))
> >  		return;
> > -	}
> >  
> >  	damon_set_regions(t, regions, 3, DAMON_MIN_REGION_SZ);
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260728052811.192712-1-sj@kernel.org?part=3


Thanks,
SJ

Re: [RFC PATCH 07/10] mm/damon/tests/core-kunit: test damon_nr_samples_per_aggr()

[SJ Park]

On Tue, 28 Jul 2026 05:33:44 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The newly added `damon_test_nr_samples_per_aggr` test uses tautological assertions that will always pass, failing to detect the regressions it was intended to prevent. Additionally, the test setup overflows to zero on 32-bit architectures.
> --
> 
> --- Patch [7]: [RFC PATCH 07/10] mm/damon/tests/core-kunit: test damon_nr_samples_per_aggr() ---
> commit 50a7320dda40ca169bc23924caa1421599f78314
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/tests/core-kunit: test damon_nr_samples_per_aggr()
> 
> This commit adds a kunit test for damon_nr_samples_per_aggr() to ensure
> that extreme interval setups are handled properly without returning zero
> or random overflowed values.
> 
> > diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
> > index 3041b8288ec26..511f2856d343c 100644
> > --- a/mm/damon/tests/core-kunit.h
> > +++ b/mm/damon/tests/core-kunit.h
> 
> [ ... ]
> 
> > +static void damon_test_nr_samples_per_aggr(struct kunit *test)
> > +{
> > +	struct damon_attrs attrs = {
> > +		.sample_interval = 0,
> > +		.aggr_interval = 0,
> > +	};
> > +
> > +	/* Zero aggregation interval doesn't cause division by zero */
> > +	KUNIT_EXPECT_EQ(test, damon_nr_samples_per_aggr(&attrs), 1);
> > +
> > +	/* Too large aggregation interval doesn't cause overflow */
> > +	attrs.aggr_interval = (unsigned long)UINT_MAX + 1;
> 
> [Severity: Medium]
> On 32-bit systems where unsigned long is 32-bits wide, does this assignment
> overflow to 0?
> 
> If it overflows, the test would configure an interval of 0 instead of a value
> larger than UINT_MAX, failing to test the large-interval scenario.

Yes, this is for testing on 64bit system.  I will add this to the comment.

> 
> > +	KUNIT_EXPECT_GE(test, damon_nr_samples_per_aggr(&attrs), 0);
> > +	KUNIT_EXPECT_LE(test, damon_nr_samples_per_aggr(&attrs), UINT_MAX);
> > +}
> 
> [Severity: Medium]
> Since damon_nr_samples_per_aggr() returns an unsigned int, are these
> assertions tautological?
> 
> An unsigned int is inherently always >= 0 and <= UINT_MAX. This means the
> test will pass regardless of the function's return value, and it wouldn't
> catch if the function unexpectedly returned 0.

Good catch.  The test should ensure the return value on 64 bit system is
exactly UINT_MAX.  I will do so.

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


Thanks,
SJ

Re: [RFC PATCH 08/10] selftests/damon/sysfs.sh: test hugepage_mem_bp quota goal

[SJ Park]

On Tue, 28 Jul 2026 05:35:33 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The selftest unconditionally fails because the kernel implementation for the 'hugepage_mem_bp' quota goal target metric is missing.
> --
> 
> --- Patch [8]: [RFC PATCH 08/10] selftests/damon/sysfs.sh: test hugepage_mem_bp quota goal ---
> commit 033ea051e9d9dfb2787f833f5569853c4af61194
> Author: SJ Park <sj@kernel.org>
> 
> selftests/damon/sysfs.sh: test hugepage_mem_bp quota goal
> 
> DAMON sysfs quota goal target_metric file now accepts 'hugepage_mem_bp'
> input. Test it is accepted in fundamental DAMON sysfs file operation
> selftest.
> 
> > diff --git a/tools/testing/selftests/damon/sysfs.sh b/tools/testing/selftests/damon/sysfs.sh
> > --- a/tools/testing/selftests/damon/sysfs.sh
> > +++ b/tools/testing/selftests/damon/sysfs.sh
> > @@ -210,6 +210,7 @@ test_goal()
> >  	ensure_write_succ "$fpath" "active_mem_bp" "valid input"
> >  	ensure_write_succ "$fpath" "inactive_mem_bp" "valid input"
> >  	ensure_write_succ "$fpath" "node_eligible_mem_bp" "valid input"
> > +	ensure_write_succ "$fpath" "hugepage_mem_bp" "valid input"
> 
> [Severity: Medium]
> Will this selftest fail when executed?
> 
> Looking at target_metric_store() in mm/damon/sysfs-schemes.c, the kernel
> validation loop returns -EINVAL because "hugepage_mem_bp" is not present in
> the damos_sysfs_qgoal_metric_names array:
> 
> mm/damon/sysfs-schemes.c:target_metric_store() {
> 	...
> 	for (i = 0; i < ARRAY_SIZE(damos_sysfs_qgoal_metric_names); i++) {
> 		struct damos_sysfs_qgoal_metric_name *metric_name;
> 
> 		metric_name = &damos_sysfs_qgoal_metric_names[i];
> 		if (sysfs_streq(buf, metric_name->name)) {
> 			goal->metric = metric_name->metric;
> 			return count;
> 		}
> 	}
> 	return -EINVAL;
> }
> 
> Because the test assumes the write will succeed, the missing kernel
> implementation for this quota goal target metric will cause the write to be
> rejected and trigger a selftest failure.

Implementation of hugepage_mem_bp is not yet in mm-new, but will be merged
before this series.  So, no issue.

> 
> >  	ensure_write_fail "$fpath" "foo" "invalid input"
> >  	ensure_file "$goal_dir/nid" "exist" "600"
> >  	ensure_file "$goal_dir/path" "exist" "600"
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260728052811.192712-1-sj@kernel.org?part=8


Thanks,
SJ