| Message ID | cover.1660897732.git.remckee0@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | memblock tests: update and extend memblock simulator | expand |
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>
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>
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()"
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.
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!
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); >
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
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 :)
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(-)