[PATCH 5.15.y 0/2] fix issue in damon_va_evenly_split_region()

[PATCH 5.15.y 0/2] fix issue in damon_va_evenly_split_region()

[SeongJae Park]

The fix depends on 044cd9750fe0 ("mm/damon/vaddr-test: split a test
function having >1024 bytes frame size"), which is not on 5.15.y tree.
Even after applying it, the fix might not cleanly cherry-picked due to
the file path change that seems not automatically detected on every
setup[1].  Since my setup somehow automatically detects the file path
change and do cherry-pick cleanly, posting both patches to avoid
unnecessary delay.

[1] https://lore.kernel.org/20241206173426.75223-1-sj@kernel.org

SeongJae Park (1):
  mm/damon/vaddr-test: split a test function having >1024 bytes frame
    size

Zheng Yejian (1):
  mm/damon/vaddr: fix issue in damon_va_evenly_split_region()

 mm/damon/vaddr-test.h | 78 +++++++++++++++++++++++--------------------
 mm/damon/vaddr.c      |  4 +--
 2 files changed, 43 insertions(+), 39 deletions(-)

-- 
2.39.5

[PATCH 5.15.y 1/2] mm/damon/vaddr-test: split a test function having >1024 bytes frame size

[SeongJae Park]

On some configuration[1], 'damon_test_split_evenly()' kunit test
function has >1024 bytes frame size, so below build warning is
triggered:

      CC      mm/damon/vaddr.o
    In file included from mm/damon/vaddr.c:672:
    mm/damon/vaddr-test.h: In function 'damon_test_split_evenly':
    mm/damon/vaddr-test.h:309:1: warning: the frame size of 1064 bytes is larger than 1024 bytes [-Wframe-larger-than=]
      309 | }
          | ^

This commit fixes the warning by separating the common logic in the
function.

[1] https://lore.kernel.org/linux-mm/202111182146.OV3C4uGr-lkp@intel.com/

Link: https://lkml.kernel.org/r/20211201150440.1088-6-sj@kernel.org
Fixes: 17ccae8bb5c9 ("mm/damon: add kunit tests")
Signed-off-by: SeongJae Park <sj@kernel.org>
Reported-by: kernel test robot <lkp@intel.com>
Cc: Brendan Higgins <brendanhiggins@google.com>
Cc: Shuah Khan <shuah@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
(cherry picked from commit 044cd9750fe010170f5dc812e4824d98f5ea928c)
---
 mm/damon/vaddr-test.h | 77 ++++++++++++++++++++++---------------------
 1 file changed, 40 insertions(+), 37 deletions(-)

diff --git a/mm/damon/vaddr-test.h b/mm/damon/vaddr-test.h
index 1f5c13257dba..95ec362cdc37 100644
--- a/mm/damon/vaddr-test.h
+++ b/mm/damon/vaddr-test.h
@@ -252,59 +252,62 @@ static void damon_test_apply_three_regions4(struct kunit *test)
 			new_three_regions, expected, ARRAY_SIZE(expected));
 }
 
-static void damon_test_split_evenly(struct kunit *test)
+static void damon_test_split_evenly_fail(struct kunit *test,
+		unsigned long start, unsigned long end, unsigned int nr_pieces)
 {
-	struct damon_ctx *c = damon_new_ctx();
-	struct damon_target *t;
-	struct damon_region *r;
-	unsigned long i;
-
-	KUNIT_EXPECT_EQ(test, damon_va_evenly_split_region(NULL, NULL, 5),
-			-EINVAL);
-
-	t = damon_new_target(42);
-	r = damon_new_region(0, 100);
-	KUNIT_EXPECT_EQ(test, damon_va_evenly_split_region(t, r, 0), -EINVAL);
+	struct damon_target *t = damon_new_target(42);
+	struct damon_region *r = damon_new_region(start, end);
 
 	damon_add_region(r, t);
-	KUNIT_EXPECT_EQ(test, damon_va_evenly_split_region(t, r, 10), 0);
-	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 10u);
+	KUNIT_EXPECT_EQ(test,
+			damon_va_evenly_split_region(t, r, nr_pieces), -EINVAL);
+	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 1u);
 
