* [PATCH V2 0/2] mm/memblock.c: fix potential bug and code refine @ 2016-12-18 14:47 ` Wei Yang 0 siblings, 0 replies; 32+ messages in thread From: Wei Yang @ 2016-12-18 14:47 UTC (permalink / raw) To: trivial, akpm; +Cc: linux-mm, linux-kernel, Wei Yang Here are two patch of mm/memblock.c. [1]. A trivial code refine in memblock_is_region_memory(), which removes an unnecessary check on base address. [2]. The original code forgets to check the return value of memblock_reserve(), which may lead to potential problem. The patch fix this. --- v2: remove a trivial code refine, which is already fixed in upstream Wei Yang (2): mm/memblock.c: trivial code refine in memblock_is_region_memory() mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal() include/linux/memblock.h | 5 ++--- mm/memblock.c | 8 +++----- 2 files changed, 5 insertions(+), 8 deletions(-) -- 1.7.9.5 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH V2 0/2] mm/memblock.c: fix potential bug and code refine @ 2016-12-18 14:47 ` Wei Yang 0 siblings, 0 replies; 32+ messages in thread From: Wei Yang @ 2016-12-18 14:47 UTC (permalink / raw) To: trivial, akpm; +Cc: linux-mm, linux-kernel, Wei Yang Here are two patch of mm/memblock.c. [1]. A trivial code refine in memblock_is_region_memory(), which removes an unnecessary check on base address. [2]. The original code forgets to check the return value of memblock_reserve(), which may lead to potential problem. The patch fix this. --- v2: remove a trivial code refine, which is already fixed in upstream Wei Yang (2): mm/memblock.c: trivial code refine in memblock_is_region_memory() mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal() include/linux/memblock.h | 5 ++--- mm/memblock.c | 8 +++----- 2 files changed, 5 insertions(+), 8 deletions(-) -- 1.7.9.5 -- 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] 32+ messages in thread
* [PATCH V2 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory() 2016-12-18 14:47 ` Wei Yang @ 2016-12-18 14:47 ` Wei Yang -1 siblings, 0 replies; 32+ messages in thread From: Wei Yang @ 2016-12-18 14:47 UTC (permalink / raw) To: trivial, akpm; +Cc: linux-mm, linux-kernel, Wei Yang The base address is already guaranteed to be in the region by memblock_search(). This patch removes the check on base. Signed-off-by: Wei Yang <richard.weiyang@gmail.com> --- mm/memblock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mm/memblock.c b/mm/memblock.c index 7608bc3..cd85303 100644 --- a/mm/memblock.c +++ b/mm/memblock.c @@ -1615,7 +1615,7 @@ int __init_memblock memblock_is_region_memory(phys_addr_t base, phys_addr_t size if (idx == -1) return 0; - return memblock.memory.regions[idx].base <= base && + return /* memblock.memory.regions[idx].base <= base && */ (memblock.memory.regions[idx].base + memblock.memory.regions[idx].size) >= end; } -- 2.5.0 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH V2 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory() @ 2016-12-18 14:47 ` Wei Yang 0 siblings, 0 replies; 32+ messages in thread From: Wei Yang @ 2016-12-18 14:47 UTC (permalink / raw) To: trivial, akpm; +Cc: linux-mm, linux-kernel, Wei Yang The base address is already guaranteed to be in the region by memblock_search(). This patch removes the check on base. Signed-off-by: Wei Yang <richard.weiyang@gmail.com> --- mm/memblock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mm/memblock.c b/mm/memblock.c index 7608bc3..cd85303 100644 --- a/mm/memblock.c +++ b/mm/memblock.c @@ -1615,7 +1615,7 @@ int __init_memblock memblock_is_region_memory(phys_addr_t base, phys_addr_t size if (idx == -1) return 0; - return memblock.memory.regions[idx].base <= base && + return /* memblock.memory.regions[idx].base <= base && */ (memblock.memory.regions[idx].base + memblock.memory.regions[idx].size) >= end; } -- 2.5.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 related [flat|nested] 32+ messages in thread
* Re: [PATCH V2 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory() 2016-12-18 14:47 ` Wei Yang @ 2016-12-19 15:15 ` Michal Hocko -1 siblings, 0 replies; 32+ messages in thread From: Michal Hocko @ 2016-12-19 15:15 UTC (permalink / raw) To: Wei Yang; +Cc: trivial, akpm, linux-mm, linux-kernel On Sun 18-12-16 14:47:49, Wei Yang wrote: > The base address is already guaranteed to be in the region by > memblock_search(). First of all the way how the check is removed is the worst possible... Apart from that it is really not clear to me why checking the base is not needed. You are mentioning memblock_search but what about other callers? adjust_range_page_size_mask e.g... You also didn't mention what is the motivation of this change? What will work better or why it makes sense in general? Also this seems to be a general purpose function so it should better be robust. > This patch removes the check on base. > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> Without a proper justification and with the horrible way how it is done Nacked-by: Michal Hocko <mhocko@suse.com> > --- > mm/memblock.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/memblock.c b/mm/memblock.c > index 7608bc3..cd85303 100644 > --- a/mm/memblock.c > +++ b/mm/memblock.c > @@ -1615,7 +1615,7 @@ int __init_memblock memblock_is_region_memory(phys_addr_t base, phys_addr_t size > > if (idx == -1) > return 0; > - return memblock.memory.regions[idx].base <= base && > + return /* memblock.memory.regions[idx].base <= base && */ > (memblock.memory.regions[idx].base + > memblock.memory.regions[idx].size) >= end; > } > -- > 2.5.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> -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory() @ 2016-12-19 15:15 ` Michal Hocko 0 siblings, 0 replies; 32+ messages in thread From: Michal Hocko @ 2016-12-19 15:15 UTC (permalink / raw) To: Wei Yang; +Cc: trivial, akpm, linux-mm, linux-kernel On Sun 18-12-16 14:47:49, Wei Yang wrote: > The base address is already guaranteed to be in the region by > memblock_search(). First of all the way how the check is removed is the worst possible... Apart from that it is really not clear to me why checking the base is not needed. You are mentioning memblock_search but what about other callers? adjust_range_page_size_mask e.g... You also didn't mention what is the motivation of this change? What will work better or why it makes sense in general? Also this seems to be a general purpose function so it should better be robust. > This patch removes the check on base. > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> Without a proper justification and with the horrible way how it is done Nacked-by: Michal Hocko <mhocko@suse.com> > --- > mm/memblock.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/memblock.c b/mm/memblock.c > index 7608bc3..cd85303 100644 > --- a/mm/memblock.c > +++ b/mm/memblock.c > @@ -1615,7 +1615,7 @@ int __init_memblock memblock_is_region_memory(phys_addr_t base, phys_addr_t size > > if (idx == -1) > return 0; > - return memblock.memory.regions[idx].base <= base && > + return /* memblock.memory.regions[idx].base <= base && */ > (memblock.memory.regions[idx].base + > memblock.memory.regions[idx].size) >= end; > } > -- > 2.5.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> -- 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] 32+ messages in thread
* Re: [PATCH V2 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory() 2016-12-19 15:15 ` Michal Hocko @ 2016-12-20 16:35 ` Wei Yang -1 siblings, 0 replies; 32+ messages in thread From: Wei Yang @ 2016-12-20 16:35 UTC (permalink / raw) To: Michal Hocko; +Cc: Wei Yang, trivial, akpm, linux-mm, linux-kernel On Mon, Dec 19, 2016 at 04:15:14PM +0100, Michal Hocko wrote: >On Sun 18-12-16 14:47:49, Wei Yang wrote: >> The base address is already guaranteed to be in the region by >> memblock_search(). > Hi, Michal Nice to receive your comment. >First of all the way how the check is removed is the worst possible... >Apart from that it is really not clear to me why checking the base >is not needed. You are mentioning memblock_search but what about other >callers? adjust_range_page_size_mask e.g... > Hmm... the memblock_search() is called by memblock_is_region_memory(). Maybe I paste the whole function here would clarify the change. int __init_memblock memblock_is_region_memory(phys_addr_t base, phys_addr_t size) { int idx = memblock_search(&memblock.memory, base); phys_addr_t end = base + memblock_cap_size(base, &size); if (idx == -1) return 0; return memblock.memory.regions[idx].base <= base && (memblock.memory.regions[idx].base + memblock.memory.regions[idx].size) >= end; } So memblock_search() will search "base" in memblock.memory. If "base" is not in memblock.memory, idx would be -1. Then following code will not be executed. And if the following code is executed, it means idx is not -1 and memblock_search() has found the "base" in memblock.memory.regions[idx], which is ture for statement (memblock.memory.regions[idx].base <= base). >You also didn't mention what is the motivation of this change? What will >work better or why it makes sense in general? > The purpose is to improve the code by reduce an extra check. >Also this seems to be a general purpose function so it should better >be robust. > I think it is as robust as it was. >> This patch removes the check on base. >> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > >Without a proper justification and with the horrible way how it is done >Nacked-by: Michal Hocko <mhocko@suse.com> > Not sure I make it clear or I may miss something? >> --- >> mm/memblock.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/mm/memblock.c b/mm/memblock.c >> index 7608bc3..cd85303 100644 >> --- a/mm/memblock.c >> +++ b/mm/memblock.c >> @@ -1615,7 +1615,7 @@ int __init_memblock memblock_is_region_memory(phys_addr_t base, phys_addr_t size >> >> if (idx == -1) >> return 0; >> - return memblock.memory.regions[idx].base <= base && >> + return /* memblock.memory.regions[idx].base <= base && */ >> (memblock.memory.regions[idx].base + >> memblock.memory.regions[idx].size) >= end; >> } >> -- >> 2.5.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> > >-- >Michal Hocko >SUSE Labs -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory() @ 2016-12-20 16:35 ` Wei Yang 0 siblings, 0 replies; 32+ messages in thread From: Wei Yang @ 2016-12-20 16:35 UTC (permalink / raw) To: Michal Hocko; +Cc: Wei Yang, trivial, akpm, linux-mm, linux-kernel On Mon, Dec 19, 2016 at 04:15:14PM +0100, Michal Hocko wrote: >On Sun 18-12-16 14:47:49, Wei Yang wrote: >> The base address is already guaranteed to be in the region by >> memblock_search(). > Hi, Michal Nice to receive your comment. >First of all the way how the check is removed is the worst possible... >Apart from that it is really not clear to me why checking the base >is not needed. You are mentioning memblock_search but what about other >callers? adjust_range_page_size_mask e.g... > Hmm... the memblock_search() is called by memblock_is_region_memory(). Maybe I paste the whole function here would clarify the change. int __init_memblock memblock_is_region_memory(phys_addr_t base, phys_addr_t size) { int idx = memblock_search(&memblock.memory, base); phys_addr_t end = base + memblock_cap_size(base, &size); if (idx == -1) return 0; return memblock.memory.regions[idx].base <= base && (memblock.memory.regions[idx].base + memblock.memory.regions[idx].size) >= end; } So memblock_search() will search "base" in memblock.memory. If "base" is not in memblock.memory, idx would be -1. Then following code will not be executed. And if the following code is executed, it means idx is not -1 and memblock_search() has found the "base" in memblock.memory.regions[idx], which is ture for statement (memblock.memory.regions[idx].base <= base). >You also didn't mention what is the motivation of this change? What will >work better or why it makes sense in general? > The purpose is to improve the code by reduce an extra check. >Also this seems to be a general purpose function so it should better >be robust. > I think it is as robust as it was. >> This patch removes the check on base. >> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > >Without a proper justification and with the horrible way how it is done >Nacked-by: Michal Hocko <mhocko@suse.com> > Not sure I make it clear or I may miss something? >> --- >> mm/memblock.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/mm/memblock.c b/mm/memblock.c >> index 7608bc3..cd85303 100644 >> --- a/mm/memblock.c >> +++ b/mm/memblock.c >> @@ -1615,7 +1615,7 @@ int __init_memblock memblock_is_region_memory(phys_addr_t base, phys_addr_t size >> >> if (idx == -1) >> return 0; >> - return memblock.memory.regions[idx].base <= base && >> + return /* memblock.memory.regions[idx].base <= base && */ >> (memblock.memory.regions[idx].base + >> memblock.memory.regions[idx].size) >= end; >> } >> -- >> 2.5.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> > >-- >Michal Hocko >SUSE Labs -- Wei Yang Help you, Help me -- 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] 32+ messages in thread
* Re: [PATCH V2 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory() 2016-12-20 16:35 ` Wei Yang @ 2016-12-21 7:48 ` Michal Hocko -1 siblings, 0 replies; 32+ messages in thread From: Michal Hocko @ 2016-12-21 7:48 UTC (permalink / raw) To: Wei Yang; +Cc: trivial, akpm, linux-mm, linux-kernel On Tue 20-12-16 16:35:40, Wei Yang wrote: > On Mon, Dec 19, 2016 at 04:15:14PM +0100, Michal Hocko wrote: > >On Sun 18-12-16 14:47:49, Wei Yang wrote: > >> The base address is already guaranteed to be in the region by > >> memblock_search(). > > > > Hi, Michal > > Nice to receive your comment. > > >First of all the way how the check is removed is the worst possible... > >Apart from that it is really not clear to me why checking the base > >is not needed. You are mentioning memblock_search but what about other > >callers? adjust_range_page_size_mask e.g... > > > > Hmm... the memblock_search() is called by memblock_is_region_memory(). Maybe I > paste the whole function here would clarify the change. > > int __init_memblock memblock_is_region_memory(phys_addr_t base, phys_addr_t size) > { > int idx = memblock_search(&memblock.memory, base); > phys_addr_t end = base + memblock_cap_size(base, &size); > > if (idx == -1) > return 0; > return memblock.memory.regions[idx].base <= base && > (memblock.memory.regions[idx].base + > memblock.memory.regions[idx].size) >= end; > } Ohh, my bad. I thought that memblock_search is calling memblock_is_region_memory. I didn't notice this is other way around. Then I agree that the check for the base is not needed and can be removed. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory() @ 2016-12-21 7:48 ` Michal Hocko 0 siblings, 0 replies; 32+ messages in thread From: Michal Hocko @ 2016-12-21 7:48 UTC (permalink / raw) To: Wei Yang; +Cc: trivial, akpm, linux-mm, linux-kernel On Tue 20-12-16 16:35:40, Wei Yang wrote: > On Mon, Dec 19, 2016 at 04:15:14PM +0100, Michal Hocko wrote: > >On Sun 18-12-16 14:47:49, Wei Yang wrote: > >> The base address is already guaranteed to be in the region by > >> memblock_search(). > > > > Hi, Michal > > Nice to receive your comment. > > >First of all the way how the check is removed is the worst possible... > >Apart from that it is really not clear to me why checking the base > >is not needed. You are mentioning memblock_search but what about other > >callers? adjust_range_page_size_mask e.g... > > > > Hmm... the memblock_search() is called by memblock_is_region_memory(). Maybe I > paste the whole function here would clarify the change. > > int __init_memblock memblock_is_region_memory(phys_addr_t base, phys_addr_t size) > { > int idx = memblock_search(&memblock.memory, base); > phys_addr_t end = base + memblock_cap_size(base, &size); > > if (idx == -1) > return 0; > return memblock.memory.regions[idx].base <= base && > (memblock.memory.regions[idx].base + > memblock.memory.regions[idx].size) >= end; > } Ohh, my bad. I thought that memblock_search is calling memblock_is_region_memory. I didn't notice this is other way around. Then I agree that the check for the base is not needed and can be removed. -- 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] 32+ messages in thread
* Re: [PATCH V2 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory() 2016-12-21 7:48 ` Michal Hocko @ 2016-12-21 12:43 ` Wei Yang -1 siblings, 0 replies; 32+ messages in thread From: Wei Yang @ 2016-12-21 12:43 UTC (permalink / raw) To: Michal Hocko; +Cc: Wei Yang, trivial, akpm, linux-mm, linux-kernel On Wed, Dec 21, 2016 at 08:48:09AM +0100, Michal Hocko wrote: >On Tue 20-12-16 16:35:40, Wei Yang wrote: >> On Mon, Dec 19, 2016 at 04:15:14PM +0100, Michal Hocko wrote: >> >On Sun 18-12-16 14:47:49, Wei Yang wrote: >> >> The base address is already guaranteed to be in the region by >> >> memblock_search(). >> > >> >> Hi, Michal >> >> Nice to receive your comment. >> >> >First of all the way how the check is removed is the worst possible... >> >Apart from that it is really not clear to me why checking the base >> >is not needed. You are mentioning memblock_search but what about other >> >callers? adjust_range_page_size_mask e.g... >> > >> >> Hmm... the memblock_search() is called by memblock_is_region_memory(). Maybe I >> paste the whole function here would clarify the change. >> >> int __init_memblock memblock_is_region_memory(phys_addr_t base, phys_addr_t size) >> { >> int idx = memblock_search(&memblock.memory, base); >> phys_addr_t end = base + memblock_cap_size(base, &size); >> >> if (idx == -1) >> return 0; >> return memblock.memory.regions[idx].base <= base && >> (memblock.memory.regions[idx].base + >> memblock.memory.regions[idx].size) >= end; >> } > >Ohh, my bad. I thought that memblock_search is calling >memblock_is_region_memory. I didn't notice this is other way around. >Then I agree that the check for the base is not needed and can be >removed. Thanks~ I would feel honored if you would like to add Acked-by :-) >-- >Michal Hocko >SUSE Labs -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory() @ 2016-12-21 12:43 ` Wei Yang 0 siblings, 0 replies; 32+ messages in thread From: Wei Yang @ 2016-12-21 12:43 UTC (permalink / raw) To: Michal Hocko; +Cc: Wei Yang, trivial, akpm, linux-mm, linux-kernel On Wed, Dec 21, 2016 at 08:48:09AM +0100, Michal Hocko wrote: >On Tue 20-12-16 16:35:40, Wei Yang wrote: >> On Mon, Dec 19, 2016 at 04:15:14PM +0100, Michal Hocko wrote: >> >On Sun 18-12-16 14:47:49, Wei Yang wrote: >> >> The base address is already guaranteed to be in the region by >> >> memblock_search(). >> > >> >> Hi, Michal >> >> Nice to receive your comment. >> >> >First of all the way how the check is removed is the worst possible... >> >Apart from that it is really not clear to me why checking the base >> >is not needed. You are mentioning memblock_search but what about other >> >callers? adjust_range_page_size_mask e.g... >> > >> >> Hmm... the memblock_search() is called by memblock_is_region_memory(). Maybe I >> paste the whole function here would clarify the change. >> >> int __init_memblock memblock_is_region_memory(phys_addr_t base, phys_addr_t size) >> { >> int idx = memblock_search(&memblock.memory, base); >> phys_addr_t end = base + memblock_cap_size(base, &size); >> >> if (idx == -1) >> return 0; >> return memblock.memory.regions[idx].base <= base && >> (memblock.memory.regions[idx].base + >> memblock.memory.regions[idx].size) >= end; >> } > >Ohh, my bad. I thought that memblock_search is calling >memblock_is_region_memory. I didn't notice this is other way around. >Then I agree that the check for the base is not needed and can be >removed. Thanks~ I would feel honored if you would like to add Acked-by :-) >-- >Michal Hocko >SUSE Labs -- Wei Yang Help you, Help me -- 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] 32+ messages in thread
* Re: [PATCH V2 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory() 2016-12-21 12:43 ` Wei Yang @ 2016-12-21 12:48 ` Michal Hocko -1 siblings, 0 replies; 32+ messages in thread From: Michal Hocko @ 2016-12-21 12:48 UTC (permalink / raw) To: Wei Yang; +Cc: trivial, akpm, linux-mm, linux-kernel On Wed 21-12-16 12:43:20, Wei Yang wrote: > On Wed, Dec 21, 2016 at 08:48:09AM +0100, Michal Hocko wrote: > >On Tue 20-12-16 16:35:40, Wei Yang wrote: > >> On Mon, Dec 19, 2016 at 04:15:14PM +0100, Michal Hocko wrote: > >> >On Sun 18-12-16 14:47:49, Wei Yang wrote: > >> >> The base address is already guaranteed to be in the region by > >> >> memblock_search(). > >> > > >> > >> Hi, Michal > >> > >> Nice to receive your comment. > >> > >> >First of all the way how the check is removed is the worst possible... > >> >Apart from that it is really not clear to me why checking the base > >> >is not needed. You are mentioning memblock_search but what about other > >> >callers? adjust_range_page_size_mask e.g... > >> > > >> > >> Hmm... the memblock_search() is called by memblock_is_region_memory(). Maybe I > >> paste the whole function here would clarify the change. > >> > >> int __init_memblock memblock_is_region_memory(phys_addr_t base, phys_addr_t size) > >> { > >> int idx = memblock_search(&memblock.memory, base); > >> phys_addr_t end = base + memblock_cap_size(base, &size); > >> > >> if (idx == -1) > >> return 0; > >> return memblock.memory.regions[idx].base <= base && > >> (memblock.memory.regions[idx].base + > >> memblock.memory.regions[idx].size) >= end; > >> } > > > >Ohh, my bad. I thought that memblock_search is calling > >memblock_is_region_memory. I didn't notice this is other way around. > >Then I agree that the check for the base is not needed and can be > >removed. > > Thanks~ > > I would feel honored if you would like to add Acked-by :-) My Nack to the original patch still holds. If you want to remove the check then remove it rather than comment it out. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory() @ 2016-12-21 12:48 ` Michal Hocko 0 siblings, 0 replies; 32+ messages in thread From: Michal Hocko @ 2016-12-21 12:48 UTC (permalink / raw) To: Wei Yang; +Cc: trivial, akpm, linux-mm, linux-kernel On Wed 21-12-16 12:43:20, Wei Yang wrote: > On Wed, Dec 21, 2016 at 08:48:09AM +0100, Michal Hocko wrote: > >On Tue 20-12-16 16:35:40, Wei Yang wrote: > >> On Mon, Dec 19, 2016 at 04:15:14PM +0100, Michal Hocko wrote: > >> >On Sun 18-12-16 14:47:49, Wei Yang wrote: > >> >> The base address is already guaranteed to be in the region by > >> >> memblock_search(). > >> > > >> > >> Hi, Michal > >> > >> Nice to receive your comment. > >> > >> >First of all the way how the check is removed is the worst possible... > >> >Apart from that it is really not clear to me why checking the base > >> >is not needed. You are mentioning memblock_search but what about other > >> >callers? adjust_range_page_size_mask e.g... > >> > > >> > >> Hmm... the memblock_search() is called by memblock_is_region_memory(). Maybe I > >> paste the whole function here would clarify the change. > >> > >> int __init_memblock memblock_is_region_memory(phys_addr_t base, phys_addr_t size) > >> { > >> int idx = memblock_search(&memblock.memory, base); > >> phys_addr_t end = base + memblock_cap_size(base, &size); > >> > >> if (idx == -1) > >> return 0; > >> return memblock.memory.regions[idx].base <= base && > >> (memblock.memory.regions[idx].base + > >> memblock.memory.regions[idx].size) >= end; > >> } > > > >Ohh, my bad. I thought that memblock_search is calling > >memblock_is_region_memory. I didn't notice this is other way around. > >Then I agree that the check for the base is not needed and can be > >removed. > > Thanks~ > > I would feel honored if you would like to add Acked-by :-) My Nack to the original patch still holds. If you want to remove the check then remove it rather than comment it out. -- 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] 32+ messages in thread
* Re: [PATCH V2 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory() 2016-12-21 12:48 ` Michal Hocko @ 2016-12-21 13:15 ` Wei Yang -1 siblings, 0 replies; 32+ messages in thread From: Wei Yang @ 2016-12-21 13:15 UTC (permalink / raw) To: Michal Hocko; +Cc: Wei Yang, trivial, akpm, linux-mm, linux-kernel On Wed, Dec 21, 2016 at 01:48:17PM +0100, Michal Hocko wrote: >On Wed 21-12-16 12:43:20, Wei Yang wrote: >> On Wed, Dec 21, 2016 at 08:48:09AM +0100, Michal Hocko wrote: >> >On Tue 20-12-16 16:35:40, Wei Yang wrote: >> >> On Mon, Dec 19, 2016 at 04:15:14PM +0100, Michal Hocko wrote: >> >> >On Sun 18-12-16 14:47:49, Wei Yang wrote: >> >> >> The base address is already guaranteed to be in the region by >> >> >> memblock_search(). >> >> > >> >> >> >> Hi, Michal >> >> >> >> Nice to receive your comment. >> >> >> >> >First of all the way how the check is removed is the worst possible... >> >> >Apart from that it is really not clear to me why checking the base >> >> >is not needed. You are mentioning memblock_search but what about other >> >> >callers? adjust_range_page_size_mask e.g... >> >> > >> >> >> >> Hmm... the memblock_search() is called by memblock_is_region_memory(). Maybe I >> >> paste the whole function here would clarify the change. >> >> >> >> int __init_memblock memblock_is_region_memory(phys_addr_t base, phys_addr_t size) >> >> { >> >> int idx = memblock_search(&memblock.memory, base); >> >> phys_addr_t end = base + memblock_cap_size(base, &size); >> >> >> >> if (idx == -1) >> >> return 0; >> >> return memblock.memory.regions[idx].base <= base && >> >> (memblock.memory.regions[idx].base + >> >> memblock.memory.regions[idx].size) >= end; >> >> } >> > >> >Ohh, my bad. I thought that memblock_search is calling >> >memblock_is_region_memory. I didn't notice this is other way around. >> >Then I agree that the check for the base is not needed and can be >> >removed. >> >> Thanks~ >> >> I would feel honored if you would like to add Acked-by :-) > >My Nack to the original patch still holds. If you want to remove the >check then remove it rather than comment it out. Got it, will send a new version. >-- >Michal Hocko >SUSE Labs -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory() @ 2016-12-21 13:15 ` Wei Yang 0 siblings, 0 replies; 32+ messages in thread From: Wei Yang @ 2016-12-21 13:15 UTC (permalink / raw) To: Michal Hocko; +Cc: Wei Yang, trivial, akpm, linux-mm, linux-kernel On Wed, Dec 21, 2016 at 01:48:17PM +0100, Michal Hocko wrote: >On Wed 21-12-16 12:43:20, Wei Yang wrote: >> On Wed, Dec 21, 2016 at 08:48:09AM +0100, Michal Hocko wrote: >> >On Tue 20-12-16 16:35:40, Wei Yang wrote: >> >> On Mon, Dec 19, 2016 at 04:15:14PM +0100, Michal Hocko wrote: >> >> >On Sun 18-12-16 14:47:49, Wei Yang wrote: >> >> >> The base address is already guaranteed to be in the region by >> >> >> memblock_search(). >> >> > >> >> >> >> Hi, Michal >> >> >> >> Nice to receive your comment. >> >> >> >> >First of all the way how the check is removed is the worst possible... >> >> >Apart from that it is really not clear to me why checking the base >> >> >is not needed. You are mentioning memblock_search but what about other >> >> >callers? adjust_range_page_size_mask e.g... >> >> > >> >> >> >> Hmm... the memblock_search() is called by memblock_is_region_memory(). Maybe I >> >> paste the whole function here would clarify the change. >> >> >> >> int __init_memblock memblock_is_region_memory(phys_addr_t base, phys_addr_t size) >> >> { >> >> int idx = memblock_search(&memblock.memory, base); >> >> phys_addr_t end = base + memblock_cap_size(base, &size); >> >> >> >> if (idx == -1) >> >> return 0; >> >> return memblock.memory.regions[idx].base <= base && >> >> (memblock.memory.regions[idx].base + >> >> memblock.memory.regions[idx].size) >= end; >> >> } >> > >> >Ohh, my bad. I thought that memblock_search is calling >> >memblock_is_region_memory. I didn't notice this is other way around. >> >Then I agree that the check for the base is not needed and can be >> >removed. >> >> Thanks~ >> >> I would feel honored if you would like to add Acked-by :-) > >My Nack to the original patch still holds. If you want to remove the >check then remove it rather than comment it out. Got it, will send a new version. >-- >Michal Hocko >SUSE Labs -- Wei Yang Help you, Help me -- 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] 32+ messages in thread
* [PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal() 2016-12-18 14:47 ` Wei Yang @ 2016-12-18 14:47 ` Wei Yang -1 siblings, 0 replies; 32+ messages in thread From: Wei Yang @ 2016-12-18 14:47 UTC (permalink / raw) To: trivial, akpm; +Cc: linux-mm, linux-kernel, Wei Yang memblock_reserve() may fail in case there is not enough regions. This patch checks the return value. Signed-off-by: Wei Yang <richard.weiyang@gmail.com> --- mm/memblock.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/mm/memblock.c b/mm/memblock.c index cd85303..93fa687 100644 --- a/mm/memblock.c +++ b/mm/memblock.c @@ -1274,18 +1274,17 @@ static void * __init memblock_virt_alloc_internal( if (max_addr > memblock.current_limit) max_addr = memblock.current_limit; - again: alloc = memblock_find_in_range_node(size, align, min_addr, max_addr, nid, flags); - if (alloc) + if (alloc && !memblock_reserve(alloc, size)) goto done; if (nid != NUMA_NO_NODE) { alloc = memblock_find_in_range_node(size, align, min_addr, max_addr, NUMA_NO_NODE, flags); - if (alloc) + if (alloc && !memblock_reserve(alloc, size)) goto done; } @@ -1303,7 +1302,6 @@ static void * __init memblock_virt_alloc_internal( return NULL; done: - memblock_reserve(alloc, size); ptr = phys_to_virt(alloc); memset(ptr, 0, size); -- 2.5.0 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal() @ 2016-12-18 14:47 ` Wei Yang 0 siblings, 0 replies; 32+ messages in thread From: Wei Yang @ 2016-12-18 14:47 UTC (permalink / raw) To: trivial, akpm; +Cc: linux-mm, linux-kernel, Wei Yang memblock_reserve() may fail in case there is not enough regions. This patch checks the return value. Signed-off-by: Wei Yang <richard.weiyang@gmail.com> --- mm/memblock.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/mm/memblock.c b/mm/memblock.c index cd85303..93fa687 100644 --- a/mm/memblock.c +++ b/mm/memblock.c @@ -1274,18 +1274,17 @@ static void * __init memblock_virt_alloc_internal( if (max_addr > memblock.current_limit) max_addr = memblock.current_limit; - again: alloc = memblock_find_in_range_node(size, align, min_addr, max_addr, nid, flags); - if (alloc) + if (alloc && !memblock_reserve(alloc, size)) goto done; if (nid != NUMA_NO_NODE) { alloc = memblock_find_in_range_node(size, align, min_addr, max_addr, NUMA_NO_NODE, flags); - if (alloc) + if (alloc && !memblock_reserve(alloc, size)) goto done; } @@ -1303,7 +1302,6 @@ static void * __init memblock_virt_alloc_internal( return NULL; done: - memblock_reserve(alloc, size); ptr = phys_to_virt(alloc); memset(ptr, 0, size); -- 2.5.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 related [flat|nested] 32+ messages in thread
* Re: [PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal() 2016-12-18 14:47 ` Wei Yang @ 2016-12-19 15:21 ` Michal Hocko -1 siblings, 0 replies; 32+ messages in thread From: Michal Hocko @ 2016-12-19 15:21 UTC (permalink / raw) To: Wei Yang; +Cc: trivial, akpm, linux-mm, linux-kernel On Sun 18-12-16 14:47:50, Wei Yang wrote: > memblock_reserve() may fail in case there is not enough regions. Have you seen this happenning in the real setups or this is a by-review driven change? [...] > again: > alloc = memblock_find_in_range_node(size, align, min_addr, max_addr, > nid, flags); > - if (alloc) > + if (alloc && !memblock_reserve(alloc, size)) > goto done; > > if (nid != NUMA_NO_NODE) { > alloc = memblock_find_in_range_node(size, align, min_addr, > max_addr, NUMA_NO_NODE, > flags); > - if (alloc) > + if (alloc && !memblock_reserve(alloc, size)) > goto done; > } This doesn't look right. You can end up leaking the first allocated range. > > @@ -1303,7 +1302,6 @@ static void * __init memblock_virt_alloc_internal( > > return NULL; > done: > - memblock_reserve(alloc, size); > ptr = phys_to_virt(alloc); > memset(ptr, 0, size); > > -- > 2.5.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> -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal() @ 2016-12-19 15:21 ` Michal Hocko 0 siblings, 0 replies; 32+ messages in thread From: Michal Hocko @ 2016-12-19 15:21 UTC (permalink / raw) To: Wei Yang; +Cc: trivial, akpm, linux-mm, linux-kernel On Sun 18-12-16 14:47:50, Wei Yang wrote: > memblock_reserve() may fail in case there is not enough regions. Have you seen this happenning in the real setups or this is a by-review driven change? [...] > again: > alloc = memblock_find_in_range_node(size, align, min_addr, max_addr, > nid, flags); > - if (alloc) > + if (alloc && !memblock_reserve(alloc, size)) > goto done; > > if (nid != NUMA_NO_NODE) { > alloc = memblock_find_in_range_node(size, align, min_addr, > max_addr, NUMA_NO_NODE, > flags); > - if (alloc) > + if (alloc && !memblock_reserve(alloc, size)) > goto done; > } This doesn't look right. You can end up leaking the first allocated range. > > @@ -1303,7 +1302,6 @@ static void * __init memblock_virt_alloc_internal( > > return NULL; > done: > - memblock_reserve(alloc, size); > ptr = phys_to_virt(alloc); > memset(ptr, 0, size); > > -- > 2.5.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> -- 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] 32+ messages in thread
* Re: [PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal() 2016-12-19 15:21 ` Michal Hocko @ 2016-12-20 16:48 ` Wei Yang -1 siblings, 0 replies; 32+ messages in thread From: Wei Yang @ 2016-12-20 16:48 UTC (permalink / raw) To: Michal Hocko; +Cc: Wei Yang, trivial, akpm, linux-mm, linux-kernel On Mon, Dec 19, 2016 at 04:21:57PM +0100, Michal Hocko wrote: >On Sun 18-12-16 14:47:50, Wei Yang wrote: >> memblock_reserve() may fail in case there is not enough regions. > >Have you seen this happenning in the real setups or this is a by-review >driven change? This is a by-review driven change. >[...] >> again: >> alloc = memblock_find_in_range_node(size, align, min_addr, max_addr, >> nid, flags); >> - if (alloc) >> + if (alloc && !memblock_reserve(alloc, size)) >> goto done; >> >> if (nid != NUMA_NO_NODE) { >> alloc = memblock_find_in_range_node(size, align, min_addr, >> max_addr, NUMA_NO_NODE, >> flags); >> - if (alloc) >> + if (alloc && !memblock_reserve(alloc, size)) >> goto done; >> } > >This doesn't look right. You can end up leaking the first allocated >range. > Hmm... why? If first memblock_reserve() succeed, it will jump to done, so that no 2nd allocation. If the second executes, it means the first allocation failed. memblock_find_in_range_node() doesn't modify the memblock, it just tell you there is a proper memory region available. >> >> @@ -1303,7 +1302,6 @@ static void * __init memblock_virt_alloc_internal( >> >> return NULL; >> done: >> - memblock_reserve(alloc, size); >> ptr = phys_to_virt(alloc); >> memset(ptr, 0, size); > > >> >> -- >> 2.5.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> > >-- >Michal Hocko >SUSE Labs -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal() @ 2016-12-20 16:48 ` Wei Yang 0 siblings, 0 replies; 32+ messages in thread From: Wei Yang @ 2016-12-20 16:48 UTC (permalink / raw) To: Michal Hocko; +Cc: Wei Yang, trivial, akpm, linux-mm, linux-kernel On Mon, Dec 19, 2016 at 04:21:57PM +0100, Michal Hocko wrote: >On Sun 18-12-16 14:47:50, Wei Yang wrote: >> memblock_reserve() may fail in case there is not enough regions. > >Have you seen this happenning in the real setups or this is a by-review >driven change? This is a by-review driven change. >[...] >> again: >> alloc = memblock_find_in_range_node(size, align, min_addr, max_addr, >> nid, flags); >> - if (alloc) >> + if (alloc && !memblock_reserve(alloc, size)) >> goto done; >> >> if (nid != NUMA_NO_NODE) { >> alloc = memblock_find_in_range_node(size, align, min_addr, >> max_addr, NUMA_NO_NODE, >> flags); >> - if (alloc) >> + if (alloc && !memblock_reserve(alloc, size)) >> goto done; >> } > >This doesn't look right. You can end up leaking the first allocated >range. > Hmm... why? If first memblock_reserve() succeed, it will jump to done, so that no 2nd allocation. If the second executes, it means the first allocation failed. memblock_find_in_range_node() doesn't modify the memblock, it just tell you there is a proper memory region available. >> >> @@ -1303,7 +1302,6 @@ static void * __init memblock_virt_alloc_internal( >> >> return NULL; >> done: >> - memblock_reserve(alloc, size); >> ptr = phys_to_virt(alloc); >> memset(ptr, 0, size); > > >> >> -- >> 2.5.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> > >-- >Michal Hocko >SUSE Labs -- Wei Yang Help you, Help me -- 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] 32+ messages in thread
* Re: [PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal() 2016-12-20 16:48 ` Wei Yang @ 2016-12-21 7:51 ` Michal Hocko -1 siblings, 0 replies; 32+ messages in thread From: Michal Hocko @ 2016-12-21 7:51 UTC (permalink / raw) To: Wei Yang; +Cc: trivial, akpm, linux-mm, linux-kernel On Tue 20-12-16 16:48:23, Wei Yang wrote: > On Mon, Dec 19, 2016 at 04:21:57PM +0100, Michal Hocko wrote: > >On Sun 18-12-16 14:47:50, Wei Yang wrote: > >> memblock_reserve() may fail in case there is not enough regions. > > > >Have you seen this happenning in the real setups or this is a by-review > >driven change? > > This is a by-review driven change. > > >[...] > >> again: > >> alloc = memblock_find_in_range_node(size, align, min_addr, max_addr, > >> nid, flags); > >> - if (alloc) > >> + if (alloc && !memblock_reserve(alloc, size)) > >> goto done; So how exactly does the reserve fail when memblock_find_in_range_node found a suitable range for the given size? > >> > >> if (nid != NUMA_NO_NODE) { > >> alloc = memblock_find_in_range_node(size, align, min_addr, > >> max_addr, NUMA_NO_NODE, > >> flags); > >> - if (alloc) > >> + if (alloc && !memblock_reserve(alloc, size)) > >> goto done; > >> } > > > >This doesn't look right. You can end up leaking the first allocated > >range. > > > > Hmm... why? > > If first memblock_reserve() succeed, it will jump to done, so that no 2nd > allocation. > If the second executes, it means the first allocation failed. > memblock_find_in_range_node() doesn't modify the memblock, it just tell you > there is a proper memory region available. yes, my bad. I have missed this. Sorry about the confusion. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal() @ 2016-12-21 7:51 ` Michal Hocko 0 siblings, 0 replies; 32+ messages in thread From: Michal Hocko @ 2016-12-21 7:51 UTC (permalink / raw) To: Wei Yang; +Cc: trivial, akpm, linux-mm, linux-kernel On Tue 20-12-16 16:48:23, Wei Yang wrote: > On Mon, Dec 19, 2016 at 04:21:57PM +0100, Michal Hocko wrote: > >On Sun 18-12-16 14:47:50, Wei Yang wrote: > >> memblock_reserve() may fail in case there is not enough regions. > > > >Have you seen this happenning in the real setups or this is a by-review > >driven change? > > This is a by-review driven change. > > >[...] > >> again: > >> alloc = memblock_find_in_range_node(size, align, min_addr, max_addr, > >> nid, flags); > >> - if (alloc) > >> + if (alloc && !memblock_reserve(alloc, size)) > >> goto done; So how exactly does the reserve fail when memblock_find_in_range_node found a suitable range for the given size? > >> > >> if (nid != NUMA_NO_NODE) { > >> alloc = memblock_find_in_range_node(size, align, min_addr, > >> max_addr, NUMA_NO_NODE, > >> flags); > >> - if (alloc) > >> + if (alloc && !memblock_reserve(alloc, size)) > >> goto done; > >> } > > > >This doesn't look right. You can end up leaking the first allocated > >range. > > > > Hmm... why? > > If first memblock_reserve() succeed, it will jump to done, so that no 2nd > allocation. > If the second executes, it means the first allocation failed. > memblock_find_in_range_node() doesn't modify the memblock, it just tell you > there is a proper memory region available. yes, my bad. I have missed this. Sorry about the confusion. -- 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] 32+ messages in thread
* Re: [PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal() 2016-12-21 7:51 ` Michal Hocko @ 2016-12-21 13:13 ` Wei Yang -1 siblings, 0 replies; 32+ messages in thread From: Wei Yang @ 2016-12-21 13:13 UTC (permalink / raw) To: Michal Hocko; +Cc: Wei Yang, trivial, akpm, linux-mm, linux-kernel On Wed, Dec 21, 2016 at 08:51:16AM +0100, Michal Hocko wrote: >On Tue 20-12-16 16:48:23, Wei Yang wrote: >> On Mon, Dec 19, 2016 at 04:21:57PM +0100, Michal Hocko wrote: >> >On Sun 18-12-16 14:47:50, Wei Yang wrote: >> >> memblock_reserve() may fail in case there is not enough regions. >> > >> >Have you seen this happenning in the real setups or this is a by-review >> >driven change? >> >> This is a by-review driven change. >> >> >[...] >> >> again: >> >> alloc = memblock_find_in_range_node(size, align, min_addr, max_addr, >> >> nid, flags); >> >> - if (alloc) >> >> + if (alloc && !memblock_reserve(alloc, size)) >> >> goto done; > >So how exactly does the reserve fail when memblock_find_in_range_node >found a suitable range for the given size? > Even memblock_find_in_range_node() gets a suitable range, memblock_reserve() still could fail. And the case just happens when memblock can't resize. memblock_reserve() reserve a range by adding a range to memblock.reserved. In case the memblock.reserved is full and can't resize, this fails. Not sure whether I get it clarified. >> >> >> >> if (nid != NUMA_NO_NODE) { >> >> alloc = memblock_find_in_range_node(size, align, min_addr, >> >> max_addr, NUMA_NO_NODE, >> >> flags); >> >> - if (alloc) >> >> + if (alloc && !memblock_reserve(alloc, size)) >> >> goto done; >> >> } >> > >> >This doesn't look right. You can end up leaking the first allocated >> >range. >> > >> >> Hmm... why? >> >> If first memblock_reserve() succeed, it will jump to done, so that no 2nd >> allocation. >> If the second executes, it means the first allocation failed. >> memblock_find_in_range_node() doesn't modify the memblock, it just tell you >> there is a proper memory region available. > >yes, my bad. I have missed this. Sorry about the confusion So do you agree with my patch now? >-- >Michal Hocko >SUSE Labs -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal() @ 2016-12-21 13:13 ` Wei Yang 0 siblings, 0 replies; 32+ messages in thread From: Wei Yang @ 2016-12-21 13:13 UTC (permalink / raw) To: Michal Hocko; +Cc: Wei Yang, trivial, akpm, linux-mm, linux-kernel On Wed, Dec 21, 2016 at 08:51:16AM +0100, Michal Hocko wrote: >On Tue 20-12-16 16:48:23, Wei Yang wrote: >> On Mon, Dec 19, 2016 at 04:21:57PM +0100, Michal Hocko wrote: >> >On Sun 18-12-16 14:47:50, Wei Yang wrote: >> >> memblock_reserve() may fail in case there is not enough regions. >> > >> >Have you seen this happenning in the real setups or this is a by-review >> >driven change? >> >> This is a by-review driven change. >> >> >[...] >> >> again: >> >> alloc = memblock_find_in_range_node(size, align, min_addr, max_addr, >> >> nid, flags); >> >> - if (alloc) >> >> + if (alloc && !memblock_reserve(alloc, size)) >> >> goto done; > >So how exactly does the reserve fail when memblock_find_in_range_node >found a suitable range for the given size? > Even memblock_find_in_range_node() gets a suitable range, memblock_reserve() still could fail. And the case just happens when memblock can't resize. memblock_reserve() reserve a range by adding a range to memblock.reserved. In case the memblock.reserved is full and can't resize, this fails. Not sure whether I get it clarified. >> >> >> >> if (nid != NUMA_NO_NODE) { >> >> alloc = memblock_find_in_range_node(size, align, min_addr, >> >> max_addr, NUMA_NO_NODE, >> >> flags); >> >> - if (alloc) >> >> + if (alloc && !memblock_reserve(alloc, size)) >> >> goto done; >> >> } >> > >> >This doesn't look right. You can end up leaking the first allocated >> >range. >> > >> >> Hmm... why? >> >> If first memblock_reserve() succeed, it will jump to done, so that no 2nd >> allocation. >> If the second executes, it means the first allocation failed. >> memblock_find_in_range_node() doesn't modify the memblock, it just tell you >> there is a proper memory region available. > >yes, my bad. I have missed this. Sorry about the confusion So do you agree with my patch now? >-- >Michal Hocko >SUSE Labs -- Wei Yang Help you, Help me -- 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] 32+ messages in thread
* Re: [PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal() 2016-12-21 13:13 ` Wei Yang @ 2016-12-21 13:22 ` Michal Hocko -1 siblings, 0 replies; 32+ messages in thread From: Michal Hocko @ 2016-12-21 13:22 UTC (permalink / raw) To: Wei Yang; +Cc: trivial, akpm, linux-mm, linux-kernel On Wed 21-12-16 13:13:32, Wei Yang wrote: > On Wed, Dec 21, 2016 at 08:51:16AM +0100, Michal Hocko wrote: > >On Tue 20-12-16 16:48:23, Wei Yang wrote: > >> On Mon, Dec 19, 2016 at 04:21:57PM +0100, Michal Hocko wrote: > >> >On Sun 18-12-16 14:47:50, Wei Yang wrote: > >> >> memblock_reserve() may fail in case there is not enough regions. > >> > > >> >Have you seen this happenning in the real setups or this is a by-review > >> >driven change? > >> > >> This is a by-review driven change. > >> > >> >[...] > >> >> again: > >> >> alloc = memblock_find_in_range_node(size, align, min_addr, max_addr, > >> >> nid, flags); > >> >> - if (alloc) > >> >> + if (alloc && !memblock_reserve(alloc, size)) > >> >> goto done; > > > >So how exactly does the reserve fail when memblock_find_in_range_node > >found a suitable range for the given size? > > > > Even memblock_find_in_range_node() gets a suitable range, memblock_reserve() > still could fail. And the case just happens when memblock can't resize. > memblock_reserve() reserve a range by adding a range to memblock.reserved. In > case the memblock.reserved is full and can't resize, this fails. Sorry for being dense but what does it mean that the reserved will get full? Also how probable is such a situation? Is it even real? In other words does this fix a real or only a theoretical problem? Anyway this all should be part of the changelog. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal() @ 2016-12-21 13:22 ` Michal Hocko 0 siblings, 0 replies; 32+ messages in thread From: Michal Hocko @ 2016-12-21 13:22 UTC (permalink / raw) To: Wei Yang; +Cc: trivial, akpm, linux-mm, linux-kernel On Wed 21-12-16 13:13:32, Wei Yang wrote: > On Wed, Dec 21, 2016 at 08:51:16AM +0100, Michal Hocko wrote: > >On Tue 20-12-16 16:48:23, Wei Yang wrote: > >> On Mon, Dec 19, 2016 at 04:21:57PM +0100, Michal Hocko wrote: > >> >On Sun 18-12-16 14:47:50, Wei Yang wrote: > >> >> memblock_reserve() may fail in case there is not enough regions. > >> > > >> >Have you seen this happenning in the real setups or this is a by-review > >> >driven change? > >> > >> This is a by-review driven change. > >> > >> >[...] > >> >> again: > >> >> alloc = memblock_find_in_range_node(size, align, min_addr, max_addr, > >> >> nid, flags); > >> >> - if (alloc) > >> >> + if (alloc && !memblock_reserve(alloc, size)) > >> >> goto done; > > > >So how exactly does the reserve fail when memblock_find_in_range_node > >found a suitable range for the given size? > > > > Even memblock_find_in_range_node() gets a suitable range, memblock_reserve() > still could fail. And the case just happens when memblock can't resize. > memblock_reserve() reserve a range by adding a range to memblock.reserved. In > case the memblock.reserved is full and can't resize, this fails. Sorry for being dense but what does it mean that the reserved will get full? Also how probable is such a situation? Is it even real? In other words does this fix a real or only a theoretical problem? Anyway this all should be part of the changelog. -- 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] 32+ messages in thread
* Re: [PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal() 2016-12-21 13:22 ` Michal Hocko @ 2016-12-21 14:39 ` Wei Yang -1 siblings, 0 replies; 32+ messages in thread From: Wei Yang @ 2016-12-21 14:39 UTC (permalink / raw) To: Michal Hocko; +Cc: Wei Yang, trivial, akpm, linux-mm, linux-kernel On Wed, Dec 21, 2016 at 02:22:01PM +0100, Michal Hocko wrote: >On Wed 21-12-16 13:13:32, Wei Yang wrote: >> On Wed, Dec 21, 2016 at 08:51:16AM +0100, Michal Hocko wrote: >> >On Tue 20-12-16 16:48:23, Wei Yang wrote: >> >> On Mon, Dec 19, 2016 at 04:21:57PM +0100, Michal Hocko wrote: >> >> >On Sun 18-12-16 14:47:50, Wei Yang wrote: >> >> >> memblock_reserve() may fail in case there is not enough regions. >> >> > >> >> >Have you seen this happenning in the real setups or this is a by-review >> >> >driven change? >> >> >> >> This is a by-review driven change. >> >> >> >> >[...] >> >> >> again: >> >> >> alloc = memblock_find_in_range_node(size, align, min_addr, max_addr, >> >> >> nid, flags); >> >> >> - if (alloc) >> >> >> + if (alloc && !memblock_reserve(alloc, size)) >> >> >> goto done; >> > >> >So how exactly does the reserve fail when memblock_find_in_range_node >> >found a suitable range for the given size? >> > >> >> Even memblock_find_in_range_node() gets a suitable range, memblock_reserve() >> still could fail. And the case just happens when memblock can't resize. >> memblock_reserve() reserve a range by adding a range to memblock.reserved. In >> case the memblock.reserved is full and can't resize, this fails. > >Sorry for being dense but what does it mean that the reserved will get >full? Also how probable is such a situation? Is it even real? In other >words does this fix a real or only a theoretical problem? > This is a theoretical problem. While if happens, it is hard to detect. Future allocator will think this range is still available. >Anyway this all should be part of the changelog. Ok, let me add this in changelog in next version. >-- >Michal Hocko >SUSE Labs -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal() @ 2016-12-21 14:39 ` Wei Yang 0 siblings, 0 replies; 32+ messages in thread From: Wei Yang @ 2016-12-21 14:39 UTC (permalink / raw) To: Michal Hocko; +Cc: Wei Yang, trivial, akpm, linux-mm, linux-kernel On Wed, Dec 21, 2016 at 02:22:01PM +0100, Michal Hocko wrote: >On Wed 21-12-16 13:13:32, Wei Yang wrote: >> On Wed, Dec 21, 2016 at 08:51:16AM +0100, Michal Hocko wrote: >> >On Tue 20-12-16 16:48:23, Wei Yang wrote: >> >> On Mon, Dec 19, 2016 at 04:21:57PM +0100, Michal Hocko wrote: >> >> >On Sun 18-12-16 14:47:50, Wei Yang wrote: >> >> >> memblock_reserve() may fail in case there is not enough regions. >> >> > >> >> >Have you seen this happenning in the real setups or this is a by-review >> >> >driven change? >> >> >> >> This is a by-review driven change. >> >> >> >> >[...] >> >> >> again: >> >> >> alloc = memblock_find_in_range_node(size, align, min_addr, max_addr, >> >> >> nid, flags); >> >> >> - if (alloc) >> >> >> + if (alloc && !memblock_reserve(alloc, size)) >> >> >> goto done; >> > >> >So how exactly does the reserve fail when memblock_find_in_range_node >> >found a suitable range for the given size? >> > >> >> Even memblock_find_in_range_node() gets a suitable range, memblock_reserve() >> still could fail. And the case just happens when memblock can't resize. >> memblock_reserve() reserve a range by adding a range to memblock.reserved. In >> case the memblock.reserved is full and can't resize, this fails. > >Sorry for being dense but what does it mean that the reserved will get >full? Also how probable is such a situation? Is it even real? In other >words does this fix a real or only a theoretical problem? > This is a theoretical problem. While if happens, it is hard to detect. Future allocator will think this range is still available. >Anyway this all should be part of the changelog. Ok, let me add this in changelog in next version. >-- >Michal Hocko >SUSE Labs -- Wei Yang Help you, Help me -- 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] 32+ messages in thread
* Re: [PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal() 2016-12-21 14:39 ` Wei Yang @ 2016-12-21 14:52 ` Michal Hocko -1 siblings, 0 replies; 32+ messages in thread From: Michal Hocko @ 2016-12-21 14:52 UTC (permalink / raw) To: Wei Yang; +Cc: trivial, akpm, linux-mm, linux-kernel On Wed 21-12-16 14:39:56, Wei Yang wrote: > On Wed, Dec 21, 2016 at 02:22:01PM +0100, Michal Hocko wrote: [...] > >Anyway this all should be part of the changelog. > > Ok, let me add this in changelog in next version. Then make sure to document how it could happen and how realistic such a scenario is. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal() @ 2016-12-21 14:52 ` Michal Hocko 0 siblings, 0 replies; 32+ messages in thread From: Michal Hocko @ 2016-12-21 14:52 UTC (permalink / raw) To: Wei Yang; +Cc: trivial, akpm, linux-mm, linux-kernel On Wed 21-12-16 14:39:56, Wei Yang wrote: > On Wed, Dec 21, 2016 at 02:22:01PM +0100, Michal Hocko wrote: [...] > >Anyway this all should be part of the changelog. > > Ok, let me add this in changelog in next version. Then make sure to document how it could happen and how realistic such a scenario is. -- 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] 32+ messages in thread
end of thread, other threads:[~2016-12-21 14:52 UTC | newest] Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-12-18 14:47 [PATCH V2 0/2] mm/memblock.c: fix potential bug and code refine Wei Yang 2016-12-18 14:47 ` Wei Yang 2016-12-18 14:47 ` [PATCH V2 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory() Wei Yang 2016-12-18 14:47 ` Wei Yang 2016-12-19 15:15 ` Michal Hocko 2016-12-19 15:15 ` Michal Hocko 2016-12-20 16:35 ` Wei Yang 2016-12-20 16:35 ` Wei Yang 2016-12-21 7:48 ` Michal Hocko 2016-12-21 7:48 ` Michal Hocko 2016-12-21 12:43 ` Wei Yang 2016-12-21 12:43 ` Wei Yang 2016-12-21 12:48 ` Michal Hocko 2016-12-21 12:48 ` Michal Hocko 2016-12-21 13:15 ` Wei Yang 2016-12-21 13:15 ` Wei Yang 2016-12-18 14:47 ` [PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal() Wei Yang 2016-12-18 14:47 ` Wei Yang 2016-12-19 15:21 ` Michal Hocko 2016-12-19 15:21 ` Michal Hocko 2016-12-20 16:48 ` Wei Yang 2016-12-20 16:48 ` Wei Yang 2016-12-21 7:51 ` Michal Hocko 2016-12-21 7:51 ` Michal Hocko 2016-12-21 13:13 ` Wei Yang 2016-12-21 13:13 ` Wei Yang 2016-12-21 13:22 ` Michal Hocko 2016-12-21 13:22 ` Michal Hocko 2016-12-21 14:39 ` Wei Yang 2016-12-21 14:39 ` Wei Yang 2016-12-21 14:52 ` Michal Hocko 2016-12-21 14:52 ` Michal Hocko
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.