* [RESEND PATCH] base/memory: pass the base_section in add_memory_block @ 2017-06-14 5:45 Wei Yang 2017-06-14 5:59 ` Michal Hocko 2017-06-14 6:05 ` Wei Yang 0 siblings, 2 replies; 6+ messages in thread From: Wei Yang @ 2017-06-14 5:45 UTC (permalink / raw) To: gregkh, mhocko; +Cc: linux-kernel, linux-mm, 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 <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 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND PATCH] base/memory: pass the base_section in add_memory_block 2017-06-14 5:45 [RESEND PATCH] base/memory: pass the base_section in add_memory_block Wei Yang @ 2017-06-14 5:59 ` Michal Hocko 2017-06-14 6:19 ` Wei Yang 2017-06-14 6:05 ` Wei Yang 1 sibling, 1 reply; 6+ messages in thread From: Michal Hocko @ 2017-06-14 5:59 UTC (permalink / raw) To: Wei Yang; +Cc: gregkh, linux-kernel, linux-mm 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 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND PATCH] base/memory: pass the base_section in add_memory_block 2017-06-14 5:59 ` Michal Hocko @ 2017-06-14 6:19 ` Wei Yang 2017-06-14 6:22 ` Michal Hocko 0 siblings, 1 reply; 6+ messages in thread From: Wei Yang @ 2017-06-14 6:19 UTC (permalink / raw) To: Michal Hocko; +Cc: Wei Yang, gregkh, linux-kernel, linux-mm [-- Attachment #1: Type: text/plain, Size: 2114 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND PATCH] base/memory: pass the base_section in add_memory_block 2017-06-14 6:19 ` Wei Yang @ 2017-06-14 6:22 ` Michal Hocko 0 siblings, 0 replies; 6+ messages in thread From: Michal Hocko @ 2017-06-14 6:22 UTC (permalink / raw) To: Wei Yang; +Cc: gregkh, linux-kernel, linux-mm 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 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND PATCH] base/memory: pass the base_section in add_memory_block 2017-06-14 5:45 [RESEND PATCH] base/memory: pass the base_section in add_memory_block Wei Yang 2017-06-14 5:59 ` Michal Hocko @ 2017-06-14 6:05 ` Wei Yang 2017-06-14 6:30 ` Michal Hocko 1 sibling, 1 reply; 6+ messages in thread From: Wei Yang @ 2017-06-14 6:05 UTC (permalink / raw) To: Wei Yang; +Cc: gregkh, mhocko, linux-kernel, linux-mm [-- Attachment #1: Type: text/plain, Size: 3680 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RESEND PATCH] base/memory: pass the base_section in add_memory_block 2017-06-14 6:05 ` Wei Yang @ 2017-06-14 6:30 ` Michal Hocko 0 siblings, 0 replies; 6+ messages in thread From: Michal Hocko @ 2017-06-14 6:30 UTC (permalink / raw) To: Wei Yang; +Cc: gregkh, linux-kernel, linux-mm 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 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-06-14 6:30 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-14 5:45 [RESEND PATCH] base/memory: pass the base_section in add_memory_block Wei Yang 2017-06-14 5:59 ` Michal Hocko 2017-06-14 6:19 ` Wei Yang 2017-06-14 6:22 ` Michal Hocko 2017-06-14 6:05 ` Wei Yang 2017-06-14 6:30 ` Michal Hocko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox