Re: [PATCH 1/2] x86/numa: Fix the address overlap check in numa_fill_memblks()

[Mike Rapoport <rppt@kernel.org>]

On Fri, Jan 12, 2024 at 12:09:50PM -0800, alison.schofield@intel.com wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> numa_fill_memblks() fills in the gaps in numa_meminfo memblks over a
> physical address range. To do so, it first creates a list of existing
> memblks that overlap that address range. The issue is that it is off
> by one when comparing to the end of the address range, so memblks
> that do not overlap are selected.
> 
> The impact of selecting a memblk that does not actually overlap is
> that an existing memblk may be filled when the expected action is to
> do nothing and return NUMA_NO_MEMBLK to the caller. The caller can
> then add a new NUMA node and memblk.
> 
> Replace the broken open-coded search for address overlap with the
> memblock helper memblock_addrs_overlap(). Update the kernel doc
> and in code comments.
> 
> Fixes: 8f012db27c95 ("x86/numa: Introduce numa_fill_memblks()")
> Suggested by: "Huang, Ying" <ying.huang@intel.com>
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> ---
> 
> Changes since single patch:
> - Use memblock_addrs_overlap() for address comparison (Dan)
> - Update kernel doc and in code comments
> - New commit message
> - Update commit log
> - Link to single patch:
> https://lore.kernel.org/linux-cxl/20240102213206.1493733-1-alison.schofield@intel.com/
> 
>  arch/x86/mm/numa.c       | 19 +++++++------------
>  include/linux/memblock.h |  2 ++
>  mm/memblock.c            |  5 +++--

For memblock changes
Acked-by: Mike Rapoport (IBM) <rppt@kernel.org>

>  3 files changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
> index adc497b93f03..8ada9bbfad58 100644
> --- a/arch/x86/mm/numa.c
> +++ b/arch/x86/mm/numa.c
> @@ -944,14 +944,12 @@ static struct numa_memblk *numa_memblk_list[NR_NODE_MEMBLKS] __initdata;
>   * @start: address to begin fill
>   * @end: address to end fill
>   *
> - * Find and extend numa_meminfo memblks to cover the @start-@end
> - * physical address range, such that the first memblk includes
> - * @start, the last memblk includes @end, and any gaps in between
> - * are filled.
> + * Find and extend numa_meminfo memblks to cover the physical
> + * address range @start-@end
>   *
>   * RETURNS:
>   * 0		  : Success
> - * NUMA_NO_MEMBLK : No memblk exists in @start-@end range
> + * NUMA_NO_MEMBLK : No memblks exist in address range @start-@end
>   */
>  
>  int __init numa_fill_memblks(u64 start, u64 end)
> @@ -963,17 +961,14 @@ int __init numa_fill_memblks(u64 start, u64 end)
>  
>  	/*
>  	 * Create a list of pointers to numa_meminfo memblks that
> -	 * overlap start, end. Exclude (start == bi->end) since
> -	 * end addresses in both a CFMWS range and a memblk range
> -	 * are exclusive.
> -	 *
> -	 * This list of pointers is used to make in-place changes
> -	 * that fill out the numa_meminfo memblks.
> +	 * overlap start, end. The list is used to make in-place
> +	 * changes that fill out the numa_meminfo memblks.
>  	 */
>  	for (int i = 0; i < mi->nr_blks; i++) {
>  		struct numa_memblk *bi = &mi->blk[i];
>  
> -		if (start < bi->end && end >= bi->start) {
> +		if (memblock_addrs_overlap(start, end - start, bi->start,
> +					   bi->end - bi->start)) {
>  			blk[count] = &mi->blk[i];
>  			count++;
>  		}
> diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> index b695f9e946da..e2082240586d 100644
> --- a/include/linux/memblock.h
> +++ b/include/linux/memblock.h
> @@ -121,6 +121,8 @@ int memblock_reserve(phys_addr_t base, phys_addr_t size);
>  int memblock_physmem_add(phys_addr_t base, phys_addr_t size);
>  #endif
>  void memblock_trim_memory(phys_addr_t align);
> +unsigned long memblock_addrs_overlap(phys_addr_t base1, phys_addr_t size1,
> +				     phys_addr_t base2, phys_addr_t size2);
>  bool memblock_overlaps_region(struct memblock_type *type,
>  			      phys_addr_t base, phys_addr_t size);
>  bool memblock_validate_numa_coverage(unsigned long threshold_bytes);
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 8c194d8afeec..d0cadeeecfe8 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -180,8 +180,9 @@ static inline phys_addr_t memblock_cap_size(phys_addr_t base, phys_addr_t *size)
>  /*
>   * Address comparison utilities
>   */
> -static unsigned long __init_memblock memblock_addrs_overlap(phys_addr_t base1, phys_addr_t size1,
> -				       phys_addr_t base2, phys_addr_t size2)
> +unsigned long __init_memblock
> +memblock_addrs_overlap(phys_addr_t base1, phys_addr_t size1, phys_addr_t base2,
> +		       phys_addr_t size2)
>  {
>  	return ((base1 < (base2 + size2)) && (base2 < (base1 + size1)));
>  }
> -- 
> 2.37.3
> 

-- 
Sincerely yours,
Mike.