mempolicy:add GFP_THISNODE when allocing new page

In funtion migrate_pages(), if the dest node have no enough free pages,it will fallback to other nodes. Add GFP_THISNODE to avoid this, the same as what funtion new_page_node() do in migrate.c.

Signed-off-by Bob Liu <[email protected]>

Quoted 6 lines · Bob Liu

In funtion migrate_pages(), if the dest node have no enough free pages,it will fallback to other nodes. Add GFP_THISNODE to avoid this, the same as what funtion new_page_node() do in migrate.c.

Signed-off-by Bob Liu <[email protected]>

Yes. It can be fixed. but I have a different concern.

I looked at 6484eb3e2a81807722c5f28ef.
" page allocator: do not check NUMA node ID when the caller knows
the node is valid

Callers of alloc_pages_node() can optionally specify -1 as a node to mean "allocate from the current node". However, a number of the callers in fast paths know for a fact their node is valid. To avoid a comparison
and
branch, this patch adds alloc_pages_exact_node() that only checks the nid with VM_BUG_ON(). Callers that know their node is valid are then converted."

alloc_pages_exact_node's naming would be not good.
It is not for allocate page from exact node but just for removing check of node's valid.
Some people like me who is poor english could misunderstood it.

How about changing name with following?
/* This function can allocate page to fallback list of node*/ alloc_pages_by_nodeid(...)

And instead of it, let's change alloc_pages_exact_node with following. static inline struct page *alloc_pages_exact_node(...)
{
VM_BUG_ON ..
return __alloc_pages(gfp_mask|__GFP_THISNODE...);
}

I think it's more clear than old.
What do you think about it?

Quoted 38 lines · Minchan Kim

Quoted 6 lines · Bob Liu

In funtion migrate_pages(), if the dest node have no enough free pages,it will fallback to other nodes. Add GFP_THISNODE to avoid this, the same as what funtion new_page_node() do in migrate.c.

Signed-off-by Bob Liu <[email protected]>

Yes. It can be fixed. but I have a different concern.

I looked at 6484eb3e2a81807722c5f28ef.
"   page allocator: do not check NUMA node ID when the caller knows the node is valid

   Callers of alloc_pages_node() can optionally specify -1 as a node to mean "allocate from the current node".  However, a number of the callers in fast paths know for a fact their node is valid.  To avoid a comparison and
   branch, this patch adds alloc_pages_exact_node() that only checks the nid with VM_BUG_ON().  Callers that know their node is valid are then converted."

alloc_pages_exact_node's naming would be not good.
It is not for allocate page from exact node but just for removing check of node's valid.
Some people like me who is poor english could misunderstood it.

How about changing name with following?
/* This function can allocate page to fallback list of node*/ alloc_pages_by_nodeid(...)

And instead of it, let's change alloc_pages_exact_node with following. static inline struct page *alloc_pages_exact_node(...)
{
 VM_BUG_ON ..
 return __alloc_pages(gfp_mask|__GFP_THISNODE...);
}

Quoted 3 lines

I think it's more clear than old.
What do you think about it?

Hm.. I agree with you, I was also misunderstanding by the name. But let's still waiting for some other reply.

By the way, what about your opinion using GFP_THISNODE or __GFP_THISNODE in __alloc_pages().
I think GFP_THISNODE is ok.

In funtion migrate_pages(), if the dest node have no enough free pages,it will fallback to other nodes. Add GFP_THISNODE to avoid this, the same as what funtion new_page_node() do in migrate.c.

Signed-off-by Bob Liu <[email protected]>

---
 mm/mempolicy.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

mm/mempolicy.c +0 -0

index 08f40a2..fc5ddf5 100644

Quoted 16 lines · Bob Liu

In funtion migrate_pages(), if the dest node have no enough free pages,it will fallback to other nodes. Add GFP_THISNODE to avoid this, the same as what funtion new_page_node() do in migrate.c.

Signed-off-by Bob Liu <[email protected]>

---
 mm/mempolicy.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

mm/mempolicy.c +0 -0

