Re: [PATCH 3/3] memblock: Avoid useless checks in memblock_merge_regions().

[Mike Rapoport]

On Fri, Jan 13, 2023 at 04:26:59PM +0800, Peng Zhang wrote:
> memblock_merge_regions() is called after regions have been modified to
> merge the neighboring compatible regions. That will check all regions
> but most checks is useless.
> 
> Most of the time we only insert one or a few new regions, or modify one
> or a few regions. At this time, we don't need to check all regions. We
> only need to check the changed regions, because other not related
> regions cannot be merged. So this patch add two parameters to
> memblock_merge_regions() to indicate the lower and upper boundary to scan.
> 
> Signed-off-by: Peng Zhang <zhangpeng.00@xxxxxxxxxxxxx>
> ---
>  mm/memblock.c | 36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/mm/memblock.c b/mm/memblock.c
> index cb92770ac22e..e19eb08efc73 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -523,15 +523,18 @@ static int __init_memblock memblock_double_array(struct memblock_type *type,
>  /**
>   * memblock_merge_regions - merge neighboring compatible regions
>   * @type: memblock type to scan
> - *
> - * Scan @type and merge neighboring compatible regions.
> + * @start_rgn: start scanning from (@start_rgn - 1)
> + * @end_rgn: end scanning at (@end_rgn - 1)
> + * Scan @type and merge neighboring compatible regions in [@start_rgn - 1, @end_rgn)
>   */
> -static void __init_memblock memblock_merge_regions(struct memblock_type *type)
> +static void __init_memblock memblock_merge_regions(struct memblock_type *type,
> +						   int start_rgn,
> +						   int end_rgn)

Make start_rgn and end_rgn unsigned longs and ...

>  {
> -	int i = 0;
> +	int i = max(start_rgn - 1, 0);
  
... open code max() as 

	if (start_rgn)
		i = start_rgn;

> -	/* cnt never goes below 1 */
> -	while (i < type->cnt - 1) {
> +	end_rgn = min(end_rgn, (int)type->cnt - 1);

... and drop the casting here.

> +	while (i < end_rgn) {
>  		struct memblock_region *this = &type->regions[i];
>  		struct memblock_region *next = &type->regions[i + 1];
>  
> @@ -548,6 +551,7 @@ static void __init_memblock memblock_merge_regions(struct memblock_type *type)
>  		/* move forward from next + 1, index of which is i + 2 */
>  		memmove(next, next + 1, (type->cnt - (i + 2)) * sizeof(*next));
>  		type->cnt--;
> +		end_rgn--;
>  	}
>  }
>  
> @@ -604,7 +608,7 @@ static int __init_memblock memblock_add_range(struct memblock_type *type,
>  	bool insert = false;
>  	phys_addr_t obase = base;
>  	phys_addr_t end = base + memblock_cap_size(base, &size);
> -	int idx, start_idx, nr_new;
> +	int idx, start_idx, nr_new, start_rgn = -1, end_rgn;
>  	struct memblock_region *rgn;
>  
>  	if (!size)
> @@ -659,10 +663,14 @@ static int __init_memblock memblock_add_range(struct memblock_type *type,
>  #endif
>  			WARN_ON(flags != rgn->flags);
>  			nr_new++;
> -			if (insert)
> +			if (insert) {
> +				if (start_rgn == -1)

you can initialize start_rgn with 0 and use if(!start_rgn) here.

> +					start_rgn = idx;
> +				end_rgn = idx + 1;
>  				memblock_insert_region(type, idx++, base,
>  						       rbase - base, nid,
>  						       flags);
> +			}
>  		}
>  		/* area below @rend is dealt with, forget about it */
>  		base = min(rend, end);
> @@ -671,9 +679,13 @@ static int __init_memblock memblock_add_range(struct memblock_type *type,
>  	/* insert the remaining portion */
>  	if (base < end) {
>  		nr_new++;
> -		if (insert)
> +		if (insert) {
> +			if (start_rgn == -1)

Ditto.

> +				start_rgn = idx;
> +			end_rgn = idx + 1;
>  			memblock_insert_region(type, idx, base, end - base,
>  					       nid, flags);
> +		}
>  	}
>  
>  	if (!nr_new)
> @@ -690,7 +702,7 @@ static int __init_memblock memblock_add_range(struct memblock_type *type,
>  		insert = true;
>  		goto repeat;
>  	} else {
> -		memblock_merge_regions(type);
> +		memblock_merge_regions(type, start_rgn, end_rgn);
>  		return 0;
>  	}
>  }
> @@ -927,7 +939,7 @@ static int __init_memblock memblock_setclr_flag(phys_addr_t base,
>  			r->flags &= ~flag;
>  	}
>  
> -	memblock_merge_regions(type);
> +	memblock_merge_regions(type, start_rgn, end_rgn);
>  	return 0;
>  }
>  
> @@ -1300,7 +1312,7 @@ int __init_memblock memblock_set_node(phys_addr_t base, phys_addr_t size,
>  	for (i = start_rgn; i < end_rgn; i++)
>  		memblock_set_region_node(&type->regions[i], nid);
>  
> -	memblock_merge_regions(type);
> +	memblock_merge_regions(type, start_rgn, end_rgn);
>  #endif
>  	return 0;
>  }
> -- 
> 2.20.1
> 

-- 
Sincerely yours,
Mike.