Re: [PATCH] x86/numa: Make numa_fill_memblks() @end parameter exclusive

[Alison Schofield <alison.schofield@intel.com>]

On Tue, Jan 02, 2024 at 03:42:50PM -0800, Dan Williams wrote:
> 
> So I realize I asked for the minimal fix before a wider cleanup to
> 'struct numa_memblk' to use 'struct range', but I want to see some of
> that cleanup now. How about the below (only compile-tested) to at least
> convert numa_fill_memblks(), since it is new, and then circle back
> sometime later to move 'struct numa_memblk' to embed a 'struct range'
> directly? I.e. save touching legacy code for later, but fix the bug in
> the new code with some modernization. This also documents that
> numa_add_memblk() expects an inclusive argument.
> 
> Note that this would be 3 patches, the minimal logic fix as you have it
> without the comment changes since they become moot later, the
> introduction of range_overlaps() with namespace conflict fixup with the
> btrfs version, and using range_overlaps() to cleanup numa_fill_memblks()

Hi Dan,

Your idea to use struct range in struct numa_memblk comes as tremendous
relief after staring at open coded usage of start/end in numa_memblk.

With that acknowledged, the minimal fix to the overlap issue could be
either in x86 or acpi. We're at x86 because we wanted to be 'like'
numa_add_memblks(). A partial cleanup now makes numa_fill_memblks()
diverge further. And, to avoid the partial cleanup from being
throw-away work, it seems we'd want to define the full cleanup ahead
of time.

Also recall that we have a bigger hope of replacing this whole
numa memblk usage with the "Memblock" (with an 'o') API.

Maybe I need clarification on what you are suggesting wrt 3 patches?
Are you saying you would not merge the minimal fix unless the partial
clean up patches are included in the same set?

Also, is this something that has to be merged through x86 or might
you be able to take it though cxl tree? Yes, that's me worrying 
about the review and intake burden this puts on x86.

Alison

