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

[Michal Hocko]

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

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

[Michal Hocko]

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

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

[Michal Hocko]

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

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

[Michal Hocko]

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

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

[Wei Yang]

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.gi10...@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 
>> ---
>>  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(, __nr_to_section(section_nr), MEM_ONLINE);
>> +ret = init_memory_block(, __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


signature.asc
Description: PGP signature

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

[Wei Yang]

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.gi10...@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 
>> ---
>>  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(, __nr_to_section(section_nr), MEM_ONLINE);
>> +ret = init_memory_block(, __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


signature.asc
Description: PGP signature

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 
>---
> 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(, __nr_to_section(section_nr), MEM_ONLINE);
>+  ret = init_memory_block(, __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


signature.asc
Description: PGP signature

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 
>---
> 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(, __nr_to_section(section_nr), MEM_ONLINE);
>+  ret = init_memory_block(, __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


signature.asc
Description: PGP signature

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

[Michal Hocko]

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.gi10...@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 
> ---
>  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(, __nr_to_section(section_nr), MEM_ONLINE);
> + ret = init_memory_block(, __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: [RESEND PATCH] base/memory: pass the base_section in add_memory_block

[Michal Hocko]

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.gi10...@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 
> ---
>  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(, __nr_to_section(section_nr), MEM_ONLINE);
> + ret = init_memory_block(, __nr_to_section(base_section_nr),
> + MEM_ONLINE);
>   if (ret)
>   return ret;
>   mem->section_count = section_count;
> -- 
> 2.11.0

-- 
Michal Hocko
SUSE Labs

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

[Wei Yang]

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 
---
 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(, __nr_to_section(section_nr), MEM_ONLINE);
+   ret = init_memory_block(, __nr_to_section(base_section_nr),
+   MEM_ONLINE);
if (ret)
return ret;
mem->section_count = section_count;
-- 
2.11.0

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

[Wei Yang]

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 
---
 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(, __nr_to_section(section_nr), MEM_ONLINE);
+   ret = init_memory_block(, __nr_to_section(base_section_nr),
+   MEM_ONLINE);
if (ret)
return ret;
mem->section_count = section_count;
-- 
2.11.0