Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1665436 > unrolled thread

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

Started byWei Yang <richard.weiyang@gmail.com>
First post2017-06-14 07:50 +0200
Last post2017-06-14 08:40 +0200
Articles 6 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [RESEND PATCH] base/memory: pass the base_section in add_memory_block Wei Yang <richard.weiyang@gmail.com> - 2017-06-14 07:50 +0200
    Re: [RESEND PATCH] base/memory: pass the base_section in  add_memory_block Michal Hocko <mhocko@kernel.org> - 2017-06-14 08:00 +0200
      Re: [RESEND PATCH] base/memory: pass the base_section in  add_memory_block Michal Hocko <mhocko@kernel.org> - 2017-06-14 08:30 +0200
      Re: [RESEND PATCH] base/memory: pass the base_section in  add_memory_block Wei Yang <richard.weiyang@gmail.com> - 2017-06-14 08:30 +0200
    Re: [RESEND PATCH] base/memory: pass the base_section in  add_memory_block Wei Yang <richard.weiyang@gmail.com> - 2017-06-14 08:10 +0200
      Re: [RESEND PATCH] base/memory: pass the base_section in  add_memory_block Michal Hocko <mhocko@kernel.org> - 2017-06-14 08:40 +0200

#1665436 — [RESEND PATCH] base/memory: pass the base_section in add_memory_block

FromWei Yang <richard.weiyang@gmail.com>
Date2017-06-14 07:50 +0200
Subject[RESEND PATCH] base/memory: pass the base_section in add_memory_block
Message-ID<tS9lL-1ah-1@gated-at.bofh.it>
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@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

[toc] | [next] | [standalone]

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

FromMichal Hocko <mhocko@kernel.org>
Date2017-06-14 08:00 +0200
SubjectRe: [RESEND PATCH] base/memory: pass the base_section in add_memory_block
Message-ID<tS9vw-1dT-55@gated-at.bofh.it>
In reply to#1665436
On Wed 14-06-17 13:45:50, 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

I have already given you feedback
http://lkml.kernel.org/r/20170613114842.GI10819@dhcp22.suse.cz
and you seemed to ignore it completely.

> 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

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

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

FromMichal Hocko <mhocko@kernel.org>
Date2017-06-14 08:30 +0200
SubjectRe: [RESEND PATCH] base/memory: pass the base_section in add_memory_block
Message-ID<tS9Yt-1Cw-1@gated-at.bofh.it>
In reply to#1665465
On Wed 14-06-17 14:19:59, Wei Yang wrote:
> On Wed, Jun 14, 2017 at 07:59:25AM +0200, Michal Hocko wrote:
> >On Wed 14-06-17 13:45:50, 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
> >
> 
> Wow, you are still working~ I just moved your response in this thread~
> 
> So that other audience would be convenient to see the whole story.

You could add linux-mm to the cc in the response to that email
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

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

FromWei Yang <richard.weiyang@gmail.com>
Date2017-06-14 08:30 +0200
SubjectRe: [RESEND PATCH] base/memory: pass the base_section in add_memory_block
Message-ID<tS9Yt-1Cw-3@gated-at.bofh.it>
In reply to#1665465

[Multipart message — attachments visible in raw view] — view raw

On Wed, Jun 14, 2017 at 07:59:25AM +0200, Michal Hocko wrote:
>On Wed 14-06-17 13:45:50, 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
>

Wow, you are still working~ I just moved your response in this thread~

So that other audience would be convenient to see the whole story.

>I have already given you feedback
>http://lkml.kernel.org/r/20170613114842.GI10819@dhcp22.suse.cz
>and you seemed to ignore it completely.
>
>> 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
>
>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me

[toc] | [prev] | [next] | [standalone]

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

FromWei Yang <richard.weiyang@gmail.com>
Date2017-06-14 08:10 +0200
SubjectRe: [RESEND PATCH] base/memory: pass the base_section in add_memory_block
Message-ID<tS9F7-1we-1@gated-at.bofh.it>
In reply to#1665436

[Multipart message — attachments visible in raw view] — view raw

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

-- 
Wei Yang
Help you, Help me

[toc] | [prev] | [next] | [standalone]

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

FromMichal Hocko <mhocko@kernel.org>
Date2017-06-14 08:40 +0200
SubjectRe: [RESEND PATCH] base/memory: pass the base_section in add_memory_block
Message-ID<tSa89-1FC-11@gated-at.bofh.it>
In reply to#1665466
On Wed 14-06-17 14:05:58, Wei Yang wrote:
> 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 code needs a larger rething rather than here and there small
changes I believe.

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

You are right. The code is confusing as hell!

That being said, I am not opposing the patch but I would much rather
appreciate a consistent cleanup in the whole memblock vs. sections area.
That would be a larger project but the end result is really worth it.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web