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

[Wei Yang]

Hi, Michael

I copied your reply here:

>[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?
>

There is no problem in this code. I just find a unnecessary calculation and
remove it in this patch.

>> 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

I don't see the situation you pointed here.

In add_memory_block(), section_nr is used to record the first section which is
present. And this variable is used to calculate the section which is passed to
init_memory_block().

In init_memory_block(), the section got from add_memory_block(), is used to
calculate scn_nr, but finally transformed to "start_section_nr". That means in
init_memory_block(), we just need the "start_section_nr" of a memory_block. We
don't care about who is the first present section.

>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.

There maybe other issues in memory_block, while for the code refine in this
patch, the change is straight and not see side effects.

The field memory_block->start_section_nr records the section number of the
first section in memory_block. No semantic change here and comply with the
high level view of memory_block hierarchy.

>
>Thanks
>

On Wed, Jun 14, 2017 at 01:45:50PM +0800, Wei Yang wrote:
>Based on Greg's comment, cc it to mm list.
>The original thread could be found https://lkml.org/lkml/2017/6/7/202
>
>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@xxxxxxxxx>
>---
> 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

-- 
Wei Yang
Help you, Help me

Attachment: signature.asc
Description: PGP signature