[PATCH v2 0/8] memblock tests: update and extend memblock simulator

[PATCH v2 0/8] memblock tests: update and extend memblock simulator

[Rebecca Mckeever]

These patches update existing tests in memblock simulator, add
additional tests for memblock functions that are already being tested,
and add test coverage for additional memblock functions.

Updated tests for:
- memblock_alloc()
- memblock_alloc_try_nid()
- memblock_alloc_from()

The updates to memblock_alloc() tests include the addition of an assert
that checks whether the entire chunk of allocated memory is cleared. For
memblock_alloc_try_nid() and memblock_alloc_from(), the assert that checks
whether the allocated memory is cleared now checks the entire chunk of
allocated memory instead of just the first byte. To make this more robust,
setup_memblock() and dummy_physical_memory_init() fill the entire MEM_SIZE
simulated physical memory with nonzero values by calling fill_memblock().
setup_memblock() is called at the beginning of most tests for
memblock_alloc() functions.

Additional tests for:
- memblock_add()
- memblock_reserve()
- memblock_remove()
- memblock_free()
- memblock_alloc()

Introducing test coverage for:
- memblock_alloc_raw()
- memblock_alloc_try_nid_raw()
- memblock_set_bottom_up()
- memblock_bottom_up()
- memblock_trim_memory()

The tests for the memblock_alloc_*raw() functions test both top-down and
bottom-up allocation directions. To add coverage for memblock_alloc_raw(),
the alloc_api was updated so that it runs through all the existing tests
twice: once for memblock_alloc() and once for memblock_alloc_raw(). When
the tests run memblock_alloc_raw(), they test that the entire memory
region is nonzero instead of testing that it is zero.

Similarly, the alloc_nid_api was updated to run through its tests twice:
once for memblock_alloc_try_nid() and once for
memblock_alloc_try_nid_raw(). When the tests run
memblock_alloc_try_nid_raw(), they test that the entire memory region is
nonzero instead of testing that it is zero.

The patch set also adds labels to verbose output for generic
memblock_alloc*() tests that indicate which allocation direction is set.
The function names of those tests do not include this information.

---
Changelog

v1 -> v2
Updates based on feedback from Shaoqin Huang:
PATCH 1:
- tests/alloc_api.c:
    - Remove fill_memblock() from alloc_no_memory_generic_check().
- tests/common.c, tests/common.h:
    - Change fill_memblock() to file static.
PATCH 3:
- Shaoqin Huang and I discussed using run_top_down() and run_bottom_up()
  even for functions with `top_down` and `bottom_up` in the name to
  maintain a consistent output style. However, this would make the
  output more redundant, so no changes were made.
PATCH 4:
- tests/basic_api.c:
    - Rename instances of r1_size and r2_size to
      new_r1_size and new_r2_size.
PATCH 6:
- tests/alloc_api.c, tests/alloc_nid_api.c, tests/common.h:
    - Change verify_mem_content() to a common function defined in
      common.h.
PATCH 8:
- tests/basic_api.c:
    - Rename instances of r2_base and r2_size to
      new_r2_base and new_r2_size.
---

Rebecca Mckeever (8):
  memblock tests: update tests to check if memblock_alloc zeroed memory
  memblock tests: update zeroed memory check for memblock_alloc_* tests
  memblock tests: add labels to verbose output for generic alloc tests
  memblock tests: add additional tests for basic api and memblock_alloc
  memblock tests: update alloc_api to test memblock_alloc_raw
  memblock tests: update alloc_nid_api to test
    memblock_alloc_try_nid_raw
  memblock tests: add tests for memblock_*bottom_up functions
  memblock tests: add tests for memblock_trim_memory

 tools/testing/memblock/tests/alloc_api.c      | 175 +++-
 .../memblock/tests/alloc_helpers_api.c        |  20 +-
 tools/testing/memblock/tests/alloc_nid_api.c  | 260 +++---
 tools/testing/memblock/tests/basic_api.c      | 767 ++++++++++++++++++
 tools/testing/memblock/tests/common.c         |   7 +
 tools/testing/memblock/tests/common.h         |  53 ++
 6 files changed, 1095 insertions(+), 187 deletions(-)

-- 
2.25.1

[PATCH v2 3/8] memblock tests: add labels to verbose output for generic alloc tests

[Rebecca Mckeever]