> 
> diff --git a/arch/x86/include/asm/sparsemem.h b/arch/x86/include/asm/sparsemem.h
> index 1be13b2dfe8b..9e2279762eaa 100644
> --- a/arch/x86/include/asm/sparsemem.h
> +++ b/arch/x86/include/asm/sparsemem.h
> @@ -37,7 +37,8 @@ extern int phys_to_target_node(phys_addr_t start);
>  #define phys_to_target_node phys_to_target_node
>  extern int memory_add_physaddr_to_nid(u64 start);
>  #define memory_add_physaddr_to_nid memory_add_physaddr_to_nid
> -extern int numa_fill_memblks(u64 start, u64 end);
> +struct range;
> +extern int numa_fill_memblks(struct range *range);
>  #define numa_fill_memblks numa_fill_memblks
>  #endif
>  #endif /* __ASSEMBLY__ */
> diff --git a/arch/x86/mm/numa.c b/arch/x86/mm/numa.c
> index b29ceb19e46e..ce1ccda6fee4 100644
> --- a/arch/x86/mm/numa.c
> +++ b/arch/x86/mm/numa.c
> @@ -13,6 +13,7 @@
>  #include <linux/sched.h>
>  #include <linux/topology.h>
>  #include <linux/sort.h>
> +#include <linux/range.h>
>  
>  #include <asm/e820/api.h>
>  #include <asm/proto.h>
> @@ -971,20 +972,17 @@ static struct numa_memblk *numa_memblk_list[NR_NODE_MEMBLKS] __initdata;
>  
>  /**
>   * numa_fill_memblks - Fill gaps in numa_meminfo memblks
> - * @start: address to begin fill
> - * @end: address to end fill
> + * @range: range to 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 and fill any gaps.
>   *
>   * RETURNS:
>   * 0		  : Success
> - * NUMA_NO_MEMBLK : No memblk exists in @start-@end range
> + * NUMA_NO_MEMBLK : No memblk exists in @range
>   */
>  
> -int __init numa_fill_memblks(u64 start, u64 end)
> +int __init numa_fill_memblks(struct range *range)
>  {
>  	struct numa_memblk **blk = &numa_memblk_list[0];
>  	struct numa_meminfo *mi = &numa_meminfo;
> @@ -993,17 +991,17 @@ 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. This 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];
> +		struct range bi_range = {
> +			.start = bi->start,
> +			.end = bi->end - 1,
> +		};
>  
> -		if (start < bi->end && end >= bi->start) {
> +		if (range_overlaps(range, &bi_range)) {
>  			blk[count] = &mi->blk[i];
>  			count++;
>  		}
> @@ -1015,8 +1013,8 @@ int __init numa_fill_memblks(u64 start, u64 end)
>  	sort(&blk[0], count, sizeof(blk[0]), cmp_memblk, NULL);
>  
>  	/* Make sure the first/last memblks include start/end */
> -	blk[0]->start = min(blk[0]->start, start);
> -	blk[count - 1]->end = max(blk[count - 1]->end, end);
> +	blk[0]->start = min(blk[0]->start, range->start);
> +	blk[count - 1]->end = max(blk[count - 1]->end, range->end + 1);
>  
>  	/*
>  	 * Fill any gaps by tracking the previous memblks
> diff --git a/drivers/acpi/numa/srat.c b/drivers/acpi/numa/srat.c
> index 12f330b0eac0..6213a15c2a8d 100644
> --- a/drivers/acpi/numa/srat.c
> +++ b/drivers/acpi/numa/srat.c
> @@ -307,8 +307,10 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
>  	int node;
>  
>  	cfmws = (struct acpi_cedt_cfmws *)header;
> -	start = cfmws->base_hpa;
> -	end = cfmws->base_hpa + cfmws->window_size;
> +	struct range range = {
> +		.start = cfmws->base_hpa,
> +		.end = cfmws->base_hpa + cfmws->window_size - 1,
> +	};
>  
>  	/*
>  	 * The SRAT may have already described NUMA details for all,
> @@ -316,7 +318,7 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
>  	 * found for any portion of the window to cover the entire
>  	 * window.
>  	 */
> -	if (!numa_fill_memblks(start, end))
> +	if (!numa_fill_memblks(&range))
>  		return 0;
>  
>  	/* No SRAT description. Create a new node. */
> @@ -327,7 +329,7 @@ static int __init acpi_parse_cfmws(union acpi_subtable_headers *header,
>  		return -EINVAL;
>  	}
>  
> -	if (numa_add_memblk(node, start, end) < 0) {
> +	if (numa_add_memblk(node, range.start, range.end + 1) < 0) {
>  		/* CXL driver must handle the NUMA_NO_NODE case */
>  		pr_warn("ACPI NUMA: Failed to add memblk for CFMWS node %d [mem %#llx-%#llx]\n",
>  			node, start, end);
> diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
> index a82e1417c4d2..e6c4ffc6003d 100644
> --- a/fs/btrfs/ordered-data.c
> +++ b/fs/btrfs/ordered-data.c
> @@ -111,7 +111,7 @@ static struct rb_node *__tree_search(struct rb_root *root, u64 file_offset,
>  	return NULL;
>  }
>  
> -static int range_overlaps(struct btrfs_ordered_extent *entry, u64 file_offset,
> +static int btrfs_range_overlaps(struct btrfs_ordered_extent *entry, u64 file_offset,
>  			  u64 len)
>  {
>  	if (file_offset + len <= entry->file_offset ||
> @@ -913,7 +913,7 @@ struct btrfs_ordered_extent *btrfs_lookup_ordered_range(
>  
>  	while (1) {
>  		entry = rb_entry(node, struct btrfs_ordered_extent, rb_node);
> -		if (range_overlaps(entry, file_offset, len))
> +		if (btrfs_range_overlaps(entry, file_offset, len))
>  			break;
>  
>  		if (entry->file_offset >= file_offset + len) {
> @@ -1042,12 +1042,12 @@ struct btrfs_ordered_extent *btrfs_lookup_first_ordered_range(
>  	}
>  	if (prev) {
>  		entry = rb_entry(prev, struct btrfs_ordered_extent, rb_node);
> -		if (range_overlaps(entry, file_offset, len))
> +		if (btrfs_range_overlaps(entry, file_offset, len))
>  			goto out;
>  	}
>  	if (next) {
>  		entry = rb_entry(next, struct btrfs_ordered_extent, rb_node);
> -		if (range_overlaps(entry, file_offset, len))
> +		if (btrfs_range_overlaps(entry, file_offset, len))
>  			goto out;
>  	}
>  	/* No ordered extent in the range */
> diff --git a/include/linux/numa.h b/include/linux/numa.h
> index a904861de800..ef35c974e5f2 100644
> --- a/include/linux/numa.h
> +++ b/include/linux/numa.h
> @@ -45,7 +45,7 @@ static inline int phys_to_target_node(u64 start)
>  }
>  #endif
>  #ifndef numa_fill_memblks
> -static inline int __init numa_fill_memblks(u64 start, u64 end)
> +static inline int __init numa_fill_memblks(struct range *range)
>  {
>  	return NUMA_NO_MEMBLK;
>  }
> diff --git a/include/linux/range.h b/include/linux/range.h
> index 6ad0b73cb7ad..981fd4f7731e 100644
> --- a/include/linux/range.h
> +++ b/include/linux/range.h
> @@ -13,11 +13,18 @@ static inline u64 range_len(const struct range *range)
>  	return range->end - range->start + 1;
>  }
>  
> +/* True if r1 completely contains r2 */
>  static inline bool range_contains(struct range *r1, struct range *r2)
>  {
>  	return r1->start <= r2->start && r1->end >= r2->end;
>  }
>  
> +/* True if any part of r1 overlaps r2 */
> +static inline bool range_overlaps(const struct range *r1, const struct range *r2)
> +{
> +       return r1->start <= r2->end && r1->end >= r2->start;
> +}
> +
>  int add_range(struct range *range, int az, int nr_range,
>  		u64 start, u64 end);
>