Date: Tue, 13 Jun 2017 13:48:42 +0200
From: Michal Hocko <mhocko@kernel.org>
To: Wei Yang <richard.weiyang@gmail.com>
Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] base/memory: pass the base_section in add_memory_block
Message-ID: <20170613114842.GI10819@dhcp22.suse.cz>
References: <20170607085212.9765-1-richard.weiyang@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170607085212.9765-1-richard.weiyang@gmail.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2117
Lines: 63

[Sorry for a late response]

On Wed 07-06-17 16:52:12, Wei Yang wrote:
> The second parameter of init_memory_block() is used to calculate the
> start_section_nr of this block, which means any section in the same block
> would get the same start_section_nr.

Could you be more specific what is the problem here?

> This patch passes the base_section to init_memory_block(), so that to
> reduce a local variable and a check in every loop.

But then you are not handling a memblock which starts with a !present
section. The code is quite hairy but I do not see why your change is any
more correct. This needs much better justification than what the above
gives us. Maybe the whole thing about incomplete memblock is just
overengineered piece of code, who knows this area is full of stuff that
makes only little sense but again the changelog should be pretty verbose
about all the consequences and focus on the high level rather than
particular issues here and there.

Thanks

> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> ---
>  drivers/base/memory.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index cc4f1d0cbffe..1e903aba2aa1 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -664,21 +664,20 @@ static int init_memory_block(struct memory_block **memory,
>  static int add_memory_block(int base_section_nr)
>  {
>  	struct memory_block *mem;
> -	int i, ret, section_count = 0, section_nr;
> +	int i, ret, section_count = 0;
>  
>  	for (i = base_section_nr;
>  	     (i < base_section_nr + sections_per_block) && i < NR_MEM_SECTIONS;
>  	     i++) {
>  		if (!present_section_nr(i))
>  			continue;
> -		if (section_count == 0)
> -			section_nr = i;
>  		section_count++;
>  	}
>  
>  	if (section_count == 0)
>  		return 0;
> -	ret = init_memory_block(&mem, __nr_to_section(section_nr), MEM_ONLINE);
> +	ret = init_memory_block(&mem, __nr_to_section(base_section_nr),
> +				MEM_ONLINE);
>  	if (ret)
>  		return ret;
>  	mem->section_count = section_count;
> -- 
> 2.11.0

-- 
Michal Hocko
SUSE Labs