Generic tests for memblock_alloc*() functions do not use separate
functions for testing top-down and bottom-up allocation directions.
Therefore, the function name that is displayed in the verbose testing
output does not include the allocation direction.

Add an additional prefix when running generic tests for
memblock_alloc*() functions that indicates which allocation direction is
set. The prefix will be displayed when the tests are run in verbose mode.

Signed-off-by: Rebecca Mckeever <remckee0@gmail.com>
---
 tools/testing/memblock/tests/alloc_api.c      | 36 +++++++------------
 .../memblock/tests/alloc_helpers_api.c        | 12 +++----
 tools/testing/memblock/tests/alloc_nid_api.c  | 36 +++++++------------
 tools/testing/memblock/tests/common.h         | 16 +++++++++
 4 files changed, 44 insertions(+), 56 deletions(-)

diff --git a/tools/testing/memblock/tests/alloc_api.c b/tools/testing/memblock/tests/alloc_api.c
index aefb67557de9..6a9c8e788cac 100644
--- a/tools/testing/memblock/tests/alloc_api.c
+++ b/tools/testing/memblock/tests/alloc_api.c
@@ -751,10 +751,8 @@ static int alloc_after_check(void)
 static int alloc_in_between_check(void)
 {
 	test_print("\tRunning %s...\n", __func__);
-	memblock_set_bottom_up(false);
-	alloc_in_between_generic_check();
-	memblock_set_bottom_up(true);
-	alloc_in_between_generic_check();
+	run_top_down(alloc_in_between_generic_check);
+	run_bottom_up(alloc_in_between_generic_check);
 
 	return 0;
 }
@@ -773,10 +771,8 @@ static int alloc_second_fit_check(void)
 static int alloc_small_gaps_check(void)
 {
 	test_print("\tRunning %s...\n", __func__);
-	memblock_set_bottom_up(false);
-	alloc_small_gaps_generic_check();
-	memblock_set_bottom_up(true);
-	alloc_small_gaps_generic_check();
+	run_top_down(alloc_small_gaps_generic_check);
+	run_bottom_up(alloc_small_gaps_generic_check);
 
 	return 0;
 }
@@ -784,10 +780,8 @@ static int alloc_small_gaps_check(void)
 static int alloc_all_reserved_check(void)
 {
 	test_print("\tRunning %s...\n", __func__);
-	memblock_set_bottom_up(false);
-	alloc_all_reserved_generic_check();
-	memblock_set_bottom_up(true);
-	alloc_all_reserved_generic_check();
+	run_top_down(alloc_all_reserved_generic_check);
+	run_bottom_up(alloc_all_reserved_generic_check);
 
 	return 0;
 }
@@ -795,10 +789,8 @@ static int alloc_all_reserved_check(void)
 static int alloc_no_space_check(void)
 {
 	test_print("\tRunning %s...\n", __func__);
-	memblock_set_bottom_up(false);
-	alloc_no_space_generic_check();
-	memblock_set_bottom_up(true);
-	alloc_no_space_generic_check();
+	run_top_down(alloc_no_space_generic_check);
+	run_bottom_up(alloc_no_space_generic_check);
 
 	return 0;
 }
@@ -806,10 +798,8 @@ static int alloc_no_space_check(void)
 static int alloc_limited_space_check(void)
 {
 	test_print("\tRunning %s...\n", __func__);
-	memblock_set_bottom_up(false);
-	alloc_limited_space_generic_check();
-	memblock_set_bottom_up(true);
-	alloc_limited_space_generic_check();
+	run_top_down(alloc_limited_space_generic_check);
+	run_bottom_up(alloc_limited_space_generic_check);
 
 	return 0;
 }
@@ -817,10 +807,8 @@ static int alloc_limited_space_check(void)
 static int alloc_no_memory_check(void)
 {
 	test_print("\tRunning %s...\n", __func__);
-	memblock_set_bottom_up(false);
-	alloc_no_memory_generic_check();
-	memblock_set_bottom_up(true);
-	alloc_no_memory_generic_check();
+	run_top_down(alloc_no_memory_generic_check);
+	run_bottom_up(alloc_no_memory_generic_check);
 
 	return 0;
 }