index 08f40a2..fc5ddf5 100644
@@ -842,7 +842,8 @@ static void migrate_page_add(struct page *page, struct list_head *pagelist,
 

mm/mempolicy.c static void migrate_page_add(struct page *page, struct list_head *pagelist, · line 848

Quoted 7 lines

mm/mempolicy.c +2 -1

 static struct page *new_node_page(struct page *page, unsigned long node, int **x)
 {
-       return alloc_pages_exact_node(node, GFP_HIGHUSER_MOVABLE, 0);
+       return alloc_pages_exact_node(node,
+                               GFP_HIGHUSER_MOVABLE | GFP_THISNODE, 0);
 }
 

Hi, Minchan and Kame
Would you please add ack or review to this thread. It's BUGFIX
and not change, so i don't resend one.

About code clean, there should be some new CLEANUP patches or
just don't make any changes decided after we finish before discussions.

Quoted 17 lines · Bob Liu

Quoted 16 lines · Bob Liu

In funtion migrate_pages(), if the dest node have no enough free pages,it will fallback to other nodes. Add GFP_THISNODE to avoid this, the same as what funtion new_page_node() do in migrate.c.

Signed-off-by Bob Liu <[email protected]>

---
 mm/mempolicy.c |    3 ++-

 1 files changed, 2 insertions(+), 1 deletions(-)

mm/mempolicy.c +0 -0

index 08f40a2..fc5ddf5 100644
@@ -842,7 +842,8 @@ static void migrate_page_add(struct page *page, struct list_head *pagelist,
 

Quoted 12 lines

mm/mempolicy.c static void migrate_page_add(struct page *page, struct list_head *pagelist, · line 848

Quoted 7 lines

mm/mempolicy.c +2 -1

  static struct page *new_node_page(struct page *page, unsigned long node, int **x)
  {
-       return alloc_pages_exact_node(node, GFP_HIGHUSER_MOVABLE, 0);
+       return alloc_pages_exact_node(node,
+                               GFP_HIGHUSER_MOVABLE | GFP_THISNODE, 0);
  }
 

Hi, Minchan and Kame
     Would you please add ack or review to this thread. It's BUGFIX and not change, so i don't resend one.

Sorry for taking so long to get around to this thread. I talked on this patch already but it's in another thread. Here is what I said there

====
This appears to be a valid bug fix.  I agree that the way things are structured that __GFP_THISNODE should be used in new_node_page(). But maybe a follow-on patch is also required. The behaviour is now;

o new_node_page will not return NULL if the target node is empty (fine).
o migrate_pages will translate this into -ENOMEM (fine)
o do_migrate_pages breaks early if it gets -ENOMEM ?

It's the last part I'd like you to double check. migrate_pages() takes a nodemask of allowed nodes to migrate to. Rather than sending this down to the allocator, it iterates over the nodes allowed in the mask. If one of those nodes is full, it returns -ENOMEM.

If -ENOMEM is returned from migrate_pages, should it not move to the next node?
====

My concern before acking this patch is that the function might be exiting too early when given a set of nodes. Granted, because __GFP_THISNODE is not specified, it's perfectly possible that migration is currently moving pages to the wrong node which is also very bad.

Quoted 4 lines

     About code clean, there should be some new CLEANUP patches or just don't make any changes decided after we finish before discussions.

Cleanup patches can be sent separately. I might be biased against a function rename but the bugfix is more important.

If move to the next node instead of early return, the relative position of the page to the beginning of the node set will be break;

(BTW:I am still not very clear about the preservation of the relative position of the
page to the beginning of the node set. I think if the user call migrate_pages() with
different count of src and dest nodes, the relative position will also break. eg. if call migrate_pags() from nodes is node(1,2,3) , dest nodes is just node(3).
the current code logical will move pages in node 1, 2 to node 3. this case the relative position is breaked).

Quoted 2 lines · Bob Liu

If move to the next node instead of early return, the relative position of the page to the beginning of the node set will be break;

Right.

Quoted 9 lines

(BTW:I am still not very clear about the preservation of the relative position of the
page to the beginning of the node set. I think if the user call migrate_pages() with
different count of src and dest nodes, the  relative position will also break. eg. if call migrate_pags() from nodes is node(1,2,3) , dest nodes is just node(3).
the current code logical will move pages in node 1, 2 to node 3. this case the relative position is breaked).

But in that case the user has specified that the set of nodes should be compacted during migration and therefore requested what ocurred.

Quoted 5 lines

Quoted 2 lines

If move to the next node instead of early return, the relative position of the page to the beginning of the node set will be break;

Right.

Thanks!
Then would you please acking this patch? So as mel.

Quoted 43 lines · Mel Gorman

Quoted 17 lines · Bob Liu

Quoted 16 lines · Bob Liu

In funtion migrate_pages(), if the dest node have no enough free pages,it will fallback to other nodes. Add GFP_THISNODE to avoid this, the same as what funtion new_page_node() do in migrate.c.

Signed-off-by Bob Liu <[email protected]>

---
 mm/mempolicy.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

mm/mempolicy.c +0 -0

index 08f40a2..fc5ddf5 100644
@@ -842,7 +842,8 @@ static void migrate_page_add(struct page *page, struct list_head *pagelist,
 

Quoted 12 lines

mm/mempolicy.c static void migrate_page_add(struct page *page, struct list_head *pagelist, · line 848

Quoted 7 lines

mm/mempolicy.c +2 -1

 static struct page *new_node_page(struct page *page, unsigned long node, int **x)
 {
-       return alloc_pages_exact_node(node, GFP_HIGHUSER_MOVABLE, 0);
+       return alloc_pages_exact_node(node,
+                               GFP_HIGHUSER_MOVABLE | GFP_THISNODE, 0);
 }
 

Hi, Minchan and Kame
Would you please add ack or review to this thread. It's BUGFIX
and not change, so i don't resend one.

Sorry for taking so long to get around to this thread. I talked on this patch already but it's in another thread. Here is what I said there

====
This appears to be a valid bug fix. I agree that the way things are structured that __GFP_THISNODE should be used in new_node_page(). But maybe a follow-on patch is also required. The behaviour is now;

o new_node_page will not return NULL if the target node is empty (fine).
o migrate_pages will translate this into -ENOMEM (fine)
o do_migrate_pages breaks early if it gets -ENOMEM ?

Quoted 8 lines

It's the last part I'd like you to double check. migrate_pages() takes a nodemask of allowed nodes to migrate to. Rather than sending this down to the allocator, it iterates over the nodes allowed in the mask. If one of those nodes is full, it returns -ENOMEM.

If -ENOMEM is returned from migrate_pages, should it not move to the next node?
====

Hm.I think early return is ok but not sure about it :)

As Christoph said
"The intended semantic is the preservation of the relative position of the page to the beginning of the node set."
"F.e. if you use page
migration (or cpuset automigration) to shift an application running on 10 nodes up by two nodes to make a hole that would allow you to run another application on the lower nodes. Applications place pages intentionally on certain nodes to be able to manage memory distances."

If move to the next node instead of early return, the relative position of the page to the beginning of the node set will be break;

Quoted 64 lines · Bob Liu

Quoted 43 lines · Mel Gorman

Quoted 17 lines · Bob Liu

Quoted 16 lines · Bob Liu

In funtion migrate_pages(), if the dest node have no enough free pages,it will fallback to other nodes. Add GFP_THISNODE to avoid this, the same as what funtion new_page_node() do in migrate.c.

Signed-off-by Bob Liu <[email protected]>

---
 mm/mempolicy.c |    3 ++-

 1 files changed, 2 insertions(+), 1 deletions(-)

mm/mempolicy.c +0 -0

index 08f40a2..fc5ddf5 100644
@@ -842,7 +842,8 @@ static void migrate_page_add(struct page *page, struct list_head *pagelist,
 

Quoted 12 lines

mm/mempolicy.c static void migrate_page_add(struct page *page, struct list_head *pagelist, · line 848

Quoted 7 lines

mm/mempolicy.c +2 -1

  static struct page *new_node_page(struct page *page, unsigned long node, int **x)
  {
-       return alloc_pages_exact_node(node, GFP_HIGHUSER_MOVABLE, 0);
+       return alloc_pages_exact_node(node,
+                               GFP_HIGHUSER_MOVABLE | GFP_THISNODE, 0);
  }
 

Hi, Minchan and Kame
     Would you please add ack or review to this thread. It's BUGFIX and not change, so i don't resend one.

Sorry for taking so long to get around to this thread. I talked on this patch already but it's in another thread. Here is what I said there

====
This appears to be a valid bug fix.  I agree that the way things are structured that __GFP_THISNODE should be used in new_node_page(). But maybe a follow-on patch is also required. The behaviour is now;

o new_node_page will not return NULL if the target node is empty (fine).
o migrate_pages will translate this into -ENOMEM (fine)
o do_migrate_pages breaks early if it gets -ENOMEM ?

Quoted 8 lines

It's the last part I'd like you to double check. migrate_pages() takes a nodemask of allowed nodes to migrate to. Rather than sending this down to the allocator, it iterates over the nodes allowed in the mask. If one of those nodes is full, it returns -ENOMEM.

If -ENOMEM is returned from migrate_pages, should it not move to the next node?
====

Hm.I think early return is ok but not sure about it :)

As Christoph said
"The intended semantic is the preservation of the relative position of the page to the beginning of the node set."
"F.e. if you use page
migration (or cpuset automigration) to shift an application running on 10 nodes up by two nodes to make a hole that would allow you to run another application on the lower nodes. Applications place pages intentionally on certain nodes to be able to manage memory distances."

Quoted 3 lines

If move to the next node instead of early return, the relative position of the page to the beginning of the node set will be break;

Yeah, but the user requested that a number of nodes to be used. Sure, if the first node is not free, a page will be allocated on the second node instead but is that not what was requested? If the user wanted one and only one node to be used, they wouldn't have specified multiple nodes. I'm not convinced an early return is what was intended here.

Hmm.
What about this change? If the from_nodes and to_nodes' weight is different, then we can don't preserv of the relative position of the page to the beginning of the node set. This case if a page allocation from the dest node failed, it will
be allocated from the next node instead of early return.

Quoted 6 lines · Bob Liu

Hmm.
What about this change? If the from_nodes and to_nodes' weight is different, then we can don't preserv of the relative position of the page to the beginning of the node set. This case if a page allocation from the dest node failed, it will
be allocated from the next node instead of early return.

Understand what you are doing first. The fallback is already there.

Quoted 7 lines

mm/mempolicy.c +0 -0

index 08f40a2..094d092 100644
@@ -842,7 +842,8 @@ static void migrate_page_add(struct page *page,
 struct list_head *pagelist,
 

Quoted 6 lines

 static struct page *new_node_page(struct page *page, unsigned long node, int **x)
 {
-       return alloc_pages_exact_node(node, GFP_HIGHUSER_MOVABLE, 0);
+       return alloc_pages_exact_node(node,
+                       GFP_HIGHUSER_MOVABLE | GFP_THISNODE, 0);

You eliminate falling back to the next node?

GFP_THISNODE forces allocation from the node. Without it we will fallback.

Quoted 2 lines

GFP_THISNODE forces allocation from the node. Without it we will fallback.

Yeah, but I think we shouldn't fallback at this case, what we want is alloc a page
from exactly the dest node during migrate_to_node(dest).So I added GFP_THISNODE.

And mel concerned that
====
This appears to be a valid bug fix. I agree that the way things are structured that __GFP_THISNODE should be used in new_node_page(). But maybe a follow-on patch is also required. The behaviour is now;

o new_node_page will not return NULL if the target node is empty (fine).
o migrate_pages will translate this into -ENOMEM (fine)
o do_migrate_pages breaks early if it gets -ENOMEM ?

It's the last part I'd like you to double check. migrate_pages() takes a nodemask of allowed nodes to migrate to. Rather than sending this down to the allocator, it iterates over the nodes allowed in the mask. If one of those nodes is full, it returns -ENOMEM.

If -ENOMEM is returned from migrate_pages, should it not move to the next node?
====

In my opinion, when we want to preserve the relative position of the page to the beginning of the node set, early return is ok. Else should try to alloc the new page from the next node(to_nodes).

So I added retry path to allocate new page from next node only when from_nodes' weight is different from to_nodes', this case the user should konw the relative position of the page to the beginning of the node set can be changed.

Quoted 3 lines · Bob Liu

Quoted 2 lines

GFP_THISNODE forces allocation from the node. Without it we will fallback.

Quoted 4 lines

Yeah, but I think we shouldn't fallback at this case, what we want is alloc a page
from exactly the dest node during migrate_to_node(dest).So I added GFP_THISNODE.

Why would we want that?

Quoted 10 lines · Christoph Lameter

Quoted 7 lines · Bob Liu

Quoted 2 lines

GFP_THISNODE forces allocation from the node. Without it we will fallback.

Yeah, but I think we shouldn't fallback at this case, what we want is alloc a page
from exactly the dest node during migrate_to_node(dest).So I added GFP_THISNODE.

Quoted 2 lines

Why would we want that?

Because if dest node have no memory, it will fallback to other nodes. The dest node's fallback nodes may be nodes in nodemask from_nodes. It maybe make circulation ?.(I am not sure.)

What's more,i think it against the user's request.

The user wants to move pages from from_nodes to to_nodes, if fallback happened, the pages may be moved to other nodes instead of any node in nodemask to_nodes.
I am not sure if the user can expect this and accept.

Quoted 19 lines · Bob Liu

Quoted 5 lines · Christoph Lameter

Quoted 3 lines · Bob Liu

Quoted 2 lines

GFP_THISNODE forces allocation from the node. Without it we will fallback.

Quoted 7 lines

Quoted 4 lines

Yeah, but I think we shouldn't fallback at this case, what we want is alloc a page
from exactly the dest node during migrate_to_node(dest).So I added GFP_THISNODE.

Why would we want that?

Because if dest node have no memory, it will fallback to other nodes. The dest node's fallback nodes may be nodes in nodemask from_nodes. It maybe make circulation ?.(I am not sure.)

Quoted 1 line

What's more,i think it against the user's request.

The problem is your perception of NUMA against the kernel NUMA design. As long as you have this problem I would suggest that you do not submit patches against NUMA functionality in the kernel.

Quoted 4 lines

The user wants to move pages from from_nodes to to_nodes, if fallback happened, the pages may be moved to other nodes instead of any node in nodemask to_nodes.
I am not sure if the user can expect this and accept.

Sure the user always had it this way. NUMA allocations (like also MPOL_INTERLEAVE round robin) are *only* attempts to allocate on specific nodes.

There was never a guarantee (until GFP_THISNODE arrived on the scene to fix SLAB breakage but that was very late in NUMA design of the kernel).

Just one small point, why do_move_pages() in migrate.c needs GFP_THISNODE ?

In funtion migrate_pages(), if the dest node have no enough free pages,it will fallback to other nodes. Add GFP_THISNODE to avoid this, the same as what funtion new_page_node() do in migrate.c.

Signed-off-by Bob Liu <[email protected]>

---
 mm/mempolicy.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

mm/mempolicy.c +2 -1

index 08f40a2..fc5ddf5 100644
@@ -842,7 +842,8 @@ static void migrate_page_add(struct page *page, struct list_head *pagelist,
 
 static struct page *new_node_page(struct page *page, unsigned long node, int **x)
 {
-	return alloc_pages_exact_node(node, GFP_HIGHUSER_MOVABLE, 0);
+	return alloc_pages_exact_node(node,
+				GFP_HIGHUSER_MOVABLE | GFP_THISNODE, 0);
 }
 
 /*

Signature · 7 lines

1.5.6.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

In funtion migrate_pages(), if the dest node have no enough free pages,it will fallback to other nodes. Add GFP_THISNODE to avoid this, the same as what funtion new_page_node() do in migrate.c.

Signed-off-by Bob Liu <[email protected]>