-	i = 0;
 	damon_for_each_region(r, t) {
-		KUNIT_EXPECT_EQ(test, r->ar.start, i++ * 10);
-		KUNIT_EXPECT_EQ(test, r->ar.end, i * 10);
+		KUNIT_EXPECT_EQ(test, r->ar.start, start);
+		KUNIT_EXPECT_EQ(test, r->ar.end, end);
 	}
+
 	damon_free_target(t);
+}
+
+static void damon_test_split_evenly_succ(struct kunit *test,
+	unsigned long start, unsigned long end, unsigned int nr_pieces)
+{
+	struct damon_target *t = damon_new_target(42);
+	struct damon_region *r = damon_new_region(start, end);
+	unsigned long expected_width = (end - start) / nr_pieces;
+	unsigned long i = 0;
 
-	t = damon_new_target(42);
-	r = damon_new_region(5, 59);
 	damon_add_region(r, t);
-	KUNIT_EXPECT_EQ(test, damon_va_evenly_split_region(t, r, 5), 0);
-	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 5u);
+	KUNIT_EXPECT_EQ(test,
+			damon_va_evenly_split_region(t, r, nr_pieces), 0);
+	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), nr_pieces);
 
-	i = 0;
 	damon_for_each_region(r, t) {
-		if (i == 4)
+		if (i == nr_pieces - 1)
 			break;
-		KUNIT_EXPECT_EQ(test, r->ar.start, 5 + 10 * i++);
-		KUNIT_EXPECT_EQ(test, r->ar.end, 5 + 10 * i);
+		KUNIT_EXPECT_EQ(test,
+				r->ar.start, start + i++ * expected_width);
+		KUNIT_EXPECT_EQ(test, r->ar.end, start + i * expected_width);
 	}
-	KUNIT_EXPECT_EQ(test, r->ar.start, 5 + 10 * i);
-	KUNIT_EXPECT_EQ(test, r->ar.end, 59ul);
+	KUNIT_EXPECT_EQ(test, r->ar.start, start + i * expected_width);
+	KUNIT_EXPECT_EQ(test, r->ar.end, end);
 	damon_free_target(t);
+}
 
-	t = damon_new_target(42);
-	r = damon_new_region(5, 6);
-	damon_add_region(r, t);
-	KUNIT_EXPECT_EQ(test, damon_va_evenly_split_region(t, r, 2), -EINVAL);
-	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 1u);
+static void damon_test_split_evenly(struct kunit *test)
+{
+	struct damon_ctx *c = damon_new_ctx();
+
+	KUNIT_EXPECT_EQ(test, damon_va_evenly_split_region(NULL, NULL, 5),
+			-EINVAL);
+
+	damon_test_split_evenly_fail(test, 0, 100, 0);
+	damon_test_split_evenly_succ(test, 0, 100, 10);
+	damon_test_split_evenly_succ(test, 5, 59, 5);
+	damon_test_split_evenly_fail(test, 5, 6, 2);
 
-	damon_for_each_region(r, t) {
-		KUNIT_EXPECT_EQ(test, r->ar.start, 5ul);
-		KUNIT_EXPECT_EQ(test, r->ar.end, 6ul);
-	}
-	damon_free_target(t);
 	damon_destroy_ctx(c);
 }
 
-- 
2.39.5

[PATCH 5.15.y 2/2] mm/damon/vaddr: fix issue in damon_va_evenly_split_region()

[SeongJae Park]

From: Zheng Yejian <zhengyejian@huaweicloud.com>

Patch series "mm/damon/vaddr: Fix issue in
damon_va_evenly_split_region()".  v2.

According to the logic of damon_va_evenly_split_region(), currently
following split case would not meet the expectation:

  Suppose DAMON_MIN_REGION=0x1000,
  Case: Split [0x0, 0x3000) into 2 pieces, then the result would be
        acutually 3 regions:
          [0x0, 0x1000), [0x1000, 0x2000), [0x2000, 0x3000)
        but NOT the expected 2 regions:
          [0x0, 0x1000), [0x1000, 0x3000) !!!

