[PATCH] base/memory: pass the base_section in add_memory_block

[PATCH] base/memory: pass the base_section in add_memory_block

[Wei Yang]

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.

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

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

Re: [PATCH] base/memory: pass the base_section in add_memory_block

[Michal Hocko]

[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

Re: [PATCH] base/memory: pass the base_section in add_memory_block

[Greg KH]

On Wed, Jun 07, 2017 at 04:52:12PM +0800, 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.
> 
> This patch passes the base_section to init_memory_block(), so that to
> reduce a local variable and a check in every loop.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> ---
>  drivers/base/memory.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)

Can you resend this and cc: linux-mm?  I think the developers there need
to review this.

thanks,

greg k-h