| Message ID | 20240414004531.6601-2-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/6] mm/memblock: reduce the two round insertion of memblock_add_range() | expand |
On Sun, Apr 14, 2024 at 12:45:27AM +0000, Wei Yang wrote: > After previous change, we may double the array based on the position of > the new range. > > Let's make sure the 129th memory block would double the size correctly > at all possible position. Rather than rewrite an existing test, just add a new one. Besides, it would be more interesting to test additions to memblock.reserved and a mix of memblock_add() and memblock_reserve() that will require resizing the memblock arrays. > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > --- > tools/testing/memblock/tests/basic_api.c | 132 +++++++++++++---------- > 1 file changed, 73 insertions(+), 59 deletions(-) > > diff --git a/tools/testing/memblock/tests/basic_api.c b/tools/testing/memblock/tests/basic_api.c > index f317fe691fc4..f1569ebb9872 100644 > --- a/tools/testing/memblock/tests/basic_api.c > +++ b/tools/testing/memblock/tests/basic_api.c > @@ -431,84 +431,98 @@ static int memblock_add_near_max_check(void) > */ > static int memblock_add_many_check(void) > { > - int i; > + int i, skip; > void *orig_region; > struct region r = { > .base = SZ_16K, > .size = SZ_16K, > }; > phys_addr_t new_memory_regions_size; > - phys_addr_t base, size = SZ_64; > + phys_addr_t base, base_start, size = SZ_64; > phys_addr_t gap_size = SZ_64; > > PREFIX_PUSH(); > > - reset_memblock_regions(); > - memblock_allow_resize(); > - > - dummy_physical_memory_init(); > - /* > - * We allocated enough memory by using dummy_physical_memory_init(), and > - * split it into small block. First we split a large enough memory block > - * as the memory region which will be choosed by memblock_double_array(). > - */ > - base = PAGE_ALIGN(dummy_physical_memory_base()); > - new_memory_regions_size = PAGE_ALIGN(INIT_MEMBLOCK_REGIONS * 2 * > - sizeof(struct memblock_region)); > - memblock_add(base, new_memory_regions_size); > - > - /* This is the base of small memory block. */ > - base += new_memory_regions_size + gap_size; > - > - orig_region = memblock.memory.regions; > + /* Add the 129th memory block for all possible positions*/ > + for (skip = 0; skip < INIT_MEMBLOCK_REGIONS; skip++) { > + reset_memblock_regions(); > + memblock_allow_resize(); > > - for (i = 0; i < INIT_MEMBLOCK_REGIONS; i++) { > + dummy_physical_memory_init(); > /* > - * Add these small block to fulfill the memblock. We keep a > - * gap between the nearby memory to avoid being merged. > + * We allocated enough memory by using dummy_physical_memory_init(), and > + * split it into small block. First we split a large enough memory block > + * as the memory region which will be choosed by memblock_double_array(). > */ > - memblock_add(base, size); > - base += size + gap_size; > - > - ASSERT_EQ(memblock.memory.cnt, i + 2); > + base = PAGE_ALIGN(dummy_physical_memory_base()); > + new_memory_regions_size = PAGE_ALIGN(INIT_MEMBLOCK_REGIONS * 2 * > + sizeof(struct memblock_region)); > + memblock_add(base, new_memory_regions_size); > + > + /* This is the base of small memory block. */ > + base_start = base += new_memory_regions_size + gap_size; > + orig_region = memblock.memory.regions; > + > + for (i = 0; i < INIT_MEMBLOCK_REGIONS; i++, base += size + gap_size) { > + if (i == skip) > + continue; > + /* > + * Add these small block to fulfill the memblock. We keep a > + * gap between the nearby memory to avoid being merged. > + */ > + memblock_add(base, size); > + > + if (i < skip) { > + ASSERT_EQ(memblock.memory.cnt, i + 2); > + ASSERT_EQ(memblock.memory.total_size, > + new_memory_regions_size + (i + 1) * size); > + } else { > + ASSERT_EQ(memblock.memory.cnt, i + 1); > + ASSERT_EQ(memblock.memory.total_size, > + new_memory_regions_size + i * size); > + } > + } > + > + /* Add the skipped one to trigger memblock_double_array() */ > + memblock_add(base_start + skip * (size + gap_size), size); > + ASSERT_EQ(memblock.memory.cnt, i + 1); > ASSERT_EQ(memblock.memory.total_size, new_memory_regions_size + > - (i + 1) * size); > - } > + i * size); > + /* > + * At there, memblock_double_array() has been succeed, check if it > + * update the memory.max. > + */ > + ASSERT_EQ(memblock.memory.max, INIT_MEMBLOCK_REGIONS * 2); > > - /* > - * At there, memblock_double_array() has been succeed, check if it > - * update the memory.max. > - */ > - ASSERT_EQ(memblock.memory.max, INIT_MEMBLOCK_REGIONS * 2); > + /* memblock_double_array() will reserve the memory it used. Check it. */ > + ASSERT_EQ(memblock.reserved.cnt, 1); > + ASSERT_EQ(memblock.reserved.total_size, new_memory_regions_size); > > - /* memblock_double_array() will reserve the memory it used. Check it. */ > - ASSERT_EQ(memblock.reserved.cnt, 1); > - ASSERT_EQ(memblock.reserved.total_size, new_memory_regions_size); > - > - /* > - * Now memblock_double_array() works fine. Let's check after the > - * double_array(), the memblock_add() still works as normal. > - */ > - memblock_add(r.base, r.size); > - ASSERT_EQ(memblock.memory.regions[0].base, r.base); > - ASSERT_EQ(memblock.memory.regions[0].size, r.size); > + /* > + * Now memblock_double_array() works fine. Let's check after the > + * double_array(), the memblock_add() still works as normal. > + */ > + memblock_add(r.base, r.size); > + ASSERT_EQ(memblock.memory.regions[0].base, r.base); > + ASSERT_EQ(memblock.memory.regions[0].size, r.size); > > - ASSERT_EQ(memblock.memory.cnt, INIT_MEMBLOCK_REGIONS + 2); > - ASSERT_EQ(memblock.memory.total_size, INIT_MEMBLOCK_REGIONS * size + > - new_memory_regions_size + > - r.size); > - ASSERT_EQ(memblock.memory.max, INIT_MEMBLOCK_REGIONS * 2); > + ASSERT_EQ(memblock.memory.cnt, INIT_MEMBLOCK_REGIONS + 2); > + ASSERT_EQ(memblock.memory.total_size, INIT_MEMBLOCK_REGIONS * size + > + new_memory_regions_size + > + r.size); > + ASSERT_EQ(memblock.memory.max, INIT_MEMBLOCK_REGIONS * 2); > > - dummy_physical_memory_cleanup(); > + 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; > + /* > + * 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(); > > -- > 2.34.1 >
On Mon, Apr 15, 2024 at 06:19:42PM +0300, Mike Rapoport wrote: >On Sun, Apr 14, 2024 at 12:45:27AM +0000, Wei Yang wrote: >> After previous change, we may double the array based on the position of >> the new range. >> >> Let's make sure the 129th memory block would double the size correctly >> at all possible position. > >Rather than rewrite an existing test, just add a new one. Ok, will add a new one for this. >Besides, it would be more interesting to test additions to >memblock.reserved and a mix of memblock_add() and memblock_reserve() that >will require resizing the memblock arrays. I don't get this very clearly. Would you mind give more hint? > >> -- >> 2.34.1 >> > >-- >Sincerely yours, >Mike.
On Tue, Apr 16, 2024 at 12:55:31PM +0000, Wei Yang wrote: > On Mon, Apr 15, 2024 at 06:19:42PM +0300, Mike Rapoport wrote: > >On Sun, Apr 14, 2024 at 12:45:27AM +0000, Wei Yang wrote: > >> After previous change, we may double the array based on the position of > >> the new range. > >> > >> Let's make sure the 129th memory block would double the size correctly > >> at all possible position. > > > >Rather than rewrite an existing test, just add a new one. > > Ok, will add a new one for this. > > >Besides, it would be more interesting to test additions to > >memblock.reserved and a mix of memblock_add() and memblock_reserve() that > >will require resizing the memblock arrays. > > I don't get this very clearly. Would you mind give more hint? There is memblock_reserve_many_check() that verifies that memblock.reserved is properly resized. I think it's better to add test that adds 129th block at multiple locations to memblock.reserved. > -- > Wei Yang > Help you, Help me
On Wed, Apr 17, 2024 at 08:51:14AM +0300, Mike Rapoport wrote: >On Tue, Apr 16, 2024 at 12:55:31PM +0000, Wei Yang wrote: >> On Mon, Apr 15, 2024 at 06:19:42PM +0300, Mike Rapoport wrote: >> >On Sun, Apr 14, 2024 at 12:45:27AM +0000, Wei Yang wrote: >> >> After previous change, we may double the array based on the position of >> >> the new range. >> >> >> >> Let's make sure the 129th memory block would double the size correctly >> >> at all possible position. >> > >> >Rather than rewrite an existing test, just add a new one. >> >> Ok, will add a new one for this. >> >> >Besides, it would be more interesting to test additions to >> >memblock.reserved and a mix of memblock_add() and memblock_reserve() that >> >will require resizing the memblock arrays. >> >> I don't get this very clearly. Would you mind give more hint? > >There is memblock_reserve_many_check() that verifies that memblock.reserved >is properly resized. I think it's better to add test that adds 129th block >at multiple locations to memblock.reserved. > I write a draft as below, is this what you expect? diff --git a/tools/testing/memblock/tests/basic_api.c b/tools/testing/memblock/tests/basic_api.c index f1569ebb9872..d2b8114921f9 100644 --- a/tools/testing/memblock/tests/basic_api.c +++ b/tools/testing/memblock/tests/basic_api.c @@ -912,84 +912,94 @@ static int memblock_reserve_near_max_check(void) * memblock.memory.max, find a new valid memory as * reserved.regions. */ +/* Keep the gap so these memory region will not be merged. */ +#define MEMORY_BASE(idx) (SZ_128K + (MEM_SIZE * 2) * (idx)) static int memblock_reserve_many_check(void) { - int i; + int i, skip; void *orig_region; struct region r = { .base = SZ_16K, .size = SZ_16K, }; - phys_addr_t memory_base = SZ_128K; phys_addr_t new_reserved_regions_size; PREFIX_PUSH(); - reset_memblock_regions(); - memblock_allow_resize(); + /* Reserve the 129th memory block for all possible positions*/ + for (skip = 0; skip < INIT_MEMBLOCK_REGIONS + 1; skip++) + { + reset_memblock_regions(); + memblock_allow_resize(); - /* Add a valid memory region used by double_array(). */ - dummy_physical_memory_init(); - memblock_add(dummy_physical_memory_base(), MEM_SIZE); + /* Add a valid memory region used by double_array(). */ + dummy_physical_memory_init(); + memblock_add(dummy_physical_memory_base(), MEM_SIZE); - for (i = 0; i < INIT_MEMBLOCK_REGIONS; i++) { - /* Reserve some fakes memory region to fulfill the memblock. */ - memblock_reserve(memory_base, MEM_SIZE); + for (i = 0; i < INIT_MEMBLOCK_REGIONS + 1; i++) { + if (i == skip) + continue; - ASSERT_EQ(memblock.reserved.cnt, i + 1); - ASSERT_EQ(memblock.reserved.total_size, (i + 1) * MEM_SIZE); + /* Reserve some fakes memory region to fulfill the memblock. */ + memblock_reserve(MEMORY_BASE(i), MEM_SIZE); - /* Keep the gap so these memory region will not be merged. */ - memory_base += MEM_SIZE * 2; - } + if (i < skip) { + ASSERT_EQ(memblock.reserved.cnt, i + 1); + ASSERT_EQ(memblock.reserved.total_size, (i + 1) * MEM_SIZE); + } else { + ASSERT_EQ(memblock.reserved.cnt, i); + ASSERT_EQ(memblock.reserved.total_size, i * MEM_SIZE); + } + } - orig_region = memblock.reserved.regions; - - /* This reserve the 129 memory_region, and makes it double array. */ - memblock_reserve(memory_base, MEM_SIZE); - - /* - * This is the memory region size used by the doubled reserved.regions, - * and it has been reserved due to it has been used. The size is used to - * calculate the total_size that the memblock.reserved have now. - */ - new_reserved_regions_size = PAGE_ALIGN((INIT_MEMBLOCK_REGIONS * 2) * - sizeof(struct memblock_region)); - /* - * The double_array() will find a free memory region as the new - * reserved.regions, and the used memory region will be reserved, so - * there will be one more region exist in the reserved memblock. And the - * one more reserved region's size is new_reserved_regions_size. - */ - ASSERT_EQ(memblock.reserved.cnt, INIT_MEMBLOCK_REGIONS + 2); - ASSERT_EQ(memblock.reserved.total_size, (INIT_MEMBLOCK_REGIONS + 1) * MEM_SIZE + - new_reserved_regions_size); - ASSERT_EQ(memblock.reserved.max, INIT_MEMBLOCK_REGIONS * 2); - - /* - * Now memblock_double_array() works fine. Let's check after the - * double_array(), the memblock_reserve() still works as normal. - */ - memblock_reserve(r.base, r.size); - ASSERT_EQ(memblock.reserved.regions[0].base, r.base); - ASSERT_EQ(memblock.reserved.regions[0].size, r.size); - - ASSERT_EQ(memblock.reserved.cnt, INIT_MEMBLOCK_REGIONS + 3); - ASSERT_EQ(memblock.reserved.total_size, (INIT_MEMBLOCK_REGIONS + 1) * MEM_SIZE + - new_reserved_regions_size + - r.size); - ASSERT_EQ(memblock.reserved.max, INIT_MEMBLOCK_REGIONS * 2); - - dummy_physical_memory_cleanup(); - - /* - * The current reserved.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.reserved.regions = orig_region; - memblock.reserved.cnt = INIT_MEMBLOCK_RESERVED_REGIONS; + orig_region = memblock.reserved.regions; + + /* This reserve the 129 memory_region, and makes it double array. */ + memblock_reserve(MEMORY_BASE(skip), MEM_SIZE); + + /* + * This is the memory region size used by the doubled reserved.regions, + * and it has been reserved due to it has been used. The size is used to + * calculate the total_size that the memblock.reserved have now. + */ + new_reserved_regions_size = PAGE_ALIGN((INIT_MEMBLOCK_REGIONS * 2) * + sizeof(struct memblock_region)); + /* + * The double_array() will find a free memory region as the new + * reserved.regions, and the used memory region will be reserved, so + * there will be one more region exist in the reserved memblock. And the + * one more reserved region's size is new_reserved_regions_size. + */ + ASSERT_EQ(memblock.reserved.cnt, INIT_MEMBLOCK_REGIONS + 2); + ASSERT_EQ(memblock.reserved.total_size, (INIT_MEMBLOCK_REGIONS + 1) * MEM_SIZE + + new_reserved_regions_size); + ASSERT_EQ(memblock.reserved.max, INIT_MEMBLOCK_REGIONS * 2); + + /* + * Now memblock_double_array() works fine. Let's check after the + * double_array(), the memblock_reserve() still works as normal. + */ + memblock_reserve(r.base, r.size); + ASSERT_EQ(memblock.reserved.regions[0].base, r.base); + ASSERT_EQ(memblock.reserved.regions[0].size, r.size); + + ASSERT_EQ(memblock.reserved.cnt, INIT_MEMBLOCK_REGIONS + 3); + ASSERT_EQ(memblock.reserved.total_size, (INIT_MEMBLOCK_REGIONS + 1) * MEM_SIZE + + new_reserved_regions_size + + r.size); + ASSERT_EQ(memblock.reserved.max, INIT_MEMBLOCK_REGIONS * 2); + + dummy_physical_memory_cleanup(); + + /* + * The current reserved.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.reserved.regions = orig_region; + memblock.reserved.cnt = INIT_MEMBLOCK_RESERVED_REGIONS; + } test_pass_pop();
On Wed, Apr 17, 2024 at 08:51:14AM +0300, Mike Rapoport wrote: >On Tue, Apr 16, 2024 at 12:55:31PM +0000, Wei Yang wrote: >> On Mon, Apr 15, 2024 at 06:19:42PM +0300, Mike Rapoport wrote: >> >On Sun, Apr 14, 2024 at 12:45:27AM +0000, Wei Yang wrote: >> >> After previous change, we may double the array based on the position of >> >> the new range. >> >> >> >> Let's make sure the 129th memory block would double the size correctly >> >> at all possible position. >> > >> >Rather than rewrite an existing test, just add a new one. >> >> Ok, will add a new one for this. >> >> >Besides, it would be more interesting to test additions to >> >memblock.reserved and a mix of memblock_add() and memblock_reserve() that >> >will require resizing the memblock arrays. >> >> I don't get this very clearly. Would you mind give more hint? > >There is memblock_reserve_many_check() that verifies that memblock.reserved >is properly resized. I think it's better to add test that adds 129th block >at multiple locations to memblock.reserved. > I come up with another version, which could address the bug fixed by commit 48c3b583bbdd ("mm/memblock: fix overlapping allocation when doubling reserved array"). Comment out the fix, the test failed since cnt mismatch after double array. Not sure you prefer to have both or just leave this version by replacing current memblock_reserve_many_check(). diff --git a/tools/testing/memblock/tests/basic_api.c b/tools/testing/memblock/tests/basic_api.c index d2b8114921f9..fb76471108b2 100644 --- a/tools/testing/memblock/tests/basic_api.c +++ b/tools/testing/memblock/tests/basic_api.c @@ -1006,6 +1006,119 @@ static int memblock_reserve_many_check(void) return 0; } +/* Keep the gap so these memory region will not be merged. */ +#define MEMORY_BASE_OFFSET(idx, offset) ((offset) + (MEM_SIZE * 2) * (idx)) +static int memblock_reserve_many_conflict_check(void) +{ + int i, skip; + void *orig_region; + struct region r = { + .base = SZ_16K, + .size = SZ_16K, + }; + phys_addr_t new_reserved_regions_size; + + /* + * 0 1 129 + * +---+ +---+ +---+ + * |32K| |32K| .. |32K| + * +---+ +---+ +---+ + * + * Pre-allocate the range for 129 memory block + one range for double + * memblock.reserved.regions at idx 0. + * See commit 48c3b583bbdd ("mm/memblock: fix overlapping allocation + * when doubling reserved array") + */ + phys_addr_t memory_base = (phys_addr_t)malloc(130 * (2 * SZ_32K)); + phys_addr_t offset = PAGE_ALIGN(memory_base); + + PREFIX_PUSH(); + + /* Reserve the 129th memory block for all possible positions*/ + for (skip = 1; skip <= INIT_MEMBLOCK_REGIONS + 1; skip++) + { + reset_memblock_regions(); + memblock_allow_resize(); + + reset_memblock_attributes(); + /* Add a valid memory region used by double_array(). */ + memblock_add(MEMORY_BASE_OFFSET(0, offset), MEM_SIZE); + /* + * Add a memory region which will be reserved as 129th memory + * region. This is not expected to be used by double_array(). + */ + memblock_add(MEMORY_BASE_OFFSET(skip, offset), MEM_SIZE); + + for (i = 1; i <= INIT_MEMBLOCK_REGIONS + 1; i++) { + if (i == skip) + continue; + + /* Reserve some fakes memory region to fulfill the memblock. */ + memblock_reserve(MEMORY_BASE_OFFSET(i, offset), MEM_SIZE); + + if (i < skip) { + ASSERT_EQ(memblock.reserved.cnt, i); + ASSERT_EQ(memblock.reserved.total_size, i * MEM_SIZE); + } else { + ASSERT_EQ(memblock.reserved.cnt, i - 1); + ASSERT_EQ(memblock.reserved.total_size, (i - 1) * MEM_SIZE); + } + } + + orig_region = memblock.reserved.regions; + + /* This reserve the 129 memory_region, and makes it double array. */ + memblock_reserve(MEMORY_BASE_OFFSET(skip, offset), MEM_SIZE); + + /* + * This is the memory region size used by the doubled reserved.regions, + * and it has been reserved due to it has been used. The size is used to + * calculate the total_size that the memblock.reserved have now. + */ + new_reserved_regions_size = PAGE_ALIGN((INIT_MEMBLOCK_REGIONS * 2) * + sizeof(struct memblock_region)); + /* + * The double_array() will find a free memory region as the new + * reserved.regions, and the used memory region will be reserved, so + * there will be one more region exist in the reserved memblock. And the + * one more reserved region's size is new_reserved_regions_size. + */ + ASSERT_EQ(memblock.reserved.cnt, INIT_MEMBLOCK_REGIONS + 2); + ASSERT_EQ(memblock.reserved.total_size, (INIT_MEMBLOCK_REGIONS + 1) * MEM_SIZE + + new_reserved_regions_size); + ASSERT_EQ(memblock.reserved.max, INIT_MEMBLOCK_REGIONS * 2); + + /* + * Now memblock_double_array() works fine. Let's check after the + * double_array(), the memblock_reserve() still works as normal. + */ + memblock_reserve(r.base, r.size); + ASSERT_EQ(memblock.reserved.regions[0].base, r.base); + ASSERT_EQ(memblock.reserved.regions[0].size, r.size); + + ASSERT_EQ(memblock.reserved.cnt, INIT_MEMBLOCK_REGIONS + 3); + ASSERT_EQ(memblock.reserved.total_size, (INIT_MEMBLOCK_REGIONS + 1) * MEM_SIZE + + new_reserved_regions_size + + r.size); + ASSERT_EQ(memblock.reserved.max, INIT_MEMBLOCK_REGIONS * 2); + + /* + * The current reserved.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.reserved.regions = orig_region; + memblock.reserved.cnt = INIT_MEMBLOCK_RESERVED_REGIONS; + } + + free((void *)memory_base); + + test_pass_pop(); + + return 0; +} + static int memblock_reserve_checks(void) { prefix_reset(); @@ -1021,6 +1134,7 @@ static int memblock_reserve_checks(void) memblock_reserve_between_check(); memblock_reserve_near_max_check(); memblock_reserve_many_check(); + memblock_reserve_many_conflict_check(); prefix_pop();
On Fri, Apr 19, 2024 at 03:15:20AM +0000, Wei Yang wrote: > On Wed, Apr 17, 2024 at 08:51:14AM +0300, Mike Rapoport wrote: > >On Tue, Apr 16, 2024 at 12:55:31PM +0000, Wei Yang wrote: > >> On Mon, Apr 15, 2024 at 06:19:42PM +0300, Mike Rapoport wrote: > >> >On Sun, Apr 14, 2024 at 12:45:27AM +0000, Wei Yang wrote: > >> >> After previous change, we may double the array based on the position of > >> >> the new range. > >> >> > >> >> Let's make sure the 129th memory block would double the size correctly > >> >> at all possible position. > >> > > >> >Rather than rewrite an existing test, just add a new one. > >> > >> Ok, will add a new one for this. > >> > >> >Besides, it would be more interesting to test additions to > >> >memblock.reserved and a mix of memblock_add() and memblock_reserve() that > >> >will require resizing the memblock arrays. > >> > >> I don't get this very clearly. Would you mind give more hint? > > > >There is memblock_reserve_many_check() that verifies that memblock.reserved > >is properly resized. I think it's better to add test that adds 129th block > >at multiple locations to memblock.reserved. > > > > I come up with another version, which could address the bug fixed by commit > 48c3b583bbdd ("mm/memblock: fix overlapping allocation when doubling reserved > array"). > > Comment out the fix, the test failed since cnt mismatch after double array. > > Not sure you prefer to have both or just leave this version by replacing > current memblock_reserve_many_check(). Let's have both of them. > diff --git a/tools/testing/memblock/tests/basic_api.c b/tools/testing/memblock/tests/basic_api.c > index d2b8114921f9..fb76471108b2 100644 > --- a/tools/testing/memblock/tests/basic_api.c > +++ b/tools/testing/memblock/tests/basic_api.c > @@ -1006,6 +1006,119 @@ static int memblock_reserve_many_check(void) > return 0; > } > > +/* Keep the gap so these memory region will not be merged. */ > +#define MEMORY_BASE_OFFSET(idx, offset) ((offset) + (MEM_SIZE * 2) * (idx)) > +static int memblock_reserve_many_conflict_check(void) > +{ > + int i, skip; > + void *orig_region; > + struct region r = { > + .base = SZ_16K, > + .size = SZ_16K, > + }; > + phys_addr_t new_reserved_regions_size; > + > + /* > + * 0 1 129 > + * +---+ +---+ +---+ > + * |32K| |32K| .. |32K| > + * +---+ +---+ +---+ > + * > + * Pre-allocate the range for 129 memory block + one range for double > + * memblock.reserved.regions at idx 0. > + * See commit 48c3b583bbdd ("mm/memblock: fix overlapping allocation > + * when doubling reserved array") > + */ > + phys_addr_t memory_base = (phys_addr_t)malloc(130 * (2 * SZ_32K)); Just increase MEM_SIZE to, say, SZ_1M and use dummy_physical_memory_init() etc > + phys_addr_t offset = PAGE_ALIGN(memory_base); > + > + PREFIX_PUSH(); > + > + /* Reserve the 129th memory block for all possible positions*/ > + for (skip = 1; skip <= INIT_MEMBLOCK_REGIONS + 1; skip++) > + { > + reset_memblock_regions(); > + memblock_allow_resize(); > + > + reset_memblock_attributes(); > + /* Add a valid memory region used by double_array(). */ > + memblock_add(MEMORY_BASE_OFFSET(0, offset), MEM_SIZE); > + /* > + * Add a memory region which will be reserved as 129th memory > + * region. This is not expected to be used by double_array(). > + */ > + memblock_add(MEMORY_BASE_OFFSET(skip, offset), MEM_SIZE); > + > + for (i = 1; i <= INIT_MEMBLOCK_REGIONS + 1; i++) { > + if (i == skip) > + continue; > + > + /* Reserve some fakes memory region to fulfill the memblock. */ > + memblock_reserve(MEMORY_BASE_OFFSET(i, offset), MEM_SIZE); > + > + if (i < skip) { > + ASSERT_EQ(memblock.reserved.cnt, i); > + ASSERT_EQ(memblock.reserved.total_size, i * MEM_SIZE); > + } else { > + ASSERT_EQ(memblock.reserved.cnt, i - 1); > + ASSERT_EQ(memblock.reserved.total_size, (i - 1) * MEM_SIZE); > + } > + } > + > + orig_region = memblock.reserved.regions; > + > + /* This reserve the 129 memory_region, and makes it double array. */ > + memblock_reserve(MEMORY_BASE_OFFSET(skip, offset), MEM_SIZE); > + > + /* > + * This is the memory region size used by the doubled reserved.regions, > + * and it has been reserved due to it has been used. The size is used to > + * calculate the total_size that the memblock.reserved have now. > + */ > + new_reserved_regions_size = PAGE_ALIGN((INIT_MEMBLOCK_REGIONS * 2) * > + sizeof(struct memblock_region)); > + /* > + * The double_array() will find a free memory region as the new > + * reserved.regions, and the used memory region will be reserved, so > + * there will be one more region exist in the reserved memblock. And the > + * one more reserved region's size is new_reserved_regions_size. > + */ > + ASSERT_EQ(memblock.reserved.cnt, INIT_MEMBLOCK_REGIONS + 2); > + ASSERT_EQ(memblock.reserved.total_size, (INIT_MEMBLOCK_REGIONS + 1) * MEM_SIZE + > + new_reserved_regions_size); > + ASSERT_EQ(memblock.reserved.max, INIT_MEMBLOCK_REGIONS * 2); > + > + /* > + * Now memblock_double_array() works fine. Let's check after the > + * double_array(), the memblock_reserve() still works as normal. > + */ > + memblock_reserve(r.base, r.size); > + ASSERT_EQ(memblock.reserved.regions[0].base, r.base); > + ASSERT_EQ(memblock.reserved.regions[0].size, r.size); > + > + ASSERT_EQ(memblock.reserved.cnt, INIT_MEMBLOCK_REGIONS + 3); > + ASSERT_EQ(memblock.reserved.total_size, (INIT_MEMBLOCK_REGIONS + 1) * MEM_SIZE + > + new_reserved_regions_size + > + r.size); > + ASSERT_EQ(memblock.reserved.max, INIT_MEMBLOCK_REGIONS * 2); > + > + /* > + * The current reserved.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.reserved.regions = orig_region; > + memblock.reserved.cnt = INIT_MEMBLOCK_RESERVED_REGIONS; > + } > + > + free((void *)memory_base); > + > + test_pass_pop(); > + > + return 0; > +} > + > static int memblock_reserve_checks(void) > { > prefix_reset(); > @@ -1021,6 +1134,7 @@ static int memblock_reserve_checks(void) > memblock_reserve_between_check(); > memblock_reserve_near_max_check(); > memblock_reserve_many_check(); > + memblock_reserve_many_conflict_check(); > > prefix_pop(); > > -- > 2.34.1
diff --git a/tools/testing/memblock/tests/basic_api.c b/tools/testing/memblock/tests/basic_api.c index f317fe691fc4..f1569ebb9872 100644 --- a/tools/testing/memblock/tests/basic_api.c +++ b/tools/testing/memblock/tests/basic_api.c @@ -431,84 +431,98 @@ static int memblock_add_near_max_check(void) */ static int memblock_add_many_check(void) { - int i; + int i, skip; void *orig_region; struct region r = { .base = SZ_16K, .size = SZ_16K, }; phys_addr_t new_memory_regions_size; - phys_addr_t base, size = SZ_64; + phys_addr_t base, base_start, size = SZ_64; phys_addr_t gap_size = SZ_64; PREFIX_PUSH(); - reset_memblock_regions(); - memblock_allow_resize(); - - dummy_physical_memory_init(); - /* - * We allocated enough memory by using dummy_physical_memory_init(), and - * split it into small block. First we split a large enough memory block - * as the memory region which will be choosed by memblock_double_array(). - */ - base = PAGE_ALIGN(dummy_physical_memory_base()); - new_memory_regions_size = PAGE_ALIGN(INIT_MEMBLOCK_REGIONS * 2 * - sizeof(struct memblock_region)); - memblock_add(base, new_memory_regions_size); - - /* This is the base of small memory block. */ - base += new_memory_regions_size + gap_size; - - orig_region = memblock.memory.regions; + /* Add the 129th memory block for all possible positions*/ + for (skip = 0; skip < INIT_MEMBLOCK_REGIONS; skip++) { + reset_memblock_regions(); + memblock_allow_resize(); - for (i = 0; i < INIT_MEMBLOCK_REGIONS; i++) { + dummy_physical_memory_init(); /* - * Add these small block to fulfill the memblock. We keep a - * gap between the nearby memory to avoid being merged. + * We allocated enough memory by using dummy_physical_memory_init(), and + * split it into small block. First we split a large enough memory block + * as the memory region which will be choosed by memblock_double_array(). */ - memblock_add(base, size); - base += size + gap_size; - - ASSERT_EQ(memblock.memory.cnt, i + 2); + base = PAGE_ALIGN(dummy_physical_memory_base()); + new_memory_regions_size = PAGE_ALIGN(INIT_MEMBLOCK_REGIONS * 2 * + sizeof(struct memblock_region)); + memblock_add(base, new_memory_regions_size); + + /* This is the base of small memory block. */ + base_start = base += new_memory_regions_size + gap_size; + orig_region = memblock.memory.regions; + + for (i = 0; i < INIT_MEMBLOCK_REGIONS; i++, base += size + gap_size) { + if (i == skip) + continue; + /* + * Add these small block to fulfill the memblock. We keep a + * gap between the nearby memory to avoid being merged. + */ + memblock_add(base, size); + + if (i < skip) { + ASSERT_EQ(memblock.memory.cnt, i + 2); + ASSERT_EQ(memblock.memory.total_size, + new_memory_regions_size + (i + 1) * size); + } else { + ASSERT_EQ(memblock.memory.cnt, i + 1); + ASSERT_EQ(memblock.memory.total_size, + new_memory_regions_size + i * size); + } + } + + /* Add the skipped one to trigger memblock_double_array() */ + memblock_add(base_start + skip * (size + gap_size), size); + ASSERT_EQ(memblock.memory.cnt, i + 1); ASSERT_EQ(memblock.memory.total_size, new_memory_regions_size + - (i + 1) * size); - } + i * size); + /* + * At there, memblock_double_array() has been succeed, check if it + * update the memory.max. + */ + ASSERT_EQ(memblock.memory.max, INIT_MEMBLOCK_REGIONS * 2); - /* - * At there, memblock_double_array() has been succeed, check if it - * update the memory.max. - */ - ASSERT_EQ(memblock.memory.max, INIT_MEMBLOCK_REGIONS * 2); + /* memblock_double_array() will reserve the memory it used. Check it. */ + ASSERT_EQ(memblock.reserved.cnt, 1); + ASSERT_EQ(memblock.reserved.total_size, new_memory_regions_size); - /* memblock_double_array() will reserve the memory it used. Check it. */ - ASSERT_EQ(memblock.reserved.cnt, 1); - ASSERT_EQ(memblock.reserved.total_size, new_memory_regions_size); - - /* - * Now memblock_double_array() works fine. Let's check after the - * double_array(), the memblock_add() still works as normal. - */ - memblock_add(r.base, r.size); - ASSERT_EQ(memblock.memory.regions[0].base, r.base); - ASSERT_EQ(memblock.memory.regions[0].size, r.size); + /* + * Now memblock_double_array() works fine. Let's check after the + * double_array(), the memblock_add() still works as normal. + */ + memblock_add(r.base, r.size); + ASSERT_EQ(memblock.memory.regions[0].base, r.base); + ASSERT_EQ(memblock.memory.regions[0].size, r.size); - ASSERT_EQ(memblock.memory.cnt, INIT_MEMBLOCK_REGIONS + 2); - ASSERT_EQ(memblock.memory.total_size, INIT_MEMBLOCK_REGIONS * size + - new_memory_regions_size + - r.size); - ASSERT_EQ(memblock.memory.max, INIT_MEMBLOCK_REGIONS * 2); + ASSERT_EQ(memblock.memory.cnt, INIT_MEMBLOCK_REGIONS + 2); + ASSERT_EQ(memblock.memory.total_size, INIT_MEMBLOCK_REGIONS * size + + new_memory_regions_size + + r.size); + ASSERT_EQ(memblock.memory.max, INIT_MEMBLOCK_REGIONS * 2); - dummy_physical_memory_cleanup(); + 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; + /* + * 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();
After previous change, we may double the array based on the position of the new range. Let's make sure the 129th memory block would double the size correctly at all possible position. Signed-off-by: Wei Yang <richard.weiyang@gmail.com> --- tools/testing/memblock/tests/basic_api.c | 132 +++++++++++++---------- 1 file changed, 73 insertions(+), 59 deletions(-)