[PATCH V2 0/2] mm/memblock.c: fix potential bug and code refine

[PATCH V2 0/2] mm/memblock.c: fix potential bug and code refine

[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

[PATCH V2 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory()

[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

Re: [PATCH V2 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory()

[Michal Hocko]

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

Re: [PATCH V2 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory()

[Wei Yang]

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

Re: [PATCH V2 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory()

[Michal Hocko]

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

Re: [PATCH V2 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory()

[Wei Yang]

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

Re: [PATCH V2 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory()

[Michal Hocko]

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

Re: [PATCH V2 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory()

[Wei Yang]

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

[PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal()

[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

Re: [PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal()

[Michal Hocko]

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

Re: [PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal()

[Wei Yang]

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

Re: [PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal()

[Michal Hocko]

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

Re: [PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal()

[Wei Yang]

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

Re: [PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal()

[Michal Hocko]

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

Re: [PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal()

[Wei Yang]

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

Re: [PATCH V2 2/2] mm/memblock.c: check return value of memblock_reserve() in memblock_virt_alloc_internal()

[Michal Hocko]

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