diff --git a/tools/testing/memblock/tests/alloc_helpers_api.c b/tools/testing/memblock/tests/alloc_helpers_api.c
index 796527cf3bd2..1ccf02639ad6 100644
--- a/tools/testing/memblock/tests/alloc_helpers_api.c
+++ b/tools/testing/memblock/tests/alloc_helpers_api.c
@@ -357,10 +357,8 @@ static int alloc_from_bottom_up_min_addr_cap_check(void)
 static int alloc_from_simple_check(void)
 {
 	test_print("\tRunning %s...\n", __func__);
-	memblock_set_bottom_up(false);
-	alloc_from_simple_generic_check();
-	memblock_set_bottom_up(true);
-	alloc_from_simple_generic_check();
+	run_top_down(alloc_from_simple_generic_check);
+	run_bottom_up(alloc_from_simple_generic_check);
 
 	return 0;
 }
@@ -368,10 +366,8 @@ static int alloc_from_simple_check(void)
 static int alloc_from_misaligned_check(void)
 {
 	test_print("\tRunning %s...\n", __func__);
-	memblock_set_bottom_up(false);
-	alloc_from_misaligned_generic_check();
-	memblock_set_bottom_up(true);
-	alloc_from_misaligned_generic_check();
+	run_top_down(alloc_from_misaligned_generic_check);
+	run_bottom_up(alloc_from_misaligned_generic_check);
 
 	return 0;
 }
diff --git a/tools/testing/memblock/tests/alloc_nid_api.c b/tools/testing/memblock/tests/alloc_nid_api.c
index 71b7beb35526..82fa8ea36320 100644
--- a/tools/testing/memblock/tests/alloc_nid_api.c
+++ b/tools/testing/memblock/tests/alloc_nid_api.c
@@ -1142,10 +1142,8 @@ static int alloc_try_nid_cap_min_check(void)
 static int alloc_try_nid_min_reserved_check(void)
 {
 	test_print("\tRunning %s...\n", __func__);
-	memblock_set_bottom_up(false);
-	alloc_try_nid_min_reserved_generic_check();
-	memblock_set_bottom_up(true);
-	alloc_try_nid_min_reserved_generic_check();
+	run_top_down(alloc_try_nid_min_reserved_generic_check);
+	run_bottom_up(alloc_try_nid_min_reserved_generic_check);
 
 	return 0;
 }
@@ -1153,10 +1151,8 @@ static int alloc_try_nid_min_reserved_check(void)
 static int alloc_try_nid_max_reserved_check(void)
 {
 	test_print("\tRunning %s...\n", __func__);
-	memblock_set_bottom_up(false);
-	alloc_try_nid_max_reserved_generic_check();
-	memblock_set_bottom_up(true);
-	alloc_try_nid_max_reserved_generic_check();
+	run_top_down(alloc_try_nid_max_reserved_generic_check);
+	run_bottom_up(alloc_try_nid_max_reserved_generic_check);
 
 	return 0;
 }
@@ -1164,10 +1160,8 @@ static int alloc_try_nid_max_reserved_check(void)
 static int alloc_try_nid_exact_address_check(void)
 {
 	test_print("\tRunning %s...\n", __func__);
-	memblock_set_bottom_up(false);
-	alloc_try_nid_exact_address_generic_check();
-	memblock_set_bottom_up(true);
-	alloc_try_nid_exact_address_generic_check();
+	run_top_down(alloc_try_nid_exact_address_generic_check);
+	run_bottom_up(alloc_try_nid_exact_address_generic_check);
 
 	return 0;
 }
@@ -1175,10 +1169,8 @@ static int alloc_try_nid_exact_address_check(void)
 static int alloc_try_nid_reserved_full_merge_check(void)
 {
 	test_print("\tRunning %s...\n", __func__);
-	memblock_set_bottom_up(false);
-	alloc_try_nid_reserved_full_merge_generic_check();
-	memblock_set_bottom_up(true);
-	alloc_try_nid_reserved_full_merge_generic_check();
+	run_top_down(alloc_try_nid_reserved_full_merge_generic_check);
+	run_bottom_up(alloc_try_nid_reserved_full_merge_generic_check);
 
 	return 0;
 }
@@ -1186,10 +1178,8 @@ static int alloc_try_nid_reserved_full_merge_check(void)
 static int alloc_try_nid_reserved_all_check(void)
 {
 	test_print("\tRunning %s...\n", __func__);
-	memblock_set_bottom_up(false);
-	alloc_try_nid_reserved_all_generic_check();
-	memblock_set_bottom_up(true);
-	alloc_try_nid_reserved_all_generic_check();
+	run_top_down(alloc_try_nid_reserved_all_generic_check);
+	run_bottom_up(alloc_try_nid_reserved_all_generic_check);
 
 	return 0;
 }
