Re: [PATCH v2 2/2] memblock tests: cover allocations below the low address limit

[Mike Rapoport]

Hi,

> Add memblock_alloc_low() coverage using the simulator's low address
> limit. Exercise aligned allocation, an allocation whose last byte is
> immediately below the limit, an alignment constraint that prevents
> fitting below it, and fully reserved low memory with high memory still
> available.
> 
> Run each case with bottom-up and top-down allocation. Check zeroing
> and reserved-region accounting as well as returned addresses. Verify
> that an unrestricted allocation can use the free high memory after
> the low allocation fails.
> 
> Document the registered memory range and its simulated limit, clarify
> the allocation boundary, and remove the completed TODO.
> 
> Assisted-by: Codex:GPT-6
> Signed-off-by: Tianyi Chen <hi@xxxxxxxxx>
>
> diff --git a/tools/testing/memblock/Makefile b/tools/testing/memblock/Makefile
> index d80982ccdc209..20524fcbe3c72 100644
> --- a/tools/testing/memblock/Makefile
> +++ b/tools/testing/memblock/Makefile
> @@ -7,7 +7,8 @@ CFLAGS += -I. -I../../include -Wall -O2 -fsanitize=address \
>  LDFLAGS += -fsanitize=address -fsanitize=undefined
>  TARGETS = main
>  TEST_OFILES = tests/alloc_nid_api.o tests/alloc_helpers_api.o tests/alloc_api.o \
> -		  tests/basic_api.o tests/common.o tests/alloc_exact_nid_api.o
> +		  tests/basic_api.o tests/common.o tests/alloc_exact_nid_api.o \
> +		  tests/alloc_low_api.o
>  DEP_OFILES = memblock.o lib/slab.o mmzone.o slab.o cmdline.o
>  OFILES = main.o $(DEP_OFILES) $(TEST_OFILES)
>  EXTR_SRC = ../../../mm/memblock.c
> diff --git a/tools/testing/memblock/README b/tools/testing/memblock/README
> index b435f48d8a700..13b7f58b2eed1 100644
> --- a/tools/testing/memblock/README
> +++ b/tools/testing/memblock/README
> @@ -67,13 +67,13 @@ memblock
>  |-- tests
>  |    |-- alloc_api.(c|h)         -- memblock_alloc tests
>  |    |-- alloc_helpers_api.(c|h) -- memblock_alloc_from tests
> +|    |-- alloc_low_api.(c|h)     -- memblock_alloc_low tests
>  |    |-- alloc_nid_api.(c|h)     -- memblock_alloc_try_nid tests
>  |    |-- basic_api.(c|h)         -- memblock_add/memblock_reserve/... tests
>  |    |-- common.(c|h)            -- helper functions for resetting memblock;
>  |-- main.c        --------------.   dummy physical memory definition
>  |-- Makefile                     `- test runner
>  |-- README
> -|-- TODO
>  |-- .gitignore
>  
>  Simulating physical memory
> @@ -101,12 +101,12 @@ There's no need to explicitly free the dummy memory from memblock via
>  memblock_free() call. The entry will be erased by reset_memblock_regions(),
>  called at the beginning of each test.
>  
> -Known issues
> -============
> -
> -1. Tests for memblock_alloc_low() can't be easily implemented. The function uses
> -   ARCH_LOW_ADDRESS_LIMIT marco, which can't be changed to point at the low
> -   memory of the memory_block.
> +The simulator defines ARCH_LOW_ADDRESS_LIMIT in asm/dma.h using the midpoint
> +of the MEM_SIZE range registered by setup_memblock(), rather than the larger
> +PHYS_MEM_SIZE backing buffer. This leaves registered memory both below and

"rather than ..." part is excessive

> +above the limit, even when malloc() returns an address above the kernel's

Why do we care about what malloc() returns here?

> +default limit. The limit is the first address an allocation must not use;
> +an allocation's base plus its size may equal the limit.
>  
>  References
>  ==========
> diff --git a/tools/testing/memblock/TODO b/tools/testing/memblock/TODO
> deleted file mode 100644
> index c13ad0dae7763..0000000000000
> --- a/tools/testing/memblock/TODO
> +++ /dev/null
> @@ -1,5 +0,0 @@
> -TODO
> -=====
> -
> -1. Add tests for memblock_alloc_low() once the simulator can model
> -   ARCH_LOW_ADDRESS_LIMIT against the low memory in memory_block
> diff --git a/tools/testing/memblock/main.c b/tools/testing/memblock/main.c
> index 278f9dec50087..9a96e178551b8 100644
> --- a/tools/testing/memblock/main.c
> +++ b/tools/testing/memblock/main.c
> @@ -4,6 +4,7 @@
>  #include "tests/alloc_helpers_api.h"
>  #include "tests/alloc_nid_api.h"
>  #include "tests/alloc_exact_nid_api.h"
> +#include "tests/alloc_low_api.h"
>  #include "tests/common.h"
>  
>  int main(int argc, char **argv)
> @@ -12,6 +13,7 @@ int main(int argc, char **argv)
>  	memblock_basic_checks();
>  	memblock_alloc_checks();
>  	memblock_alloc_helpers_checks();
> +	memblock_alloc_low_checks();
>  	memblock_alloc_nid_checks();
>  	memblock_alloc_exact_nid_checks();
>  
> diff --git a/tools/testing/memblock/tests/alloc_low_api.c b/tools/testing/memblock/tests/alloc_low_api.c
> new file mode 100644
> index 0000000000000..1ea5a36fcf0d8
> --- /dev/null
> +++ b/tools/testing/memblock/tests/alloc_low_api.c
> @@ -0,0 +1,148 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +#include "alloc_low_api.h"
> +#include <linux/align.h>
> +
> +/* Allocate at the first or last aligned address below the low limit. */
> +static int alloc_low_simple_check(void)
> +{
> +	struct memblock_region *rgn = &memblock.reserved.regions[0];
> +	phys_addr_t size = SZ_64;
> +	phys_addr_t expected;
> +	void *allocated_ptr;
> +
> +	PREFIX_PUSH();
> +	setup_memblock();
> +
> +	/* malloc() does not guarantee SMP_CACHE_BYTES alignment. */

I'd drop malloc() and say "simulated physical RAM isn't necessarily
SMP_CACHE_BYTES aligned"

> +	if (memblock_bottom_up())
> +		expected = ALIGN(memblock_start_of_DRAM(), SMP_CACHE_BYTES);
> +	else
> +		expected = ALIGN_DOWN(ARCH_LOW_ADDRESS_LIMIT - size,
> +				      SMP_CACHE_BYTES);
> +
> +	allocated_ptr = memblock_alloc_low(size, SMP_CACHE_BYTES);
> +
> +	ASSERT_NE(allocated_ptr, NULL);
> +	ASSERT_EQ((phys_addr_t)(uintptr_t)allocated_ptr, expected);

Isn't (phys_addr_t) cast enough?

-- 
Sincerely yours,
Mike.