| Message ID | 20220728190051.237437-1-shaoqin.huang@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | memblock test: Add test to memblock_add() 129th region | expand |
On 7/30/2022 1:07 PM, Rebecca Mckeever wrote: > On Thu, Jul 28, 2022 at 01:00:47PM -0600, shaoqin.huang@intel.com wrote: >> From: Shaoqin Huang <shaoqin.huang@intel.com> >> > I tested this out, and everything is working well. I have a couple of > suggestions for improvements. > >> Add 129th region into the memblock, and this will trigger the >> memblock_double_array() function, this needs valid memory regions. So >> using dummy_physical_memory_init() to allocate some valid memory, when >> memblock_double_array() choose a new memory region from memory.regions, >> it will always choose a valid memory region if we add all valid memory >> region, so the memblock_double_array() must success. >> >> Another thing should be done is to restore the memory.regions after >> memblock_double_array(), due to now the memory.regions is pointing to a >> memory region allocated by dummy_physical_memory_init(). And it will >> affect the subsequent tests if we don't restore the memory region. So >> Simply record the origin region, and restore it after the test. >> >> Signed-off-by: Shaoqin Huang <shaoqin.huang@intel.com> >> --- >> tools/testing/memblock/tests/basic_api.c | 65 ++++++++++++++++++++++++ >> tools/testing/memblock/tests/common.c | 9 ++-- >> tools/testing/memblock/tests/common.h | 5 ++ >> 3 files changed, 76 insertions(+), 3 deletions(-) >> >> diff --git a/tools/testing/memblock/tests/basic_api.c b/tools/testing/memblock/tests/basic_api.c >> index 66f46f261e66..ded93f97d98e 100644 >> --- a/tools/testing/memblock/tests/basic_api.c >> +++ b/tools/testing/memblock/tests/basic_api.c >> @@ -326,6 +326,70 @@ static int memblock_add_twice_check(void) >> return 0; >> } >> >> +static int memblock_add_many_check(void) >> +{ >> + int i; >> + void *base[INIT_MEMBLOCK_REGIONS + 1]; >> + void *orig_region; >> + struct region r = { >> + .base = SZ_16K, >> + .size = MEM_SIZE, >> + }; >> + >> + PREFIX_PUSH(); >> + >> + reset_memblock_regions(); >> + memblock_allow_resize(); >> + >> + for (i = 0; i < INIT_MEMBLOCK_REGIONS; i++) { >> + dummy_physical_memory_init(); >> + append_memblock(); >> + base[i] = memory_block.base; >> + >> + assert(memblock.memory.cnt == i + 1); > > Maybe you could call the ASSERT_EQ() macro here instead of directly > calling assert. That way, if the test fails in verbose mode, it will print > out the test prefix. This applies to the other asserts as well. > Yes, It should use the ASSERT_EQ(). Thanks for the notification. >> + assert(memblock.memory.total_size == (i + 1) * MEM_SIZE); >> + } >> + >> + orig_region = memblock.memory.regions; >> + >> + /* This adds the 129 memory_region, and makes it double array. */ >> + dummy_physical_memory_init(); >> + append_memblock(); >> + base[i] = memory_block.base; >> + >> + assert(memblock.memory.cnt == INIT_MEMBLOCK_REGIONS + 1); >> + assert(memblock.memory.total_size == (INIT_MEMBLOCK_REGIONS + 1) * MEM_SIZE); >> + assert(memblock.memory.max == INIT_MEMBLOCK_REGIONS * 2); >> + >> + /* The base is very small, so it should be insert to the first region. */ >> + memblock_add(r.base, r.size); >> + assert(memblock.memory.regions[0].base == r.base); >> + assert(memblock.memory.regions[0].size == r.size); >> + >> + assert(memblock.memory.cnt == INIT_MEMBLOCK_REGIONS + 2); >> + assert(memblock.memory.total_size == (INIT_MEMBLOCK_REGIONS + 2) * MEM_SIZE); >> + assert(memblock.memory.max == INIT_MEMBLOCK_REGIONS * 2); >> + >> + /* Free these allocated memory. */ >> + for (i = 0; i < INIT_MEMBLOCK_REGIONS + 1; i++) { >> + memory_block.base = base[i]; >> + dummy_physical_memory_cleanup(); >> + } >> + > This could be moved to a function in common.c since it may be useful if > we write similar tests later. For example: > > void dummy_physical_memory_many_cleanup(void *base[], int cnt) > { > for (int i = 0; i < cnt; i++) { > memory_block.base = base[i]; > dummy_physical_memory_cleanup(); > } > } > Thanks for your advice. The another thing we should make sure at here is the base[i] is both allocated from dummy_physical_memory_init(). > Since the other usages of memory_block in basic_api.c do not need to > modify it, you could then replace those usages with a call to a function > from common.c like: > > void *get_memory_block_base(void) > { > return memory_block.base; > } > > to avoid adding memory_block to common.h and changing to non-static in > common.c. > I will modify it. Thanks, Shaoqin >> + /* >> + * The current memory.regions is occupying a range of memory that >> + * allocated from dummy_physical_memory_init(). After free the memory, >> + * we must not use it. So restore the origin memory region to make sure >> + * the tests can run as normal and not affected by the double array. >> + */ >> + memblock.memory.regions = orig_region; >> + memblock.memory.cnt = INIT_MEMBLOCK_REGIONS; >> + >> + test_pass_pop(); >> + >> + return 0; >> +} >> + >> static int memblock_add_checks(void) >> { >> prefix_reset(); >> @@ -339,6 +403,7 @@ static int memblock_add_checks(void) >> memblock_add_overlap_bottom_check(); >> memblock_add_within_check(); >> memblock_add_twice_check(); >> + memblock_add_many_check(); >> >> prefix_pop(); >> >> diff --git a/tools/testing/memblock/tests/common.c b/tools/testing/memblock/tests/common.c >> index e43b2676af81..4741e860123a 100644 >> --- a/tools/testing/memblock/tests/common.c >> +++ b/tools/testing/memblock/tests/common.c >> @@ -5,12 +5,10 @@ >> #include <linux/memory_hotplug.h> >> #include <linux/build_bug.h> >> >> -#define INIT_MEMBLOCK_REGIONS 128 >> -#define INIT_MEMBLOCK_RESERVED_REGIONS INIT_MEMBLOCK_REGIONS >> #define PREFIXES_MAX 15 >> #define DELIM ": " >> >> -static struct test_memory memory_block; >> +struct test_memory memory_block; >> static const char __maybe_unused *prefixes[PREFIXES_MAX]; >> static int __maybe_unused nr_prefixes; >> >> @@ -64,6 +62,11 @@ void setup_memblock(void) >> memblock_add((phys_addr_t)memory_block.base, MEM_SIZE); >> } >> >> +void append_memblock(void) >> +{ >> + memblock_add((phys_addr_t)memory_block.base, MEM_SIZE); >> +} >> + >> void dummy_physical_memory_init(void) >> { >> memory_block.base = malloc(MEM_SIZE); >> diff --git a/tools/testing/memblock/tests/common.h b/tools/testing/memblock/tests/common.h >> index 3e7f23d341d7..8946a3b77f24 100644 >> --- a/tools/testing/memblock/tests/common.h >> +++ b/tools/testing/memblock/tests/common.h >> @@ -11,6 +11,8 @@ >> #include <../selftests/kselftest.h> >> >> #define MEM_SIZE SZ_16K >> +#define INIT_MEMBLOCK_REGIONS 128 >> +#define INIT_MEMBLOCK_RESERVED_REGIONS INIT_MEMBLOCK_REGIONS >> >> /** >> * ASSERT_EQ(): >> @@ -65,9 +67,12 @@ struct region { >> phys_addr_t size; >> }; >> >> +extern struct test_memory memory_block; >> + >> void reset_memblock_regions(void); >> void reset_memblock_attributes(void); >> void setup_memblock(void); >> +void append_memblock(void); >> void dummy_physical_memory_init(void); >> void dummy_physical_memory_cleanup(void); >> void parse_args(int argc, char **argv); >> -- >> 2.30.2 >> > Thanks, > Rebecca
diff --git a/tools/testing/memblock/tests/basic_api.c b/tools/testing/memblock/tests/basic_api.c index 66f46f261e66..ded93f97d98e 100644 --- a/tools/testing/memblock/tests/basic_api.c +++ b/tools/testing/memblock/tests/basic_api.c @@ -326,6 +326,70 @@ static int memblock_add_twice_check(void) return 0; } +static int memblock_add_many_check(void) +{ + int i; + void *base[INIT_MEMBLOCK_REGIONS + 1]; + void *orig_region; + struct region r = { + .base = SZ_16K, + .size = MEM_SIZE, + }; + + PREFIX_PUSH(); + + reset_memblock_regions(); + memblock_allow_resize(); + + for (i = 0; i < INIT_MEMBLOCK_REGIONS; i++) { + dummy_physical_memory_init(); + append_memblock(); + base[i] = memory_block.base; + + assert(memblock.memory.cnt == i + 1); + assert(memblock.memory.total_size == (i + 1) * MEM_SIZE); + } + + orig_region = memblock.memory.regions; + + /* This adds the 129 memory_region, and makes it double array. */ + dummy_physical_memory_init(); + append_memblock(); + base[i] = memory_block.base; + + assert(memblock.memory.cnt == INIT_MEMBLOCK_REGIONS + 1); + assert(memblock.memory.total_size == (INIT_MEMBLOCK_REGIONS + 1) * MEM_SIZE); + assert(memblock.memory.max == INIT_MEMBLOCK_REGIONS * 2); + + /* The base is very small, so it should be insert to the first region. */ + memblock_add(r.base, r.size); + assert(memblock.memory.regions[0].base == r.base); + assert(memblock.memory.regions[0].size == r.size); + + assert(memblock.memory.cnt == INIT_MEMBLOCK_REGIONS + 2); + assert(memblock.memory.total_size == (INIT_MEMBLOCK_REGIONS + 2) * MEM_SIZE); + assert(memblock.memory.max == INIT_MEMBLOCK_REGIONS * 2); + + /* Free these allocated memory. */ + for (i = 0; i < INIT_MEMBLOCK_REGIONS + 1; i++) { + memory_block.base = base[i]; + dummy_physical_memory_cleanup(); + } + + /* + * The current memory.regions is occupying a range of memory that + * allocated from dummy_physical_memory_init(). After free the memory, + * we must not use it. So restore the origin memory region to make sure + * the tests can run as normal and not affected by the double array. + */ + memblock.memory.regions = orig_region; + memblock.memory.cnt = INIT_MEMBLOCK_REGIONS; + + test_pass_pop(); + + return 0; +} + static int memblock_add_checks(void) { prefix_reset(); @@ -339,6 +403,7 @@ static int memblock_add_checks(void) memblock_add_overlap_bottom_check(); memblock_add_within_check(); memblock_add_twice_check(); + memblock_add_many_check(); prefix_pop(); diff --git a/tools/testing/memblock/tests/common.c b/tools/testing/memblock/tests/common.c index e43b2676af81..4741e860123a 100644 --- a/tools/testing/memblock/tests/common.c +++ b/tools/testing/memblock/tests/common.c @@ -5,12 +5,10 @@ #include <linux/memory_hotplug.h> #include <linux/build_bug.h> -#define INIT_MEMBLOCK_REGIONS 128 -#define INIT_MEMBLOCK_RESERVED_REGIONS INIT_MEMBLOCK_REGIONS #define PREFIXES_MAX 15 #define DELIM ": " -static struct test_memory memory_block; +struct test_memory memory_block; static const char __maybe_unused *prefixes[PREFIXES_MAX]; static int __maybe_unused nr_prefixes; @@ -64,6 +62,11 @@ void setup_memblock(void) memblock_add((phys_addr_t)memory_block.base, MEM_SIZE); } +void append_memblock(void) +{ + memblock_add((phys_addr_t)memory_block.base, MEM_SIZE); +} + void dummy_physical_memory_init(void) { memory_block.base = malloc(MEM_SIZE); diff --git a/tools/testing/memblock/tests/common.h b/tools/testing/memblock/tests/common.h index 3e7f23d341d7..8946a3b77f24 100644 --- a/tools/testing/memblock/tests/common.h +++ b/tools/testing/memblock/tests/common.h @@ -11,6 +11,8 @@ #include <../selftests/kselftest.h> #define MEM_SIZE SZ_16K +#define INIT_MEMBLOCK_REGIONS 128 +#define INIT_MEMBLOCK_RESERVED_REGIONS INIT_MEMBLOCK_REGIONS /** * ASSERT_EQ(): @@ -65,9 +67,12 @@ struct region { phys_addr_t size; }; +extern struct test_memory memory_block; + void reset_memblock_regions(void); void reset_memblock_attributes(void); void setup_memblock(void); +void append_memblock(void); void dummy_physical_memory_init(void); void dummy_physical_memory_cleanup(void); void parse_args(int argc, char **argv);