@@ -1197,10 +1187,8 @@ static int alloc_try_nid_reserved_all_check(void)
 static int alloc_try_nid_low_max_check(void)
 {
 	test_print("\tRunning %s...\n", __func__);
-	memblock_set_bottom_up(false);
-	alloc_try_nid_low_max_generic_check();
-	memblock_set_bottom_up(true);
-	alloc_try_nid_low_max_generic_check();
+	run_top_down(alloc_try_nid_low_max_generic_check);
+	run_bottom_up(alloc_try_nid_low_max_generic_check);
 
 	return 0;
 }
diff --git a/tools/testing/memblock/tests/common.h b/tools/testing/memblock/tests/common.h
index 29756e652e3e..58f84bf2c9ae 100644
--- a/tools/testing/memblock/tests/common.h
+++ b/tools/testing/memblock/tests/common.h
@@ -100,4 +100,20 @@ static inline void test_pass_pop(void)
 	prefix_pop();
 }
 
+static inline void run_top_down(int (*func)())
+{
+	memblock_set_bottom_up(false);
+	prefix_push("top-down");
+	func();
+	prefix_pop();
+}
+
+static inline void run_bottom_up(int (*func)())
+{
+	memblock_set_bottom_up(true);
+	prefix_push("bottom-up");
+	func();
+	prefix_pop();
+}
+
 #endif
-- 
2.25.1

Re: [PATCH v2 3/8] memblock tests: add labels to verbose output for generic alloc tests

[David Hildenbrand]

On 19.08.22 10:34, Rebecca Mckeever wrote:
> Generic tests for memblock_alloc*() functions do not use separate
> functions for testing top-down and bottom-up allocation directions.
> Therefore, the function name that is displayed in the verbose testing
> output does not include the allocation direction.
> 
> Add an additional prefix when running generic tests for
> memblock_alloc*() functions that indicates which allocation direction is
> set. The prefix will be displayed when the tests are run in verbose mode.
> 
> Signed-off-by: Rebecca Mckeever <remckee0@gmail.com>

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 1/8] memblock tests: update tests to check if memblock_alloc zeroed memory

[David Hildenbrand]

On 19.08.22 10:34, Rebecca Mckeever wrote:
> Add an assert in memblock_alloc() tests where allocation is expected to
> occur. The assert checks whether the entire chunk of allocated memory is
> cleared.
> 
> The current memblock_alloc() tests do not check whether the allocated
> memory was zeroed. memblock_alloc() should zero the allocated memory since
> it is a wrapper for memblock_alloc_try_nid().
> 
> Reviewed-by: Shaoqin Huang <shaoqin.huang@intel.com>
> Signed-off-by: Rebecca Mckeever <remckee0@gmail.com>

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 1/8] memblock tests: update tests to check if memblock_alloc zeroed memory

[Mike Rapoport]

On Fri, Aug 19, 2022 at 01:34:49AM -0700, Rebecca Mckeever wrote:
> Add an assert in memblock_alloc() tests where allocation is expected to
> occur. The assert checks whether the entire chunk of allocated memory is
> cleared.
> 
> The current memblock_alloc() tests do not check whether the allocated
> memory was zeroed. memblock_alloc() should zero the allocated memory since
> it is a wrapper for memblock_alloc_try_nid().
> 
> Reviewed-by: Shaoqin Huang <shaoqin.huang@intel.com>
> Signed-off-by: Rebecca Mckeever <remckee0@gmail.com>
> ---
>  tools/testing/memblock/tests/alloc_api.c | 23 +++++++++++++++++++++++
>  tools/testing/memblock/tests/common.c    |  7 +++++++
>  tools/testing/memblock/tests/common.h    | 12 ++++++++++++
>  3 files changed, 42 insertions(+)
> 
> diff --git a/tools/testing/memblock/tests/alloc_api.c b/tools/testing/memblock/tests/alloc_api.c
> index a14f38eb8a89..aefb67557de9 100644
> --- a/tools/testing/memblock/tests/alloc_api.c
> +++ b/tools/testing/memblock/tests/alloc_api.c
> @@ -22,6 +22,8 @@ static int alloc_top_down_simple_check(void)
>  	allocated_ptr = memblock_alloc(size, SMP_CACHE_BYTES);
>  
>  	ASSERT_NE(allocated_ptr, NULL);
> +	ASSERT_MEM_EQ((char *)allocated_ptr, 0, size);
> +