The root cause is that when calculating size of each split piece in
damon_va_evenly_split_region():

  `sz_piece = ALIGN_DOWN(sz_orig / nr_pieces, DAMON_MIN_REGION);`

both the dividing and the ALIGN_DOWN may cause loss of precision, then
each time split one piece of size 'sz_piece' from origin 'start' to 'end'
would cause more pieces are split out than expected!!!

To fix it, count for each piece split and make sure no more than
'nr_pieces'.  In addition, add above case into damon_test_split_evenly().

And add 'nr_piece == 1' check in damon_va_evenly_split_region() for better
code readability and add a corresponding kunit testcase.

This patch (of 2):

According to the logic of damon_va_evenly_split_region(), currently
following split case would not meet the expectation:

  Suppose DAMON_MIN_REGION=0x1000,
  Case: Split [0x0, 0x3000) into 2 pieces, then the result would be
        acutually 3 regions:
          [0x0, 0x1000), [0x1000, 0x2000), [0x2000, 0x3000)
        but NOT the expected 2 regions:
          [0x0, 0x1000), [0x1000, 0x3000) !!!

The root cause is that when calculating size of each split piece in
damon_va_evenly_split_region():

  `sz_piece = ALIGN_DOWN(sz_orig / nr_pieces, DAMON_MIN_REGION);`

both the dividing and the ALIGN_DOWN may cause loss of precision,
then each time split one piece of size 'sz_piece' from origin 'start' to
'end' would cause more pieces are split out than expected!!!

To fix it, count for each piece split and make sure no more than
'nr_pieces'. In addition, add above case into damon_test_split_evenly().

After this patch, damon-operations test passed:

 # ./tools/testing/kunit/kunit.py run damon-operations
 [...]
 ============== damon-operations (6 subtests) ===============
 [PASSED] damon_test_three_regions_in_vmas
 [PASSED] damon_test_apply_three_regions1
 [PASSED] damon_test_apply_three_regions2
 [PASSED] damon_test_apply_three_regions3
 [PASSED] damon_test_apply_three_regions4
 [PASSED] damon_test_split_evenly
 ================ [PASSED] damon-operations =================

Link: https://lkml.kernel.org/r/20241022083927.3592237-1-zhengyejian@huaweicloud.com
Link: https://lkml.kernel.org/r/20241022083927.3592237-2-zhengyejian@huaweicloud.com
Fixes: 3f49584b262c ("mm/damon: implement primitives for the virtual memory address spaces")
Signed-off-by: Zheng Yejian <zhengyejian@huaweicloud.com>
Reviewed-by: SeongJae Park <sj@kernel.org>
Cc: Fernand Sieber <sieberf@amazon.com>
Cc: Leonard Foerster <foersleo@amazon.de>
Cc: Shakeel Butt <shakeel.butt@linux.dev>
Cc: Ye Weihua <yeweihua4@huawei.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
(cherry picked from commit f3c7a1ede435e2e45177d7a490a85fb0a0ec96d1)
---
 mm/damon/vaddr-test.h | 1 +
 mm/damon/vaddr.c      | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/mm/damon/vaddr-test.h b/mm/damon/vaddr-test.h
index 95ec362cdc37..5531766ff09f 100644
--- a/mm/damon/vaddr-test.h
+++ b/mm/damon/vaddr-test.h
@@ -306,6 +306,7 @@ static void damon_test_split_evenly(struct kunit *test)
 	damon_test_split_evenly_fail(test, 0, 100, 0);
 	damon_test_split_evenly_succ(test, 0, 100, 10);
 	damon_test_split_evenly_succ(test, 5, 59, 5);
+	damon_test_split_evenly_succ(test, 0, 3, 2);
 	damon_test_split_evenly_fail(test, 5, 6, 2);
 
 	damon_destroy_ctx(c);
diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
index 6ad96da15081..6d8036671e60 100644
--- a/mm/damon/vaddr.c
+++ b/mm/damon/vaddr.c
@@ -69,6 +69,7 @@ static int damon_va_evenly_split_region(struct damon_target *t,
 	unsigned long sz_orig, sz_piece, orig_end;
 	struct damon_region *n = NULL, *next;
 	unsigned long start;
+	unsigned int i;
 
 	if (!r || !nr_pieces)
 		return -EINVAL;
@@ -82,8 +83,7 @@ static int damon_va_evenly_split_region(struct damon_target *t,
 
 	r->ar.end = r->ar.start + sz_piece;
 	next = damon_next_region(r);
-	for (start = r->ar.end; start + sz_piece <= orig_end;
-			start += sz_piece) {
+	for (start = r->ar.end, i = 1; i < nr_pieces; start += sz_piece, i++) {
 		n = damon_new_region(start, start + sz_piece);
 		if (!n)
 			return -ENOMEM;
-- 
2.39.5

RE: [PATCH 5.15.y 1/2] mm/damon/vaddr-test: split a test function having >1024 bytes frame size

[David Laight]

From: SeongJae Park
> Sent: 06 December 2024 18:16
> 
> On some configuration[1], 'damon_test_split_evenly()' kunit test
> function has >1024 bytes frame size, so below build warning is
> triggered:
> 
>       CC      mm/damon/vaddr.o
>     In file included from mm/damon/vaddr.c:672:
>     mm/damon/vaddr-test.h: In function 'damon_test_split_evenly':
>     mm/damon/vaddr-test.h:309:1: warning: the frame size of 1064 bytes is larger than 1024 bytes [-
> Wframe-larger-than=]
>       309 | }
>           | ^
> 
> This commit fixes the warning by separating the common logic in the
> function.

In that guaranteed to make any difference without some noinline_for_stack
attributes?

Not that I can see exactly where the stack was used.
But it tends to be clang that doesn't re-use stack space.

	David

> 
> [1] https://lore.kernel.org/linux-mm/202111182146.OV3C4uGr-lkp@intel.com/
> 
> Link: https://lkml.kernel.org/r/20211201150440.1088-6-sj@kernel.org
> Fixes: 17ccae8bb5c9 ("mm/damon: add kunit tests")
> Signed-off-by: SeongJae Park <sj@kernel.org>
> Reported-by: kernel test robot <lkp@intel.com>
> Cc: Brendan Higgins <brendanhiggins@google.com>
> Cc: Shuah Khan <shuah@kernel.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> (cherry picked from commit 044cd9750fe010170f5dc812e4824d98f5ea928c)
> ---
>  mm/damon/vaddr-test.h | 77 ++++++++++++++++++++++---------------------
>  1 file changed, 40 insertions(+), 37 deletions(-)
> 
> diff --git a/mm/damon/vaddr-test.h b/mm/damon/vaddr-test.h
> index 1f5c13257dba..95ec362cdc37 100644
> --- a/mm/damon/vaddr-test.h
> +++ b/mm/damon/vaddr-test.h
> @@ -252,59 +252,62 @@ static void damon_test_apply_three_regions4(struct kunit *test)
>  			new_three_regions, expected, ARRAY_SIZE(expected));
>  }
> 
> -static void damon_test_split_evenly(struct kunit *test)
> +static void damon_test_split_evenly_fail(struct kunit *test,
> +		unsigned long start, unsigned long end, unsigned int nr_pieces)
>  {
> -	struct damon_ctx *c = damon_new_ctx();
> -	struct damon_target *t;
> -	struct damon_region *r;
> -	unsigned long i;
> -
> -	KUNIT_EXPECT_EQ(test, damon_va_evenly_split_region(NULL, NULL, 5),
> -			-EINVAL);
> -
> -	t = damon_new_target(42);
> -	r = damon_new_region(0, 100);
> -	KUNIT_EXPECT_EQ(test, damon_va_evenly_split_region(t, r, 0), -EINVAL);
> +	struct damon_target *t = damon_new_target(42);
> +	struct damon_region *r = damon_new_region(start, end);
> 
>  	damon_add_region(r, t);
> -	KUNIT_EXPECT_EQ(test, damon_va_evenly_split_region(t, r, 10), 0);
> -	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 10u);
> +	KUNIT_EXPECT_EQ(test,
> +			damon_va_evenly_split_region(t, r, nr_pieces), -EINVAL);
> +	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 1u);
> 
> -	i = 0;
>  	damon_for_each_region(r, t) {
> -		KUNIT_EXPECT_EQ(test, r->ar.start, i++ * 10);
> -		KUNIT_EXPECT_EQ(test, r->ar.end, i * 10);
> +		KUNIT_EXPECT_EQ(test, r->ar.start, start);
> +		KUNIT_EXPECT_EQ(test, r->ar.end, end);
>  	}
> +
>  	damon_free_target(t);
> +}
> +
> +static void damon_test_split_evenly_succ(struct kunit *test,
> +	unsigned long start, unsigned long end, unsigned int nr_pieces)
> +{
> +	struct damon_target *t = damon_new_target(42);
> +	struct damon_region *r = damon_new_region(start, end);
> +	unsigned long expected_width = (end - start) / nr_pieces;
> +	unsigned long i = 0;
> 
> -	t = damon_new_target(42);
> -	r = damon_new_region(5, 59);
>  	damon_add_region(r, t);
> -	KUNIT_EXPECT_EQ(test, damon_va_evenly_split_region(t, r, 5), 0);
> -	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 5u);
> +	KUNIT_EXPECT_EQ(test,
> +			damon_va_evenly_split_region(t, r, nr_pieces), 0);
> +	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), nr_pieces);
> 
> -	i = 0;
>  	damon_for_each_region(r, t) {
> -		if (i == 4)
> +		if (i == nr_pieces - 1)
>  			break;
> -		KUNIT_EXPECT_EQ(test, r->ar.start, 5 + 10 * i++);
> -		KUNIT_EXPECT_EQ(test, r->ar.end, 5 + 10 * i);
> +		KUNIT_EXPECT_EQ(test,
> +				r->ar.start, start + i++ * expected_width);
> +		KUNIT_EXPECT_EQ(test, r->ar.end, start + i * expected_width);
>  	}
> -	KUNIT_EXPECT_EQ(test, r->ar.start, 5 + 10 * i);
> -	KUNIT_EXPECT_EQ(test, r->ar.end, 59ul);
> +	KUNIT_EXPECT_EQ(test, r->ar.start, start + i * expected_width);
> +	KUNIT_EXPECT_EQ(test, r->ar.end, end);
>  	damon_free_target(t);
> +}
> 
> -	t = damon_new_target(42);
> -	r = damon_new_region(5, 6);
> -	damon_add_region(r, t);
> -	KUNIT_EXPECT_EQ(test, damon_va_evenly_split_region(t, r, 2), -EINVAL);
> -	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 1u);
> +static void damon_test_split_evenly(struct kunit *test)
> +{
> +	struct damon_ctx *c = damon_new_ctx();
> +
> +	KUNIT_EXPECT_EQ(test, damon_va_evenly_split_region(NULL, NULL, 5),
> +			-EINVAL);
> +
> +	damon_test_split_evenly_fail(test, 0, 100, 0);
> +	damon_test_split_evenly_succ(test, 0, 100, 10);
> +	damon_test_split_evenly_succ(test, 5, 59, 5);
> +	damon_test_split_evenly_fail(test, 5, 6, 2);
> 
> -	damon_for_each_region(r, t) {
> -		KUNIT_EXPECT_EQ(test, r->ar.start, 5ul);
> -		KUNIT_EXPECT_EQ(test, r->ar.end, 6ul);
> -	}
> -	damon_free_target(t);
>  	damon_destroy_ctx(c);
>  }
> 
> --
> 2.39.5
> 

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)