Message-ID: <2ee31efb-a07f-6132-88fd-d69ad180cf33@bytedance.com>
Date:   Sun, 29 Jan 2023 17:14:19 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.6.1
Subject: Re: [PATCH v2 2/2] memblock: Avoid useless checks in
 memblock_merge_regions().
To:     rppt@kernel.org
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org
References: <20230129090034.12310-1-zhangpeng.00@bytedance.com>
 <20230129090034.12310-3-zhangpeng.00@bytedance.com>
From:   Peng Zhang <zhangpeng.00@bytedance.com>
In-Reply-To: <20230129090034.12310-3-zhangpeng.00@bytedance.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi, Mike

> @@ -581,7 +586,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, nr_new;
> +	int idx, nr_new, start_rgn = -1, end_rgn;
>   	struct memblock_region *rgn;
>   
>   	if (!size)
> @@ -635,10 +640,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)
We can't initialize start_rgn with 0 and use if(!start_rgn).

If start_rgn is equal to 0, it will be reassigned. It is possible that 
we will eventually need start_rgn to be equal to 0, and it may be 
reassigned to a larger value that does not meet expectations.

Thanks.

> +					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);
> @@ -647,9 +656,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)
> +				start_rgn = idx;
> +			end_rgn = idx + 1;
>   			memblock_insert_region(type, idx, base, end - base,
>   					       nid, flags);
> +		}
>   	}
>   
>   	if (!nr_new)
> @@ -666,7 +679,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;
>   	}
>   }
> @@ -902,7 +915,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;
>   }
>   
> @@ -1275,7 +1288,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;
>   }
Sincerely yours,
Peng.