Can we please hide the casting inside ASSERT_MEM_EQ()?

Like if ASSERT_MEM_EQ() were a function its declaration would be

bool ASSERT_MEM_EQ(void *mem, char val, size_t size);

>  	ASSERT_EQ(rgn->size, size);
>  	ASSERT_EQ(rgn->base, expected_start);
>  

-- 
Sincerely yours,
Mike.

Re: [PATCH v2 4/8] memblock tests: add additional tests for basic api and memblock_alloc

[David Hildenbrand]

On 19.08.22 10:34, Rebecca Mckeever wrote:
> Add tests for memblock_add(), memblock_reserve(), memblock_remove(),
> memblock_free(), and memblock_alloc() for the following test scenarios.
> 
> memblock_add() and memblock_reserve():
> - add/reserve a memory block in the gap between two existing memory
>   blocks, and check that the blocks are merged into one region
> - try to add/reserve memblock regions that extend past PHYS_ADDR_MAX
> 
> memblock_remove() and memblock_free():
> - remove/free a region when it is the only available region
>     + These tests ensure that the first region is overwritten with a
>       "dummy" region when the last remaining region of that type is
>       removed or freed.
> - remove/free() a region that overlaps with two existing regions of the
>   relevant type
> - try to remove/free memblock regions that extend past PHYS_ADDR_MAX
> 
> memblock_alloc():
> - try to allocate a region that is larger than the total size of available
>   memory (memblock.memory)
> 
> Signed-off-by: Rebecca Mckeever <remckee0@gmail.com>

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 5/8] memblock tests: update alloc_api to test memblock_alloc_raw

[David Hildenbrand]

On 19.08.22 10:34, Rebecca Mckeever wrote:
> Update memblock_alloc() tests so that they test either memblock_alloc()
> or memblock_alloc_raw() depending on the value of alloc_test_flags. Run
> through all the existing tests in memblock_alloc_api twice: once for
> memblock_alloc() and once for memblock_alloc_raw().
> 
> When the tests run memblock_alloc(), they test that the entire memory
> region is zero. When the tests run memblock_alloc_raw(), they test that
> the entire memory region is nonzero.

Could add a comment stating that we initialize the content to nonzero in
that case, and expect it to remain unchanged (== not zeroed).

