[PATCH] mem-hotplug: Don't clear the only node in new_node_page()

[PATCH] mem-hotplug: Don't clear the only node in new_node_page()

[Li Zhong]

Commit 394e31d2c introduced new_node_page() for memory hotplug. 

In new_node_page(), the nid is cleared before calling __alloc_pages_nodemask().
But if it is the only node of the system, and the first round allocation fails,
it will not be able to get memory from an empty nodemask, and trigger oom. 

The patch checks whether it is the last node on the system, and if it is, then
don't clear the nid in the nodemask.

Reported-by: John Allen <jallen@linux.vnet.ibm.com>
Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
---
 mm/memory_hotplug.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 41266dc..b58906b 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1567,7 +1567,9 @@ static struct page *new_node_page(struct page *page, unsigned long private,
 		return alloc_huge_page_node(page_hstate(compound_head(page)),
 					next_node_in(nid, nmask));
 
-	node_clear(nid, nmask);
+	if (nid != next_node_in(nid, nmask))
+		node_clear(nid, nmask);
+
 	if (PageHighMem(page)
 	    || (zone_idx(page_zone(page)) == ZONE_MOVABLE))
 		gfp_mask |= __GFP_HIGHMEM;



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

Re: [PATCH] mem-hotplug: Don't clear the only node in new_node_page()

[Vlastimil Babka]

On 09/05/2016 04:59 AM, Li Zhong wrote:
> Commit 394e31d2c introduced new_node_page() for memory hotplug.
>
> In new_node_page(), the nid is cleared before calling __alloc_pages_nodemask().
> But if it is the only node of the system,

So the use case is that we are partially offlining the only online node?

> and the first round allocation fails,
> it will not be able to get memory from an empty nodemask, and trigger oom.

Hmm triggering OOM due to empty nodemask sounds like a wrong thing to do. CCing 
some OOM experts for insight. Also OOM is skipped for __GFP_THISNODE 
allocations, so we might also consider the same for nodemask-constrained 
allocations?

> The patch checks whether it is the last node on the system, and if it is, then
> don't clear the nid in the nodemask.

I'd rather see the allocation not OOM, and rely on the fallback in 
new_node_page() that doesn't have nodemask. But I suspect it might also make 
sense to treat empty nodemask as something unexpected and put some WARN_ON 
(instead of OOM) in the allocator.

> Reported-by: John Allen <jallen@linux.vnet.ibm.com>
> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz>
Fixes: 394e31d2ceb4 ("mem-hotplug: alloc new page from a nearest neighbor node 
when mem-offline")

> ---
>  mm/memory_hotplug.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 41266dc..b58906b 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1567,7 +1567,9 @@ static struct page *new_node_page(struct page *page, unsigned long private,
>  		return alloc_huge_page_node(page_hstate(compound_head(page)),
>  					next_node_in(nid, nmask));
>
> -	node_clear(nid, nmask);
> +	if (nid != next_node_in(nid, nmask))
> +		node_clear(nid, nmask);
> +
>  	if (PageHighMem(page)
>  	    || (zone_idx(page_zone(page)) == ZONE_MOVABLE))
>  		gfp_mask |= __GFP_HIGHMEM;
>
>
>

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

Re: [PATCH] mem-hotplug: Don't clear the only node in new_node_page()

[Li Zhong]

> On Sep 5, 2016, at 22:18, Vlastimil Babka <vbabka@suse.cz> wrote:
> 
> On 09/05/2016 04:59 AM, Li Zhong wrote:
>> Commit 394e31d2c introduced new_node_page() for memory hotplug.
>> 
>> In new_node_page(), the nid is cleared before calling __alloc_pages_nodemask().
>> But if it is the only node of the system,
> 
> So the use case is that we are partially offlining the only online node?

Yes.
> 
>> and the first round allocation fails,
>> it will not be able to get memory from an empty nodemask, and trigger oom.
> 
> Hmm triggering OOM due to empty nodemask sounds like a wrong thing to do. CCing some OOM experts for insight. Also OOM is skipped for __GFP_THISNODE allocations, so we might also consider the same for nodemask-constrained allocations?
> 
>> The patch checks whether it is the last node on the system, and if it is, then
>> don't clear the nid in the nodemask.
> 
> I'd rather see the allocation not OOM, and rely on the fallback in new_node_page() that doesn't have nodemask. But I suspect it might also make sense to treat empty nodemask as something unexpected and put some WARN_ON (instead of OOM) in the allocator.

I think it would be much easier to understand these kind of empty nodemask allocation failure with this WARN_ON(), how about something like this?

===
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index a2214c6..57edf18 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3629,6 +3629,11 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
                .migratetype = gfpflags_to_migratetype(gfp_mask),
        };
 
+       if (nodemask && nodes_empty(*nodemask)) {
+               WARN_ON(1);
+               return NULL;
+       }
+
        if (cpusets_enabled()) {
                alloc_mask |= __GFP_HARDWALL;
                alloc_flags |= ALLOC_CPUSET;
===

If that’s ok, maybe I can send a separate patch for this? 

Thanks, Zhong

> 
>> Reported-by: John Allen <jallen@linux.vnet.ibm.com>
>> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
> 
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
> Fixes: 394e31d2ceb4 ("mem-hotplug: alloc new page from a nearest neighbor node when mem-offline")
> 
>> ---
>> mm/memory_hotplug.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 41266dc..b58906b 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -1567,7 +1567,9 @@ static struct page *new_node_page(struct page *page, unsigned long private,
>> 		return alloc_huge_page_node(page_hstate(compound_head(page)),
>> 					next_node_in(nid, nmask));
>> 
>> -	node_clear(nid, nmask);
>> +	if (nid != next_node_in(nid, nmask))
>> +		node_clear(nid, nmask);
>> +
>> 	if (PageHighMem(page)
>> 	    || (zone_idx(page_zone(page)) == ZONE_MOVABLE))
>> 		gfp_mask |= __GFP_HIGHMEM;
>> 
>> 
>> 
> 

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

Re: [PATCH] mem-hotplug: Don't clear the only node in new_node_page()

[Xishi Qiu]

On 2016/9/5 10:59, Li Zhong wrote:

> Commit 394e31d2c introduced new_node_page() for memory hotplug. 
> 
> In new_node_page(), the nid is cleared before calling __alloc_pages_nodemask().
> But if it is the only node of the system, and the first round allocation fails,
> it will not be able to get memory from an empty nodemask, and trigger oom. 
> 

Hi,

Yes, I missed this case, thanks for your fix.

Thanks,
Xishi Qiu

> The patch checks whether it is the last node on the system, and if it is, then
> don't clear the nid in the nodemask.
> 
> Reported-by: John Allen <jallen@linux.vnet.ibm.com>
> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>




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

Re: [PATCH] mem-hotplug: Don't clear the only node in new_node_page()

[Vlastimil Babka]

On 09/06/2016 10:13 AM, Li Zhong wrote:
>
>> On Sep 5, 2016, at 22:18, Vlastimil Babka <vbabka@suse.cz> wrote:
>>
>> On 09/05/2016 04:59 AM, Li Zhong wrote:
>>> Commit 394e31d2c introduced new_node_page() for memory hotplug.
>>>
>>> In new_node_page(), the nid is cleared before calling __alloc_pages_nodemask().
>>> But if it is the only node of the system,
>>
>> So the use case is that we are partially offlining the only online node?
>
> Yes.
>>
>>> and the first round allocation fails,
>>> it will not be able to get memory from an empty nodemask, and trigger oom.
>>
>> Hmm triggering OOM due to empty nodemask sounds like a wrong thing to do. CCing some OOM experts for insight. Also OOM is skipped for __GFP_THISNODE allocations, so we might also consider the same for nodemask-constrained allocations?
>>
>>> The patch checks whether it is the last node on the system, and if it is, then
>>> don't clear the nid in the nodemask.
>>
>> I'd rather see the allocation not OOM, and rely on the fallback in new_node_page() that doesn't have nodemask. But I suspect it might also make sense to treat empty nodemask as something unexpected and put some WARN_ON (instead of OOM) in the allocator.
>
> I think it would be much easier to understand these kind of empty nodemask allocation failure with this WARN_ON(), how about something like this?
>
> ===
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index a2214c6..57edf18 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3629,6 +3629,11 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
>                 .migratetype = gfpflags_to_migratetype(gfp_mask),
>         };
>
> +       if (nodemask && nodes_empty(*nodemask)) {
> +               WARN_ON(1);
> +               return NULL;
> +       }
> +
>         if (cpusets_enabled()) {
>                 alloc_mask |= __GFP_HARDWALL;
>                 alloc_flags |= ALLOC_CPUSET;
> ===
>
> If thata??s ok, maybe I can send a separate patch for this?

Something like that, but please not in the hotpath. I think the earliest 
suitable place is in __alloc_pages_slowpath() after the 
get_page_from_freelist() fails. And probably the best way would be to do 
something like pr_warn("nodemask is empty") and then clear __GFP_NOWARN 
from gfp_mask and goto nopage.

Thanks, Vlastimil

> Thanks, Zhong
>
>>
>>> Reported-by: John Allen <jallen@linux.vnet.ibm.com>
>>> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
>>
>> Acked-by: Vlastimil Babka <vbabka@suse.cz>
>> Fixes: 394e31d2ceb4 ("mem-hotplug: alloc new page from a nearest neighbor node when mem-offline")
>>
>>> ---
>>> mm/memory_hotplug.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>> index 41266dc..b58906b 100644
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -1567,7 +1567,9 @@ static struct page *new_node_page(struct page *page, unsigned long private,
>>> 		return alloc_huge_page_node(page_hstate(compound_head(page)),
>>> 					next_node_in(nid, nmask));
>>>
>>> -	node_clear(nid, nmask);
>>> +	if (nid != next_node_in(nid, nmask))
>>> +		node_clear(nid, nmask);
>>> +
>>> 	if (PageHighMem(page)
>>> 	    || (zone_idx(page_zone(page)) == ZONE_MOVABLE))
>>> 		gfp_mask |= __GFP_HIGHMEM;
>>>
>>>
>>>
>>
>

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

[PATCH] mm, page_alloc: warn about empty nodemask

[Li Zhong]

Warn about allocating with an empty nodemask, it would be easier to
understand than oom messages. The check is added in the slow path.

Suggested-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
--- 
mm/page_alloc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index a2214c6..d624ff3 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3448,6 +3448,12 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	if (page)
 		goto got_pg;
 
+	if (ac->nodemask && nodes_empty(*ac->nodemask)) {
+		pr_warn("nodemask is empty\n");
+		gfp_mask &= ~__GFP_NOWARN;
+		goto nopage;
+	}
+
 	/*
 	 * For costly allocations, try direct compaction first, as it's likely
 	 * that we have enough base pages and don't need to reclaim. Don't try


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

Re: [PATCH] mm, page_alloc: warn about empty nodemask

[Andrew Morton]

On Wed, 07 Sep 2016 08:41:26 +0800 Li Zhong <zhong@linux.vnet.ibm.com> wrote:

> Warn about allocating with an empty nodemask, it would be easier to
> understand than oom messages. The check is added in the slow path.
> 
> Suggested-by: Vlastimil Babka <vbabka@suse.cz>
> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
> --- 
> mm/page_alloc.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index a2214c6..d624ff3 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3448,6 +3448,12 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	if (page)
>  		goto got_pg;
>  
> +	if (ac->nodemask && nodes_empty(*ac->nodemask)) {
> +		pr_warn("nodemask is empty\n");
> +		gfp_mask &= ~__GFP_NOWARN;
> +		goto nopage;
> +	}
> +

Wouldn't it be better to do

	if (WARN_ON(ac->nodemask && nodes_empty(*ac->nodemask)) {
		...

so we can identify the misbehaving call site?

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

Re: [PATCH] mm, page_alloc: warn about empty nodemask

[Li Zhong]

> On Sep 9, 2016, at 07:26, Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> On Wed, 07 Sep 2016 08:41:26 +0800 Li Zhong <zhong@linux.vnet.ibm.com> wrote:
> 
>> Warn about allocating with an empty nodemask, it would be easier to
>> understand than oom messages. The check is added in the slow path.
>> 
>> Suggested-by: Vlastimil Babka <vbabka@suse.cz>
>> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
>> --- 
>> mm/page_alloc.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>> 
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index a2214c6..d624ff3 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -3448,6 +3448,12 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>> 	if (page)
>> 		goto got_pg;
>> 
>> +	if (ac->nodemask && nodes_empty(*ac->nodemask)) {
>> +		pr_warn("nodemask is empty\n");
>> +		gfp_mask &= ~__GFP_NOWARN;
>> +		goto nopage;
>> +	}
>> +
> 
> Wouldn't it be better to do
> 
> 	if (WARN_ON(ac->nodemask && nodes_empty(*ac->nodemask)) {
> 		...
> 
> so we can identify the misbehaving call site?

I think with __GFP_NOWARN cleared, we could know the call site from warn_alloc_failed(). 
And the message “nodemask is empty” makes the error obvious without going to the source. 

Thanks, Zhong

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

Re: [PATCH] mem-hotplug: Don't clear the only node in new_node_page()

[Michal Hocko]

On Mon 05-09-16 16:18:29, Vlastimil Babka wrote:
> On 09/05/2016 04:59 AM, Li Zhong wrote:
> > Commit 394e31d2c introduced new_node_page() for memory hotplug.
> > 
> > In new_node_page(), the nid is cleared before calling __alloc_pages_nodemask().
> > But if it is the only node of the system,
> 
> So the use case is that we are partially offlining the only online node?
> 
> > and the first round allocation fails,
> > it will not be able to get memory from an empty nodemask, and trigger oom.
> 
> Hmm triggering OOM due to empty nodemask sounds like a wrong thing to do.
> CCing some OOM experts for insight.

Hmm, to be honest I think that using an empty nodemask is just a bug in
the code. I do not see any reasonable scenario when this would make a
sense. I agree that triggering an OOM killer for that is bad as well but
do we actually want to allow for such a case at all? How can this
happen?

> Also OOM is skipped for __GFP_THISNODE
> allocations, so we might also consider the same for nodemask-constrained
> allocations?
> 
> > The patch checks whether it is the last node on the system, and if it is, then
> > don't clear the nid in the nodemask.
> 
> I'd rather see the allocation not OOM, and rely on the fallback in
> new_node_page() that doesn't have nodemask. But I suspect it might also make
> sense to treat empty nodemask as something unexpected and put some WARN_ON
> (instead of OOM) in the allocator.

To be honest I am really not all that happy about 394e31d2ceb4
("mem-hotplug: alloc new page from a nearest neighbor node when
mem-offline") and find it a bit fishy. I would rather re-iterate that
patch rather than build new hacks on top.
-- 
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>

Re: [PATCH] mm, page_alloc: warn about empty nodemask

[Vlastimil Babka]

On 09/09/2016 06:03 AM, Li Zhong wrote:
>
>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>> index a2214c6..d624ff3 100644
>>> --- a/mm/page_alloc.c
>>> +++ b/mm/page_alloc.c
>>> @@ -3448,6 +3448,12 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>>> 	if (page)
>>> 		goto got_pg;
>>>
>>> +	if (ac->nodemask && nodes_empty(*ac->nodemask)) {
>>> +		pr_warn("nodemask is empty\n");
>>> +		gfp_mask &= ~__GFP_NOWARN;
>>> +		goto nopage;
>>> +	}
>>> +
>>
>> Wouldn't it be better to do
>>
>> 	if (WARN_ON(ac->nodemask && nodes_empty(*ac->nodemask)) {
>> 		...
>>
>> so we can identify the misbehaving call site?
>
> I think with __GFP_NOWARN cleared, we could know the call site from warn_alloc_failed().
> And the message a??nodemask is emptya?? makes the error obvious without going to the source.

Yes, that was my suggestion. It uses the generic warn_alloc_failed() this way. 
With a WARN_ON we would either have to "return NULL" (and get only the WARN_ON 
without the extra warn_alloc_failed() stuff) or "goto nopage" and thus get two 
backtraces. But this should be really rare occurence, so I don't have a 
particularly strong preference.

Anyway, since I suggested it in the first place:
Acked-by: Vlastimil Babka <vbabka@suse.cz>

> Thanks, Zhong
>

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

Re: [PATCH] mem-hotplug: Don't clear the only node in new_node_page()

[Vlastimil Babka]

On 09/12/2016 11:18 AM, Michal Hocko wrote:
> On Mon 05-09-16 16:18:29, Vlastimil Babka wrote:
>
>> Also OOM is skipped for __GFP_THISNODE
>> allocations, so we might also consider the same for nodemask-constrained
>> allocations?
>>
>> > The patch checks whether it is the last node on the system, and if it is, then
>> > don't clear the nid in the nodemask.
>>
>> I'd rather see the allocation not OOM, and rely on the fallback in
>> new_node_page() that doesn't have nodemask. But I suspect it might also make
>> sense to treat empty nodemask as something unexpected and put some WARN_ON
>> (instead of OOM) in the allocator.
>
> To be honest I am really not all that happy about 394e31d2ceb4
> ("mem-hotplug: alloc new page from a nearest neighbor node when
> mem-offline") and find it a bit fishy. I would rather re-iterate that
> patch rather than build new hacks on top.

OK, IIRC I suggested the main idea of clearing the current node from nodemask 
and relying on nodelist to get us the other nodes sorted by their distance. 
Which I thought was an easy way to get to the theoretically optimal result. How 
would you rewrite it then? (but note that the fix is already mainline).

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

Re: [PATCH] mem-hotplug: Don't clear the only node in new_node_page()

[David Rientjes]

On Tue, 20 Sep 2016, Vlastimil Babka wrote:

> On 09/12/2016 11:18 AM, Michal Hocko wrote:
> > On Mon 05-09-16 16:18:29, Vlastimil Babka wrote:
> > 
> > > Also OOM is skipped for __GFP_THISNODE
> > > allocations, so we might also consider the same for nodemask-constrained
> > > allocations?
> > > 
> > > > The patch checks whether it is the last node on the system, and if it
> > > is, then
> > > > don't clear the nid in the nodemask.
> > > 
> > > I'd rather see the allocation not OOM, and rely on the fallback in
> > > new_node_page() that doesn't have nodemask. But I suspect it might also
> > > make
> > > sense to treat empty nodemask as something unexpected and put some WARN_ON
> > > (instead of OOM) in the allocator.
> > 
> > To be honest I am really not all that happy about 394e31d2ceb4
> > ("mem-hotplug: alloc new page from a nearest neighbor node when
> > mem-offline") and find it a bit fishy. I would rather re-iterate that
> > patch rather than build new hacks on top.
> 
> OK, IIRC I suggested the main idea of clearing the current node from nodemask
> and relying on nodelist to get us the other nodes sorted by their distance.
> Which I thought was an easy way to get to the theoretically optimal result.
> How would you rewrite it then? (but note that the fix is already mainline).
> 

This is a mess.  Commit 9bb627be47a5 ("mem-hotplug: don't clear the only 
node in new_node_page()") is wrong because it's clearing nid when the next 
node in node_online_map doesn't match.  node_online_map is wrong because 
it includes memoryless nodes.  (Nodes with closest NUMA distance also do 
not need to have adjacent node ids.)

This is all protected by mem_hotplug_begin() and the zonelists will be 
stable.  The solution is to rewrite new_node_page() to work correctly.  
Use node_states[N_MEMORY] as mask, clear page_to_nid(page).  If mask is 
not empty, do

__alloc_pages_nodemask(gfp_mask, 0,
node_zonelist(page_to_nid(page), gfp_mask), &mask) 

and fallback to alloc_page(gfp_mask), which should also be used if the 
mask is empty -- do not try to allocate memory from the empty set of 
nodes.

mm-page_alloc-warn-about-empty-nodemask.patch is a rather ridiculous 
warning to need.  The largest user of a page allocator nodemask is 
mempolicies which makes sure it doesn't pass an empty set.  If it's really 
required, it should at least be unlikely() since the vast majority of 
callers will have ac->nodemask == NULL.

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

Re: [PATCH] mem-hotplug: Don't clear the only node in new_node_page()

[Li Zhong]

> On Sep 21, 2016, at 05:53, David Rientjes <rientjes@google.com> wrote:
> 
> On Tue, 20 Sep 2016, Vlastimil Babka wrote:
> 
>> On 09/12/2016 11:18 AM, Michal Hocko wrote:
>>> On Mon 05-09-16 16:18:29, Vlastimil Babka wrote:
>>> 
>>>> Also OOM is skipped for __GFP_THISNODE
>>>> allocations, so we might also consider the same for nodemask-constrained
>>>> allocations?
>>>> 
>>>>> The patch checks whether it is the last node on the system, and if it
>>>> is, then
>>>>> don't clear the nid in the nodemask.
>>>> 
>>>> I'd rather see the allocation not OOM, and rely on the fallback in
>>>> new_node_page() that doesn't have nodemask. But I suspect it might also
>>>> make
>>>> sense to treat empty nodemask as something unexpected and put some WARN_ON
>>>> (instead of OOM) in the allocator.
>>> 
>>> To be honest I am really not all that happy about 394e31d2ceb4
>>> ("mem-hotplug: alloc new page from a nearest neighbor node when
>>> mem-offline") and find it a bit fishy. I would rather re-iterate that
>>> patch rather than build new hacks on top.
>> 
>> OK, IIRC I suggested the main idea of clearing the current node from nodemask
>> and relying on nodelist to get us the other nodes sorted by their distance.
>> Which I thought was an easy way to get to the theoretically optimal result.
>> How would you rewrite it then? (but note that the fix is already mainline).
>> 
> 
> This is a mess.  Commit 9bb627be47a5 ("mem-hotplug: don't clear the only 
> node in new_node_page()") is wrong because it's clearing nid when the next 
> node in node_online_map doesn't match.  node_online_map is wrong because 
> it includes memoryless nodes.  (Nodes with closest NUMA distance also do 
> not need to have adjacent node ids.)

Thanks for pointing out that, so it is still possible that we are allocating from one
or more memoryless nodes, which is the same as from an empty mask. 

I will try to fix it as you suggested below, test and send it soon. 
 
> 
> This is all protected by mem_hotplug_begin() and the zonelists will be 
> stable.  The solution is to rewrite new_node_page() to work correctly.  
> Use node_states[N_MEMORY] as mask, clear page_to_nid(page).  If mask is 
> not empty, do
> 
> __alloc_pages_nodemask(gfp_mask, 0,
> node_zonelist(page_to_nid(page), gfp_mask), &mask) 
> 
> and fallback to alloc_page(gfp_mask), which should also be used if the 
> mask is empty -- do not try to allocate memory from the empty set of 
> nodes.
> 
> mm-page_alloc-warn-about-empty-nodemask.patch is a rather ridiculous 
> warning to need.  The largest user of a page allocator nodemask is 
> mempolicies which makes sure it doesn't pass an empty set.  If it's really 
> required, it should at least be unlikely() since the vast majority of 
> callers will have ac->nodemask == NULL.
> 
OK, I’ll send a new version adding unlikely().

Thanks, Zhong

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

[PATCH] mem-hotplug: Use nodes that contain memory as mask in new_node_page()

[Li Zhong]

Commit 9bb627be47a5 ("mem-hotplug: don't clear the only node in
new_node_page()") prevents allocating from an empty nodemask, but as David
points out, it is still wrong. As node_online_map may include memoryless
nodes, only allocating from these nodes is meaningless.

This patch uses node_states[N_MEMORY] mask to prevent the above case.

Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
---
 mm/memory_hotplug.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index b58906b..9d29ba0 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1555,8 +1555,8 @@ static struct page *new_node_page(struct page *page, unsigned long private,
 {
 	gfp_t gfp_mask = GFP_USER | __GFP_MOVABLE;
 	int nid = page_to_nid(page);
-	nodemask_t nmask = node_online_map;
-	struct page *new_page;
+	nodemask_t nmask = node_states[N_MEMORY];
+	struct page *new_page = NULL;
 
 	/*
 	 * TODO: allocate a destination hugepage from a nearest neighbor node,
@@ -1567,14 +1567,14 @@ static struct page *new_node_page(struct page *page, unsigned long private,
 		return alloc_huge_page_node(page_hstate(compound_head(page)),
 					next_node_in(nid, nmask));
 
-	if (nid != next_node_in(nid, nmask))
-		node_clear(nid, nmask);
+	node_clear(nid, nmask);
 
 	if (PageHighMem(page)
 	    || (zone_idx(page_zone(page)) == ZONE_MOVABLE))
 		gfp_mask |= __GFP_HIGHMEM;
 
-	new_page = __alloc_pages_nodemask(gfp_mask, 0,
+	if (!nodes_empty(nmask))
+		new_page = __alloc_pages_nodemask(gfp_mask, 0,
 					node_zonelist(nid, gfp_mask), &nmask);
 	if (!new_page)
 		new_page = __alloc_pages(gfp_mask, 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>

Re: [PATCH] mem-hotplug: Use nodes that contain memory as mask in new_node_page()

[Vlastimil Babka]

On 09/21/2016 10:38 AM, Li Zhong wrote:
> Commit 9bb627be47a5 ("mem-hotplug: don't clear the only node in
> new_node_page()") prevents allocating from an empty nodemask, but as David
> points out, it is still wrong. As node_online_map may include memoryless
> nodes, only allocating from these nodes is meaningless.

Right, those pesky memoryless nodes...

> This patch uses node_states[N_MEMORY] mask to prevent the above case.

Suggested-by: David Rientjes <rientjes@google.com>

> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>  mm/memory_hotplug.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index b58906b..9d29ba0 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1555,8 +1555,8 @@ static struct page *new_node_page(struct page *page, unsigned long private,
>  {
>  	gfp_t gfp_mask = GFP_USER | __GFP_MOVABLE;
>  	int nid = page_to_nid(page);
> -	nodemask_t nmask = node_online_map;
> -	struct page *new_page;
> +	nodemask_t nmask = node_states[N_MEMORY];
> +	struct page *new_page = NULL;
>
>  	/*
>  	 * TODO: allocate a destination hugepage from a nearest neighbor node,
> @@ -1567,14 +1567,14 @@ static struct page *new_node_page(struct page *page, unsigned long private,
>  		return alloc_huge_page_node(page_hstate(compound_head(page)),
>  					next_node_in(nid, nmask));
>
> -	if (nid != next_node_in(nid, nmask))
> -		node_clear(nid, nmask);
> +	node_clear(nid, nmask);
>
>  	if (PageHighMem(page)
>  	    || (zone_idx(page_zone(page)) == ZONE_MOVABLE))
>  		gfp_mask |= __GFP_HIGHMEM;
>
> -	new_page = __alloc_pages_nodemask(gfp_mask, 0,
> +	if (!nodes_empty(nmask))
> +		new_page = __alloc_pages_nodemask(gfp_mask, 0,
>  					node_zonelist(nid, gfp_mask), &nmask);
>  	if (!new_page)
>  		new_page = __alloc_pages(gfp_mask, 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>

Re: [PATCH] mem-hotplug: Don't clear the only node in new_node_page()

[Michal Hocko]

On Tue 20-09-16 14:53:32, David Rientjes wrote:
> On Tue, 20 Sep 2016, Vlastimil Babka wrote:
> 
> > On 09/12/2016 11:18 AM, Michal Hocko wrote:
> > > On Mon 05-09-16 16:18:29, Vlastimil Babka wrote:
> > > 
> > > > Also OOM is skipped for __GFP_THISNODE
> > > > allocations, so we might also consider the same for nodemask-constrained
> > > > allocations?
> > > > 
> > > > > The patch checks whether it is the last node on the system, and if it
> > > > is, then
> > > > > don't clear the nid in the nodemask.
> > > > 
> > > > I'd rather see the allocation not OOM, and rely on the fallback in
> > > > new_node_page() that doesn't have nodemask. But I suspect it might also
> > > > make
> > > > sense to treat empty nodemask as something unexpected and put some WARN_ON
> > > > (instead of OOM) in the allocator.
> > > 
> > > To be honest I am really not all that happy about 394e31d2ceb4
> > > ("mem-hotplug: alloc new page from a nearest neighbor node when
> > > mem-offline") and find it a bit fishy. I would rather re-iterate that
> > > patch rather than build new hacks on top.
> > 
> > OK, IIRC I suggested the main idea of clearing the current node from nodemask
> > and relying on nodelist to get us the other nodes sorted by their distance.
> > Which I thought was an easy way to get to the theoretically optimal result.
> > How would you rewrite it then? (but note that the fix is already mainline).
> > 
> 
> This is a mess.  Commit 9bb627be47a5 ("mem-hotplug: don't clear the only 
> node in new_node_page()") is wrong because it's clearing nid when the next 
> node in node_online_map doesn't match.  node_online_map is wrong because 
> it includes memoryless nodes.  (Nodes with closest NUMA distance also do 
> not need to have adjacent node ids.)
> 
> This is all protected by mem_hotplug_begin() and the zonelists will be 
> stable.  The solution is to rewrite new_node_page() to work correctly.  
> Use node_states[N_MEMORY] as mask, clear page_to_nid(page).  If mask is 
> not empty, do
> 
> __alloc_pages_nodemask(gfp_mask, 0,
> node_zonelist(page_to_nid(page), gfp_mask), &mask) 
> 
> and fallback to alloc_page(gfp_mask), which should also be used if the 
> mask is empty -- do not try to allocate memory from the empty set of 
> nodes.
> 
> mm-page_alloc-warn-about-empty-nodemask.patch is a rather ridiculous 
> warning to need.  The largest user of a page allocator nodemask is 
> mempolicies which makes sure it doesn't pass an empty set.  If it's really 
> required, it should at least be unlikely() since the vast majority of 
> callers will have ac->nodemask == NULL.

Sorry to respond late, I was too busy with other thigns but I completely
agree with the above. This is the way we should go forward!

Thanks!

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

Re: [PATCH] mem-hotplug: Use nodes that contain memory as mask in new_node_page()

[Michal Hocko]

On Wed 21-09-16 16:38:37, Li Zhong wrote:
> Commit 9bb627be47a5 ("mem-hotplug: don't clear the only node in
> new_node_page()") prevents allocating from an empty nodemask, but as David
> points out, it is still wrong. As node_online_map may include memoryless
> nodes, only allocating from these nodes is meaningless.
> 
> This patch uses node_states[N_MEMORY] mask to prevent the above case.
> 

OK, this is much better

I believe this should have

Fixes: 9bb627be47a5 ("mem-hotplug: don't clear the only node in new_node_page()")
Fixes: 394e31d2ceb4 ("mem-hotplug: alloc new page from a nearest neighbor node when mem-offline")

Fortunatelly everything is 4.8 so we do not need any backporting to
stable. Anyway this just shows that the hotplug patches need much better
review. E.g. 394e31d2ceb4 didn't have a single ack or r-b. Sigh

> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
> ---
>  mm/memory_hotplug.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index b58906b..9d29ba0 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1555,8 +1555,8 @@ static struct page *new_node_page(struct page *page, unsigned long private,
>  {
>  	gfp_t gfp_mask = GFP_USER | __GFP_MOVABLE;
>  	int nid = page_to_nid(page);
> -	nodemask_t nmask = node_online_map;
> -	struct page *new_page;
> +	nodemask_t nmask = node_states[N_MEMORY];
> +	struct page *new_page = NULL;
>  
>  	/*
>  	 * TODO: allocate a destination hugepage from a nearest neighbor node,
> @@ -1567,14 +1567,14 @@ static struct page *new_node_page(struct page *page, unsigned long private,
>  		return alloc_huge_page_node(page_hstate(compound_head(page)),
>  					next_node_in(nid, nmask));
>  
> -	if (nid != next_node_in(nid, nmask))
> -		node_clear(nid, nmask);
> +	node_clear(nid, nmask);
>  
>  	if (PageHighMem(page)
>  	    || (zone_idx(page_zone(page)) == ZONE_MOVABLE))
>  		gfp_mask |= __GFP_HIGHMEM;
>  
> -	new_page = __alloc_pages_nodemask(gfp_mask, 0,
> +	if (!nodes_empty(nmask))
> +		new_page = __alloc_pages_nodemask(gfp_mask, 0,
>  					node_zonelist(nid, gfp_mask), &nmask);
>  	if (!new_page)
>  		new_page = __alloc_pages(gfp_mask, 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>