> 
> Signed-off-by: Rebecca Mckeever <remckee0@gmail.com>
> ---
>  tools/testing/memblock/tests/alloc_api.c | 98 ++++++++++++++++--------
>  tools/testing/memblock/tests/common.h    | 25 ++++++
>  2 files changed, 90 insertions(+), 33 deletions(-)
> 
> diff --git a/tools/testing/memblock/tests/alloc_api.c b/tools/testing/memblock/tests/alloc_api.c
> index 65bff77dd55b..cf67687ae044 100644
> --- a/tools/testing/memblock/tests/alloc_api.c
> +++ b/tools/testing/memblock/tests/alloc_api.c
> @@ -1,6 +1,29 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  #include "alloc_api.h"
>  
> +static const char * const func_testing[] = {
> +	"memblock_alloc",
> +	"memblock_alloc_raw"
> +};
> +
> +static int alloc_test_flags = TEST_ZEROED;
> +
> +static inline const char * const get_func_testing(int flags)
> +{
> +	if (flags & TEST_RAW)
> +		return func_testing[1];
> +	else
> +		return func_testing[0];

No need for the else, you can return directly.

Can we avoid the func_testing array?


Persoally, I consider the "get_func_testing()" name a bit confusing.

get_memblock_alloc_name() ?


> diff --git a/tools/testing/memblock/tests/common.h b/tools/testing/memblock/tests/common.h
> index 58f84bf2c9ae..4fd3534ff955 100644
> --- a/tools/testing/memblock/tests/common.h
> +++ b/tools/testing/memblock/tests/common.h
> @@ -12,6 +12,11 @@
>  
>  #define MEM_SIZE SZ_16K
>  
> +enum test_flags {
> +	TEST_ZEROED = 0x0,
> +	TEST_RAW = 0x1
> +};

I'd have called this

enum test_flags {
	/* No special request. */
	TEST_F_NONE = 0x0,
	/* Perform raw allocations (no zeroing of memory).
	TEST_F_RAW = 0x1,
};

Further, I'd just have use #define for the flags.

> +
>  /**
>   * ASSERT_EQ():
>   * Check the condition
> @@ -63,6 +68,18 @@
>  	} \
>  } while (0)
>  
> +/**
> + * ASSERT_MEM_NE():
> + * Check that none of the first @_size bytes of @_seen are equal to @_expected.
> + * If false, print failed test message (if running with --verbose) and then
> + * assert.
> + */
> +#define ASSERT_MEM_NE(_seen, _expected, _size) do { \
> +	for (int _i = 0; _i < (_size); _i++) { \
> +		ASSERT_NE((_seen)[_i], (_expected)); \
> +	} \
> +} while (0)
> +
>  #define PREFIX_PUSH() prefix_push(__func__)
>  
>  /*
> @@ -116,4 +133,12 @@ static inline void run_bottom_up(int (*func)())
>  	prefix_pop();
>  }
>  
> +static inline void verify_mem_content(void *mem, int size, int flags)

nit: why use verify here when the other functions "assert". I'd have
called this something like "assert_mem_content()"


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 5/8] memblock tests: update alloc_api to test memblock_alloc_raw

[Rebecca Mckeever]

On Tue, Aug 23, 2022 at 11:49:46AM +0200, David Hildenbrand wrote:
> On 19.08.22 10:34, Rebecca Mckeever wrote:
> > Update memblock_alloc() tests so that they test either memblock_alloc()
> > or memblock_alloc_raw() depending on the value of alloc_test_flags. Run
> > through all the existing tests in memblock_alloc_api twice: once for
> > memblock_alloc() and once for memblock_alloc_raw().
> > 
> > When the tests run memblock_alloc(), they test that the entire memory
> > region is zero. When the tests run memblock_alloc_raw(), they test that
> > the entire memory region is nonzero.
> 
> Could add a comment stating that we initialize the content to nonzero in
> that case, and expect it to remain unchanged (== not zeroed).
> 
> > 
> > Signed-off-by: Rebecca Mckeever <remckee0@gmail.com>
> > ---
> >  tools/testing/memblock/tests/alloc_api.c | 98 ++++++++++++++++--------
> >  tools/testing/memblock/tests/common.h    | 25 ++++++
> >  2 files changed, 90 insertions(+), 33 deletions(-)
> > 
> > diff --git a/tools/testing/memblock/tests/alloc_api.c b/tools/testing/memblock/tests/alloc_api.c
> > index 65bff77dd55b..cf67687ae044 100644
> > --- a/tools/testing/memblock/tests/alloc_api.c
> > +++ b/tools/testing/memblock/tests/alloc_api.c
> > @@ -1,6 +1,29 @@
> >  // SPDX-License-Identifier: GPL-2.0-or-later
> >  #include "alloc_api.h"
> >  
> > +static const char * const func_testing[] = {
> > +	"memblock_alloc",
> > +	"memblock_alloc_raw"
> > +};
> > +
> > +static int alloc_test_flags = TEST_ZEROED;
> > +
> > +static inline const char * const get_func_testing(int flags)
> > +{
> > +	if (flags & TEST_RAW)
> > +		return func_testing[1];
> > +	else
> > +		return func_testing[0];
> 
> No need for the else, you can return directly.
> 
> Can we avoid the func_testing array?
> 
> 
> Persoally, I consider the "get_func_testing()" name a bit confusing.
> 
> get_memblock_alloc_name() ?
> 
> 
> > diff --git a/tools/testing/memblock/tests/common.h b/tools/testing/memblock/tests/common.h
> > index 58f84bf2c9ae..4fd3534ff955 100644
> > --- a/tools/testing/memblock/tests/common.h
> > +++ b/tools/testing/memblock/tests/common.h
> > @@ -12,6 +12,11 @@
> >  
> >  #define MEM_SIZE SZ_16K
> >  
> > +enum test_flags {
> > +	TEST_ZEROED = 0x0,
> > +	TEST_RAW = 0x1
> > +};
> 
> I'd have called this
> 
> enum test_flags {
> 	/* No special request. */
> 	TEST_F_NONE = 0x0,
> 	/* Perform raw allocations (no zeroing of memory).
> 	TEST_F_RAW = 0x1,
> };
> 
> Further, I'd just have use #define for the flags.
> 
Do you mean use two #defines instead of the enum? I thought enums were
preferred when defining related constants.

> > +
> >  /**
> >   * ASSERT_EQ():
> >   * Check the condition
> > @@ -63,6 +68,18 @@
> >  	} \
> >  } while (0)
> >  
> > +/**
> > + * ASSERT_MEM_NE():
> > + * Check that none of the first @_size bytes of @_seen are equal to @_expected.
> > + * If false, print failed test message (if running with --verbose) and then
> > + * assert.
> > + */
> > +#define ASSERT_MEM_NE(_seen, _expected, _size) do { \
> > +	for (int _i = 0; _i < (_size); _i++) { \
> > +		ASSERT_NE((_seen)[_i], (_expected)); \
> > +	} \
> > +} while (0)
> > +
> >  #define PREFIX_PUSH() prefix_push(__func__)
> >  
> >  /*
> > @@ -116,4 +133,12 @@ static inline void run_bottom_up(int (*func)())
> >  	prefix_pop();
> >  }
> >  
> > +static inline void verify_mem_content(void *mem, int size, int flags)
> 
> nit: why use verify here when the other functions "assert". I'd have
> called this something like "assert_mem_content()"
> 
> 
> -- 
> Thanks,
> 
> David / dhildenb
> 
> 
Thanks,
Rebecca

Re: [PATCH v2 5/8] memblock tests: update alloc_api to test memblock_alloc_raw

[David Hildenbrand]

On 25.08.22 23:35, Rebecca Mckeever wrote:
> On Tue, Aug 23, 2022 at 11:49:46AM +0200, David Hildenbrand wrote:
>> On 19.08.22 10:34, Rebecca Mckeever wrote:
>>> Update memblock_alloc() tests so that they test either memblock_alloc()
>>> or memblock_alloc_raw() depending on the value of alloc_test_flags. Run
>>> through all the existing tests in memblock_alloc_api twice: once for
>>> memblock_alloc() and once for memblock_alloc_raw().
>>>
>>> When the tests run memblock_alloc(), they test that the entire memory
>>> region is zero. When the tests run memblock_alloc_raw(), they test that
>>> the entire memory region is nonzero.
>>
>> Could add a comment stating that we initialize the content to nonzero in
>> that case, and expect it to remain unchanged (== not zeroed).
>>
>>>
>>> Signed-off-by: Rebecca Mckeever <remckee0@gmail.com>
>>> ---
>>>  tools/testing/memblock/tests/alloc_api.c | 98 ++++++++++++++++--------
>>>  tools/testing/memblock/tests/common.h    | 25 ++++++
>>>  2 files changed, 90 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/tools/testing/memblock/tests/alloc_api.c b/tools/testing/memblock/tests/alloc_api.c
>>> index 65bff77dd55b..cf67687ae044 100644
>>> --- a/tools/testing/memblock/tests/alloc_api.c
>>> +++ b/tools/testing/memblock/tests/alloc_api.c
>>> @@ -1,6 +1,29 @@
>>>  // SPDX-License-Identifier: GPL-2.0-or-later
>>>  #include "alloc_api.h"
>>>  
>>> +static const char * const func_testing[] = {
>>> +	"memblock_alloc",
>>> +	"memblock_alloc_raw"
>>> +};
>>> +
>>> +static int alloc_test_flags = TEST_ZEROED;
>>> +
>>> +static inline const char * const get_func_testing(int flags)
>>> +{
>>> +	if (flags & TEST_RAW)
>>> +		return func_testing[1];
>>> +	else
>>> +		return func_testing[0];
>>
>> No need for the else, you can return directly.
>>
>> Can we avoid the func_testing array?
>>
>>
>> Persoally, I consider the "get_func_testing()" name a bit confusing.
>>
>> get_memblock_alloc_name() ?
>>
>>
>>> diff --git a/tools/testing/memblock/tests/common.h b/tools/testing/memblock/tests/common.h
>>> index 58f84bf2c9ae..4fd3534ff955 100644
>>> --- a/tools/testing/memblock/tests/common.h
>>> +++ b/tools/testing/memblock/tests/common.h
>>> @@ -12,6 +12,11 @@
>>>  
>>>  #define MEM_SIZE SZ_16K
>>>  
>>> +enum test_flags {
>>> +	TEST_ZEROED = 0x0,
>>> +	TEST_RAW = 0x1
>>> +};
>>
>> I'd have called this
>>
>> enum test_flags {
>> 	/* No special request. */
>> 	TEST_F_NONE = 0x0,
>> 	/* Perform raw allocations (no zeroing of memory).
>> 	TEST_F_RAW = 0x1,
>> };
>>
>> Further, I'd just have use #define for the flags.
>>
> Do you mean use two #defines instead of the enum? I thought enums were
> preferred when defining related constants.

I guess we have a wild mixture of raw define, enums and __bitwise +
defines nowdays.

E.g., take a look at include/linux/rmap.h "typedef int __bitwise rmap_t"
and how it's used --  that seems to be the new "best" solution for use
in the kernel.

Having that said, feel free to just let it be an enum :)

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 6/8] memblock tests: update alloc_nid_api to test memblock_alloc_try_nid_raw

[David Hildenbrand]

On 19.08.22 10:34, Rebecca Mckeever wrote:
> Update memblock_alloc_try_nid() tests so that they test either
> memblock_alloc_try_nid() or memblock_alloc_try_nid_raw() depending on the
> value of alloc_nid_test_flags. Run through all the existing tests in
> alloc_nid_api twice: once for memblock_alloc_try_nid() and once for
> memblock_alloc_try_nid_raw().
> 
> When the tests run memblock_alloc_try_nid(), they test that the entire
> memory region is zero. When the tests run memblock_alloc_try_nid_raw(),
> they test that the entire memory region is nonzero.
> 
> Signed-off-by: Rebecca Mckeever <remckee0@gmail.com>
> ---
>  tools/testing/memblock/tests/alloc_nid_api.c | 188 ++++++++++++-------
>  1 file changed, 119 insertions(+), 69 deletions(-)
> 
> diff --git a/tools/testing/memblock/tests/alloc_nid_api.c b/tools/testing/memblock/tests/alloc_nid_api.c
> index 82fa8ea36320..2c1d5035e057 100644
> --- a/tools/testing/memblock/tests/alloc_nid_api.c
> +++ b/tools/testing/memblock/tests/alloc_nid_api.c
> @@ -1,6 +1,34 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  #include "alloc_nid_api.h"
>  
> +static const char * const func_testing[] = {
> +	"memblock_alloc_try_nid",
> +	"memblock_alloc_try_nid_raw"
> +};
> +
> +static int alloc_nid_test_flags = TEST_ZEROED;
> +
> +static inline const char * const get_func_testing(int flags)
> +{
> +	if (flags & TEST_RAW)
> +		return func_testing[1];
> +	else
> +		return func_testing[0];
> +}

Same comments as for patch #5. Otherwise looks good.


-- 
Thanks,

David / dhildenb

Re: [PATCH v2 8/8] memblock tests: add tests for memblock_trim_memory

[David Hildenbrand]

On 19.08.22 10:34, Rebecca Mckeever wrote:
> Add tests for memblock_trim_memory() for the following scenarios:
> - all regions aligned
> - one region unalign that is smaller than the alignment

s/region unalign/unaligned region/

> - one region unaligned at the base
> - one region unaligned at the end

"unaligned region" ?

[...]

>  
> +/*
> + * A test that tries to trim memory when both ends of the memory region are
> + * aligned. Expect that the memory will not be trimmed. Expect the counter to
> + * not be updated.
> + */
> +static int memblock_trim_memory_aligned_check(void)
> +{
> +	struct memblock_region *rgn;
> +	phys_addr_t alignment = SMP_CACHE_BYTES;

Could make that "const" -- same applies to other functions.

> +
> +	rgn = &memblock.memory.regions[0];
> +
> +	struct region r = {
> +		.base = alignment,
> +		.size = alignment * 4
> +	};
> +

[[[.]

> +static int memblock_trim_memory_checks(void)
> +{
> +	prefix_reset();
> +	prefix_push(FUNC_TRIM);
> +	test_print("Running %s tests...\n", FUNC_TRIM);

Just a note that this could have been

test_print("Running " FUNC_TRIM " tests...\n");

But it's good to keep this consistent for all tests.


Nice test cases!

-- 
Thanks,

David / dhildenb