mm/migrate.c: cleanup on do_pages_move()
Subsystems (2)
cover mm/migrate.c: cleanup on do_pages_move()
The logic in do_pages_move() is a little mess for audience to read and has some potential error on handling the return value. Especially there are three calls on do_move_pages_to_node() and store_status() with almost the same form.
This patch set tries to make the code a little friendly for audience by consolidate the calls and remove some unnecessary repeat code.
After this, we can do a better fix.
Wei Yang (8):
mm/migrate.c: skip node check if done in last round mm/migrate.c: not necessary to check start and i mm/migrate.c: reform the last call on do_move_pages_to_node() mm/migrate.c: wrap do_move_pages_to_node() and store_status() mm/migrate.c: check pagelist in move_pages_and_store_status() mm/migrate.c: handle same node and add failure in the same way mm/migrate.c: move page on next iteration mm/migrate.c: use break instead of goto out_flush
mm/migrate.c | 90 ++++++++++++++++++++++++++++------------------------ 1 file changed, 48 insertions(+), 42 deletions(-)
Signature · 1 line
2.17.1
PATCH 1/8 mm/migrate.c: skip node check if done in last round
Before move page to target node, we would check if the node id is valid. In case we would try to move pages to the same target node, it is not necessary to do the check each time.
This patch tries to skip the check if the node has been checked.
--- mm/migrate.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)
mm/migrate.c +11 -8
@@ -1612,15 +1612,18 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, goto out_flush; addr = (unsigned long )untagged_addr (p); - err = -ENODEV ; -if (node <0 || node >=MAX_NUMNODES ) - goto out_flush; -if (!node_state (node,N_MEMORY )) - goto out_flush; +/* Check node if it is not checked. */ +if (current_node ==NUMA_NO_NODE || node != current_node) { + err = -ENODEV ; +if (node <0 || node >=MAX_NUMNODES ) + goto out_flush; +if (!node_state (node,N_MEMORY )) + goto out_flush; - err = -EACCES ; -if (!node_isset (node, task_nodes)) - goto out_flush; + err = -EACCES ; +if (!node_isset (node, task_nodes)) + goto out_flush; + }if (current_node ==NUMA_NO_NODE ) { current_node = node;
Signature · 1 line
2.17.1
Re: [PATCH 1/8] mm/migrate.c: skip node check if done in last round
Quoted 21 lines · Wei Yang
Before move page to target node, we would check if the node id is valid. In case we would try to move pages to the same target node, it is not necessary to do the check each time.
This patch tries to skip the check if the node has been checked.
Signed-off-by Wei Yang <[email protected]>--- mm/migrate.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)mm/migrate.c +0
-2@@ -1612,15 +1612,18 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, goto out_flush; addr = (unsigned long )untagged_addr (p); - err = -ENODEV ; -if (node <0 || node >=MAX_NUMNODES )
mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1621
Quoted 10 lines
mm/migrate.c +7
-3- goto out_flush; -if (!node_state (node,N_MEMORY )) - goto out_flush; +/* Check node if it is not checked. */ +if (current_node ==NUMA_NO_NODE || node != current_node) { + err = -ENODEV ; +if (node <0 || node >=MAX_NUMNODES ) + goto out_flush; +if (!node_state (node,N_MEMORY )) + goto out_flush;
This makes the code harder to read IMHO. The original code checks the valid node first and it doesn't conflate that with the node caching logic which your change does.
Quoted 13 lines
mm/migrate.c +4
-3- err = -EACCES ; -if (!node_isset (node, task_nodes)) - goto out_flush; + err = -EACCES ; +if (!node_isset (node, task_nodes)) + goto out_flush; + }if (current_node ==NUMA_NO_NODE ) { current_node = node;Signature · 1 line
2.17.1
Signature · 2 lines
Michal Hocko SUSE Labs
Re: [PATCH 1/8] mm/migrate.c: skip node check if done in last round
Quoted 20 lines · Michal Hocko
Quoted 19 lines · Wei Yang
Before move page to target node, we would check if the node id is valid. In case we would try to move pages to the same target node, it is not necessary to do the check each time.
This patch tries to skip the check if the node has been checked.
Signed-off-by Wei Yang <[email protected]>--- mm/migrate.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)mm/migrate.c +0
-0@@ -1612,15 +1612,18 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, goto out_flush; addr = (unsigned long )untagged_addr (p);
Quoted 17 lines
mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1621
Quoted 12 lines
mm/migrate.c +7
-5- err = -ENODEV ; -if (node <0 || node >=MAX_NUMNODES ) - goto out_flush; -if (!node_state (node,N_MEMORY )) - goto out_flush; +/* Check node if it is not checked. */ +if (current_node ==NUMA_NO_NODE || node != current_node) { + err = -ENODEV ; +if (node <0 || node >=MAX_NUMNODES ) + goto out_flush; +if (!node_state (node,N_MEMORY )) + goto out_flush;This makes the code harder to read IMHO. The original code checks the valid node first and it doesn't conflate that with the node caching logic which your change does.
I am sorry, would you mind showing me an example about the conflate in my change? I don't get it.
Quoted 17 lines
Quoted 13 lines
mm/migrate.c +4
-3- err = -EACCES ; -if (!node_isset (node, task_nodes)) - goto out_flush; + err = -EACCES ; +if (!node_isset (node, task_nodes)) + goto out_flush; + }if (current_node ==NUMA_NO_NODE ) { current_node = node;Signature · 1 line
2.17.1Signature · 2 lines
Michal Hocko SUSE Labs
Signature · 2 lines
Wei Yang Help you, Help me
Re: [PATCH 1/8] mm/migrate.c: skip node check if done in last round
Quoted 39 lines · Wei Yang
Quoted 20 lines · Michal Hocko
Quoted 19 lines · Wei Yang
Before move page to target node, we would check if the node id is valid. In case we would try to move pages to the same target node, it is not necessary to do the check each time.
This patch tries to skip the check if the node has been checked.
Signed-off-by Wei Yang <[email protected]>--- mm/migrate.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)mm/migrate.c +0
-0@@ -1612,15 +1612,18 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, goto out_flush; addr = (unsigned long )untagged_addr (p);Quoted 17 lines
mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1621
Quoted 12 lines
mm/migrate.c +7
-5- err = -ENODEV ; -if (node <0 || node >=MAX_NUMNODES ) - goto out_flush; -if (!node_state (node,N_MEMORY )) - goto out_flush; +/* Check node if it is not checked. */ +if (current_node ==NUMA_NO_NODE || node != current_node) { + err = -ENODEV ; +if (node <0 || node >=MAX_NUMNODES ) + goto out_flush; +if (!node_state (node,N_MEMORY )) + goto out_flush;This makes the code harder to read IMHO. The original code checks the valid node first and it doesn't conflate that with the node caching logic which your change does.
Quoted 2 lines
I am sorry, would you mind showing me an example about the conflate in my change? I don't get it.
NUMA_NO_NODE is the iteration logic, right? It resets the batching node. Node check read from the userspace is an input sanitization. Do not put those two into the same checks. More clear now?
Signature · 2 lines
Michal Hocko SUSE Labs
Re: [PATCH 1/8] mm/migrate.c: skip node check if done in last round
Quoted 40 lines · Michal Hocko
Quoted 39 lines · Wei Yang
Quoted 37 lines · Michal Hocko
Quoted 21 lines · Wei Yang
Before move page to target node, we would check if the node id is valid. In case we would try to move pages to the same target node, it is not necessary to do the check each time.
This patch tries to skip the check if the node has been checked.
Signed-off-by Wei Yang <[email protected]>--- mm/migrate.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)mm/migrate.c +0
-2@@ -1612,15 +1612,18 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, goto out_flush; addr = (unsigned long )untagged_addr (p); - err = -ENODEV ; -if (node <0 || node >=MAX_NUMNODES )mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1621
Quoted 10 lines
mm/migrate.c +7
-3- goto out_flush; -if (!node_state (node,N_MEMORY )) - goto out_flush; +/* Check node if it is not checked. */ +if (current_node ==NUMA_NO_NODE || node != current_node) { + err = -ENODEV ; +if (node <0 || node >=MAX_NUMNODES ) + goto out_flush; +if (!node_state (node,N_MEMORY )) + goto out_flush;This makes the code harder to read IMHO. The original code checks the valid node first and it doesn't conflate that with the node caching logic which your change does.
Quoted 6 lines
Quoted 2 lines
I am sorry, would you mind showing me an example about the conflate in my change? I don't get it.
NUMA_NO_NODEis the iteration logic, right? It resets the batching node. Node check read from the userspace is an input sanitization. Do not put those two into the same checks. More clear now?
Yes, I see your point.
Can we think like this:
On each iteration, we do an input sanitization?
Well, this is a trivial one. If you don't like it, I would remove this.
Quoted 3 lines
Signature · 2 lines
Michal Hocko SUSE Labs
Signature · 2 lines
Wei Yang Help you, Help me
Re: [PATCH 1/8] mm/migrate.c: skip node check if done in last round
On Wed 22-01-20 08:36:50, Wei Yang wrote:
[...]
Quoted 1 line
Well, this is a trivial one. If you don't like it, I would remove this.
Yes, please drop it. Thanks!
Signature · 2 lines
Michal Hocko SUSE Labs
PATCH 2/8 mm/migrate.c: not necessary to check start and i
In reply to Wei YangTill here, i must no less than start. And if i equals to start, store_status() would always return 0.
Remove some unnecessary check to make it easy to read and prepare for further cleanup.
--- mm/migrate.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
mm/migrate.c +3 -5
@@ -1664,11 +1664,9 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, err =do_move_pages_to_node (mm, &pagelist, current_node);if (err) goto out; -if (i > start) { - err =store_status (status, start, current_node, i - start); -if (err) - goto out; - } + err =store_status (status, start, current_node, i - start); +if (err) + goto out; current_node =NUMA_NO_NODE ; } out_flush:
Signature · 1 line
2.17.1
Re: [PATCH 2/8] mm/migrate.c: not necessary to check start and i
Quoted 9 lines · Wei Yang
mm/migrate.c +0
-1@@ -1664,11 +1664,9 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, err =do_move_pages_to_node (mm, &pagelist, current_node);if (err) goto out; -if (i > start) {
mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1672
Quoted 10 lines
mm/migrate.c +3
-4- err =store_status (status, start, current_node, i - start); -if (err) - goto out; - } + err =store_status (status, start, current_node, i - start); +if (err) + goto out; current_node =NUMA_NO_NODE ; } out_flush:
Not sure this is useful, it relies on the implementation of store_status() when i == start and the overhead of the function call should actually be slower than the simple conditional to avoid it in that case?
Re: [PATCH 2/8] mm/migrate.c: not necessary to check start and i
Quoted 17 lines · David Rientjes
Quoted 15 lines · Wei Yang
mm/migrate.c +2
-5@@ -1664,11 +1664,9 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, err =do_move_pages_to_node (mm, &pagelist, current_node);if (err) goto out; -if (i > start) { - err =store_status (status, start, current_node, i - start); -if (err) - goto out; - } + err =store_status (status, start, current_node, i - start); +if (err)
Quoted 8 lines
mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1672
Quoted 4 lines
mm/migrate.c +1
-0+ goto out; current_node =NUMA_NO_NODE ; } out_flush:Not sure this is useful, it relies on the implementation of
store_status()when i == start and the overhead of the function call should actually be slower than the simple conditional to avoid it in that case?
Not sure about this.
While this patch is a transient state for the following cleanup. The purpose of this is to make the consolidation a little easy to review.
Signature · 2 lines
Wei Yang Help you, Help me
Re: [PATCH 2/8] mm/migrate.c: not necessary to check start and i
In reply to Wei YangQuoted 5 lines · Wei Yang
Till here, i must no less than start. And if i equals to start,
store_status()would always return 0.Remove some unnecessary check to make it easy to read and prepare for further cleanup.
You are right. This is likely a left over from the development. i >= start because the former is the actual iterator and start is the first index with the cached node.
Dropping the check improves readability because one might indeed wonder why this is the only place to do the check and the overal iteration is complex enough to add more questions on top.
Quoted 1 line
Signed-off-by Wei Yang <[email protected]>
Thanks!
Quoted 25 lines
--- mm/migrate.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)mm/migrate.c +3
-5@@ -1664,11 +1664,9 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, err =do_move_pages_to_node (mm, &pagelist, current_node);if (err) goto out; -if (i > start) { - err =store_status (status, start, current_node, i - start); -if (err) - goto out; - } + err =store_status (status, start, current_node, i - start); +if (err) + goto out; current_node =NUMA_NO_NODE ; } out_flush:Signature · 1 line
2.17.1
Signature · 2 lines
Michal Hocko SUSE Labs
PATCH 3/8 mm/migrate.c: reform the last call on do_move_pages_to_node()
In reply to Wei YangNo functional change, just reform it to make it as the same shape as other calls on do_move_pages_to_node().
This is a preparation for further cleanup.
--- mm/migrate.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
mm/migrate.c +6 -2
@@ -1675,8 +1675,12 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,/* Make sure we do not overwrite the existing error */ err1 =do_move_pages_to_node (mm, &pagelist, current_node); -if (!err1) - err1 =store_status (status, start, current_node, i - start); +if (err1) { +if (err >=0 ) + err = err1; + goto out; + } + err1 =store_status (status, start, current_node, i - start);if (err >=0 ) err = err1; out:
Signature · 1 line
2.17.1
Re: [PATCH 3/8] mm/migrate.c: reform the last call on do_move_pages_to_node()
Quoted 16 lines · Wei Yang
No functional change, just reform it to make it as the same shape as other calls on
do_move_pages_to_node().This is a preparation for further cleanup.
Signed-off-by Wei Yang <[email protected]>--- mm/migrate.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)mm/migrate.c +0
-0@@ -1675,8 +1675,12 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1683
Quoted 10 lines
mm/migrate.c +6
-2/* Make sure we do not overwrite the existing error */ err1 =do_move_pages_to_node (mm, &pagelist, current_node); -if (!err1) - err1 =store_status (status, start, current_node, i - start); +if (err1) { +if (err >=0 ) + err = err1; + goto out; + } + err1 =store_status (status, start, current_node, i - start);
Please don't. This just makes the code harder to follow. The current err and err1 is already quite ugly so do not make it more so.
Quoted 5 lines
mm/migrate.c +0
-0if (err >=0 ) err = err1; out:Signature · 1 line
2.17.1
Signature · 2 lines
Michal Hocko SUSE Labs
Re: [PATCH 3/8] mm/migrate.c: reform the last call on do_move_pages_to_node()
Quoted 17 lines · Michal Hocko
Quoted 16 lines · Wei Yang
No functional change, just reform it to make it as the same shape as other calls on
do_move_pages_to_node().This is a preparation for further cleanup.
Signed-off-by Wei Yang <[email protected]>--- mm/migrate.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)mm/migrate.c +0
-0@@ -1675,8 +1675,12 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
Quoted 14 lines
mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1683
Quoted 10 lines
mm/migrate.c +6
-2/* Make sure we do not overwrite the existing error */ err1 =do_move_pages_to_node (mm, &pagelist, current_node); -if (!err1) - err1 =store_status (status, start, current_node, i - start); +if (err1) { +if (err >=0 ) + err = err1; + goto out; + } + err1 =store_status (status, start, current_node, i - start);Please don't. This just makes the code harder to follow. The current err and err1 is already quite ugly so do not make it more so.
Yes, I struggled a little on doing this change. Sounds we can merge this one with the following consolidation.
Quoted 9 lines
Quoted 5 lines
mm/migrate.c +0
-0if (err >=0 ) err = err1; out:Signature · 1 line
2.17.1Signature · 2 lines
Michal Hocko SUSE Labs
Signature · 2 lines
Wei Yang Help you, Help me
PATCH 4/8 mm/migrate.c: wrap do_move_pages_to_node() and store_status()
In reply to Wei YangUsually do_move_pages_to_node() and store_status() is a pair. There are three places call this pair of functions with almost the same form.
This patch just wrap it to make it friendly to audience and also consolidate the move and store action into one place. Also mentioned by Yang Shi, the handling of do_move_pages_to_node()'s return value is not proper. Now we can fix it in one place.
--- mm/migrate.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-)
mm/migrate.c +19 -15
@@ -1583,6 +1583,19 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,return err; } +static int move_pages_and_store_status (struct mm_struct *mm,int node, +struct list_head *pagelist,int __user *status, +int start,int nr) +{ +int err; + + err =do_move_pages_to_node (mm, pagelist, node); +if (err) +return err; + err =store_status (status, start, node, nr); +return err; +} +/* * Migrate an array of page address onto an array of nodes and fill * the corresponding array of status.
@@ -1629,10 +1642,8 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, current_node = node; start = i; }else if (node != current_node) { - err =do_move_pages_to_node (mm, &pagelist, current_node); -if (err) - goto out; - err =store_status (status, start, current_node, i - start); + err =move_pages_and_store_status (mm, current_node, + &pagelist, status, start, i - start);if (err) goto out; start = i;
@@ -1661,10 +1672,8 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,if (err) goto out_flush; - err =do_move_pages_to_node (mm, &pagelist, current_node); -if (err) - goto out; - err =store_status (status, start, current_node, i - start); + err =move_pages_and_store_status (mm, current_node, &pagelist, + status, start, i - start);if (err) goto out; current_node =NUMA_NO_NODE ;
@@ -1674,13 +1683,8 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,return err;/* Make sure we do not overwrite the existing error */ - err1 =do_move_pages_to_node (mm, &pagelist, current_node); -if (err1) { -if (err >=0 ) - err = err1; - goto out; - } - err1 =store_status (status, start, current_node, i - start); + err1 =move_pages_and_store_status (mm, current_node, &pagelist, + status, start, i - start);if (err >=0 ) err = err1; out:
Signature · 1 line
2.17.1
Re: [PATCH 4/8] mm/migrate.c: wrap do_move_pages_to_node() and store_status()
Quoted 7 lines · Wei Yang
Usually
do_move_pages_to_node()andstore_status()is a pair. There are three places call this pair of functions with almost the same form.This patch just wrap it to make it friendly to audience and also consolidate the move and store action into one place. Also mentioned by Yang Shi, the handling of do_move_pages_to_node()'s return value is not proper. Now we can fix it in one place.
The helper helps here indeed. Thanks this makes the code easier to read.
Quoted 2 lines
Signed-off-by Wei Yang <[email protected]>
Quoted 73 lines
--- mm/migrate.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-)mm/migrate.c +19
-15@@ -1583,6 +1583,19 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,return err; } +static int move_pages_and_store_status (struct mm_struct *mm,int node, +struct list_head *pagelist,int __user *status, +int start,int nr) +{ +int err; + + err =do_move_pages_to_node (mm, pagelist, node); +if (err) +return err; + err =store_status (status, start, node, nr); +return err; +} +/* * Migrate an array of page address onto an array of nodes and fill * the corresponding array of status. @@ -1629,10 +1642,8 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, current_node = node; start = i; }else if (node != current_node) { - err =do_move_pages_to_node (mm, &pagelist, current_node); -if (err) - goto out; - err =store_status (status, start, current_node, i - start); + err =move_pages_and_store_status (mm, current_node, + &pagelist, status, start, i - start);if (err) goto out; start = i;@@ -1661,10 +1672,8 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,if (err) goto out_flush; - err =do_move_pages_to_node (mm, &pagelist, current_node); -if (err) - goto out; - err =store_status (status, start, current_node, i - start); + err =move_pages_and_store_status (mm, current_node, &pagelist, + status, start, i - start);if (err) goto out; current_node =NUMA_NO_NODE ;@@ -1674,13 +1683,8 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,return err;/* Make sure we do not overwrite the existing error */ - err1 =do_move_pages_to_node (mm, &pagelist, current_node); -if (err1) { -if (err >=0 ) - err = err1; - goto out; - } - err1 =store_status (status, start, current_node, i - start); + err1 =move_pages_and_store_status (mm, current_node, &pagelist, + status, start, i - start);if (err >=0 ) err = err1; out:Signature · 1 line
2.17.1
Signature · 2 lines
Michal Hocko SUSE Labs
PATCH 5/8 mm/migrate.c: check pagelist in move_pages_and_store_status()
In reply to Wei YangWhen pagelist is empty, it is not necessary to do the move and store. Also it consolidate the empty list check in one place.
--- mm/migrate.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
mm/migrate.c +3 -6
@@ -1499,9 +1499,6 @@ static int do_move_pages_to_node(struct mm_struct *mm, {int err; -if (list_empty (pagelist)) -return 0 ; - err =migrate_pages (pagelist, alloc_new_node_page,NULL , node,MIGRATE_SYNC ,MR_SYSCALL );if (err)
@@ -1589,6 +1586,9 @@ static int move_pages_and_store_status(struct mm_struct *mm, int node, {int err; +if (list_empty (pagelist)) +return 0 ; + err =do_move_pages_to_node (mm, pagelist, node);if (err)return err;
@@ -1679,9 +1679,6 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, current_node =NUMA_NO_NODE ; } out_flush: -if (list_empty (&pagelist)) -return err; -/* Make sure we do not overwrite the existing error */ err1 =move_pages_and_store_status (mm, current_node, &pagelist, status, start, i - start);
Signature · 1 line
2.17.1
Re: [PATCH 5/8] mm/migrate.c: check pagelist in move_pages_and_store_status()
Quoted 2 lines · Wei Yang
When pagelist is empty, it is not necessary to do the move and store. Also it consolidate the empty list check in one place.
OK looks good to me.
Quoted 1 line
Signed-off-by Wei Yang <[email protected]>
Quoted 41 lines
--- mm/migrate.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)mm/migrate.c +3
-6@@ -1499,9 +1499,6 @@ static int do_move_pages_to_node(struct mm_struct *mm, {int err; -if (list_empty (pagelist)) -return 0 ; - err =migrate_pages (pagelist, alloc_new_node_page,NULL , node,MIGRATE_SYNC ,MR_SYSCALL );if (err)@@ -1589,6 +1586,9 @@ static int move_pages_and_store_status(struct mm_struct *mm, int node, {int err; +if (list_empty (pagelist)) +return 0 ; + err =do_move_pages_to_node (mm, pagelist, node);if (err)return err;@@ -1679,9 +1679,6 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, current_node =NUMA_NO_NODE ; } out_flush: -if (list_empty (&pagelist)) -return err; -/* Make sure we do not overwrite the existing error */ err1 =move_pages_and_store_status (mm, current_node, &pagelist, status, start, i - start);Signature · 1 line
2.17.1
Signature · 2 lines
Michal Hocko SUSE Labs
PATCH 6/8 mm/migrate.c: handle same node and add failure in the same way
In reply to Wei YangWhen page is not queued for migration, there are two possible cases:
* page already on the target node
* failed to add to migration queue
Current code handle them differently, this leads to a behavior inconsistency.
Usually for each page's status, we just do store for once. While for the page already on the target node, we might store the node information for twice:
* once when we found the page is on the target node
* second when moving the pages to target node successfully after above action
The reason is even we don't add the page to pagelist, but store_status() does store in a range which still contains the page.
This patch handles these two cases in the same way to reduce this inconsistency and also make the code a little easier to read.
--- mm/migrate.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
mm/migrate.c +8 -8
@@ -1657,18 +1657,18 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, err =add_page_for_migration (mm, addr, current_node, &pagelist, flags &MPOL_MF_MOVE_ALL ); -if (!err) { -/* The page is already on the target node */ - err =store_status (status, i, current_node,1 ); -if (err) - goto out_flush; -continue ; - }else if (err >0 ) { +if (err >0 ) {/* The page is successfully queued for migration */ continue ; } - err =store_status (status, i, err,1 ); +/* +* Two possible cases for err here: +* == 0: page is already on the target node, then store +* current_node to status +* < 0: failed to add page to list, then store err to status +*/ + err =store_status (status, i, err ? : current_node,1 );if (err) goto out_flush;
Signature · 1 line
2.17.1
Re: [PATCH 6/8] mm/migrate.c: handle same node and add failure in the same way
Quoted 16 lines · Wei Yang
When page is not queued for migration, there are two possible cases:
* page already on the target node
* failed to add to migration queueCurrent code handle them differently, this leads to a behavior inconsistency.
Usually for each page's status, we just do store for once. While for the page already on the target node, we might store the node information for twice:
* once when we found the page is on the target node
* second when moving the pages to target node successfully after above action
Quoted 5 lines
The reason is even we don't add the page to pagelist, but
store_status()does store in a range which still contains the page.This patch handles these two cases in the same way to reduce this inconsistency and also make the code a little easier to read.
Yeah, the improvement is really marginal. I do not feel strongly one way or another.
Quoted 1 line
Signed-off-by Wei Yang <[email protected]>
Quoted 38 lines
--- mm/migrate.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)mm/migrate.c +8
-8@@ -1657,18 +1657,18 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, err =add_page_for_migration (mm, addr, current_node, &pagelist, flags &MPOL_MF_MOVE_ALL ); -if (!err) { -/* The page is already on the target node */ - err =store_status (status, i, current_node,1 ); -if (err) - goto out_flush; -continue ; - }else if (err >0 ) { +if (err >0 ) {/* The page is successfully queued for migration */ continue ; } - err =store_status (status, i, err,1 ); +/* +* Two possible cases for err here: +* == 0: page is already on the target node, then store +* current_node to status +* < 0: failed to add page to list, then store err to status +*/ + err =store_status (status, i, err ? : current_node,1 );if (err) goto out_flush;Signature · 1 line
2.17.1
Signature · 2 lines
Michal Hocko SUSE Labs
PATCH 7/8 mm/migrate.c: move page on next iteration
In reply to Wei YangWhen page is not successfully queued for migration, we would move pages on pagelist immediately. Actually, this could be done in the next iteration by telling it we need some help.
This patch adds a new variable need_move to be an indication. After this, we only need to call move_pages_and_store_status() twice.
--- mm/migrate.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
mm/migrate.c +9 -9
@@ -1610,6 +1610,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,LIST_HEAD (pagelist);int start, i;int err =0 , err1; +int need_move =0 ;migrate_prep ();
@@ -1641,13 +1642,15 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,if (current_node ==NUMA_NO_NODE ) { current_node = node; start = i; - }else if (node != current_node) { + }else if (node != current_node || need_move) { err =move_pages_and_store_status (mm, current_node, - &pagelist, status, start, i - start); + &pagelist, status, start, + i - start - need_move);if (err) goto out; start = i; current_node = node; + need_move =0 ; }/*
@@ -1662,6 +1665,9 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,continue ; } +/* Ask next iteration to move us */ + need_move =1 ; +/* * Two possible cases for err here: * == 0: page is already on the target node, then store
@@ -1671,17 +1677,11 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, err =store_status (status, i, err ? : current_node,1 );if (err) goto out_flush; - - err =move_pages_and_store_status (mm, current_node, &pagelist, - status, start, i - start); -if (err) - goto out; - current_node =NUMA_NO_NODE ; } out_flush:/* Make sure we do not overwrite the existing error */ err1 =move_pages_and_store_status (mm, current_node, &pagelist, - status, start, i - start); + status, start, i - start - need_move);if (err >=0 ) err = err1; out:
Signature · 1 line
2.17.1
Re: [PATCH 7/8] mm/migrate.c: move page on next iteration
Quoted 6 lines · Wei Yang
When page is not successfully queued for migration, we would move pages on pagelist immediately. Actually, this could be done in the next iteration by telling it we need some help.
This patch adds a new variable
need_moveto be an indication. After this, we only need to callmove_pages_and_store_status()twice.
No! Not another iterator. There are two and this makes the function quite hard to follow already. We do not want to add a third one.
Quoted 67 lines
Signed-off-by Wei Yang <[email protected]>--- mm/migrate.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)mm/migrate.c +9
-9@@ -1610,6 +1610,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,LIST_HEAD (pagelist);int start, i;int err =0 , err1; +int need_move =0 ;migrate_prep ();@@ -1641,13 +1642,15 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,if (current_node ==NUMA_NO_NODE ) { current_node = node; start = i; - }else if (node != current_node) { + }else if (node != current_node || need_move) { err =move_pages_and_store_status (mm, current_node, - &pagelist, status, start, i - start); + &pagelist, status, start, + i - start - need_move);if (err) goto out; start = i; current_node = node; + need_move =0 ; }/* @@ -1662,6 +1665,9 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,continue ; } +/* Ask next iteration to move us */ + need_move =1 ; +/* * Two possible cases for err here: * == 0: page is already on the target node, then store @@ -1671,17 +1677,11 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, err =store_status (status, i, err ? : current_node,1 );if (err) goto out_flush; - - err =move_pages_and_store_status (mm, current_node, &pagelist, - status, start, i - start); -if (err) - goto out; - current_node =NUMA_NO_NODE ; } out_flush:/* Make sure we do not overwrite the existing error */ err1 =move_pages_and_store_status (mm, current_node, &pagelist, - status, start, i - start); + status, start, i - start - need_move);if (err >=0 ) err = err1; out:Signature · 1 line
2.17.1
Signature · 2 lines
Michal Hocko SUSE Labs
Re: [PATCH 7/8] mm/migrate.c: move page on next iteration
Quoted 11 lines · Michal Hocko
Quoted 6 lines · Wei Yang
When page is not successfully queued for migration, we would move pages on pagelist immediately. Actually, this could be done in the next iteration by telling it we need some help.
This patch adds a new variable
need_moveto be an indication. After this, we only need to callmove_pages_and_store_status()twice.No! Not another iterator. There are two and this makes the function quite hard to follow already. We do not want to add a third one.
Two iterators here are? I don't get your point.
Quoted 71 lines
Quoted 67 lines
Signed-off-by Wei Yang <[email protected]>--- mm/migrate.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)mm/migrate.c +9
-9@@ -1610,6 +1610,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,LIST_HEAD (pagelist);int start, i;int err =0 , err1; +int need_move =0 ;migrate_prep ();@@ -1641,13 +1642,15 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,if (current_node ==NUMA_NO_NODE ) { current_node = node; start = i; - }else if (node != current_node) { + }else if (node != current_node || need_move) { err =move_pages_and_store_status (mm, current_node, - &pagelist, status, start, i - start); + &pagelist, status, start, + i - start - need_move);if (err) goto out; start = i; current_node = node; + need_move =0 ; }/* @@ -1662,6 +1665,9 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,continue ; } +/* Ask next iteration to move us */ + need_move =1 ; +/* * Two possible cases for err here: * == 0: page is already on the target node, then store @@ -1671,17 +1677,11 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, err =store_status (status, i, err ? : current_node,1 );if (err) goto out_flush; - - err =move_pages_and_store_status (mm, current_node, &pagelist, - status, start, i - start); -if (err) - goto out; - current_node =NUMA_NO_NODE ; } out_flush:/* Make sure we do not overwrite the existing error */ err1 =move_pages_and_store_status (mm, current_node, &pagelist, - status, start, i - start); + status, start, i - start - need_move);if (err >=0 ) err = err1; out:Signature · 1 line
2.17.1Signature · 2 lines
Michal Hocko SUSE Labs
Signature · 2 lines
Wei Yang Help you, Help me
Re: [PATCH 7/8] mm/migrate.c: move page on next iteration
Quoted 6 lines · Wei Yang
Quoted 5 lines · Michal Hocko
Quoted 4 lines · Wei Yang
When page is not successfully queued for migration, we would move pages on pagelist immediately. Actually, this could be done in the next iteration by telling it we need some help.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
This patch adds a new variable
need_moveto be an indication. After this, we only need to callmove_pages_and_store_status()twice.No! Not another iterator. There are two and this makes the function quite hard to follow already. We do not want to add a third one.
Two iterators here are? I don't get your point.
i is the main iterator to process the whole imput. start is another one to control the batch to migrate. We do not need/want more. More clear?
Signature · 2 lines
Michal Hocko SUSE Labs
Re: [PATCH 7/8] mm/migrate.c: move page on next iteration
Quoted 10 lines · Michal Hocko
Quoted 9 lines · Wei Yang
Quoted 8 lines · Michal Hocko
Quoted 6 lines · Wei Yang
When page is not successfully queued for migration, we would move pages on pagelist immediately. Actually, this could be done in the next iteration by telling it we need some help.
This patch adds a new variable
need_moveto be an indication. After this, we only need to callmove_pages_and_store_status()twice.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
No! Not another iterator. There are two and this makes the function quite hard to follow already. We do not want to add a third one.
Two iterators here are? I don't get your point.
i is the main iterator to process the whole imput. start is another one to control the batch to migrate. We do not need/want more. More clear?
Yes, more clear.
I hope you are angry with me. Sorry for my poor English.
Quoted 3 lines
Signature · 2 lines
Michal Hocko SUSE Labs
Signature · 2 lines
Wei Yang Help you, Help me
Re: [PATCH 7/8] mm/migrate.c: move page on next iteration
Quoted 15 lines · Wei Yang
Quoted 14 lines · Michal Hocko
Quoted 13 lines · Wei Yang
Quoted 11 lines · Michal Hocko
Quoted 6 lines · Wei Yang
When page is not successfully queued for migration, we would move pages on pagelist immediately. Actually, this could be done in the next iteration by telling it we need some help.
This patch adds a new variable
need_moveto be an indication. After this, we only need to callmove_pages_and_store_status()twice.No! Not another iterator. There are two and this makes the function quite hard to follow already. We do not want to add a third one.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Two iterators here are? I don't get your point.
i is the main iterator to process the whole imput. start is another one to control the batch to migrate. We do not need/want more. More clear?
Yes, more clear.
Great
Quoted 1 line
I hope you are angry with me. Sorry for my poor English.
Heh, not at all.
Signature · 2 lines
Michal Hocko SUSE Labs
PATCH 8/8 mm/migrate.c: use break instead of goto out_flush
In reply to Wei YangLabel out_flush is just outside the loop, so break the loop is enough.
--- mm/migrate.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
mm/migrate.c +7 -7
@@ -1621,22 +1621,22 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, err = -EFAULT ;if (get_user (p, pages + i)) - goto out_flush; +break ;if (get_user (node, nodes + i)) - goto out_flush; +break ; addr = (unsigned long )untagged_addr (p);/* Check node if it is not checked. */ if (current_node ==NUMA_NO_NODE || node != current_node) { err = -ENODEV ;if (node <0 || node >=MAX_NUMNODES ) - goto out_flush; +break ;if (!node_state (node,N_MEMORY )) - goto out_flush; +break ; err = -EACCES ;if (!node_isset (node, task_nodes)) - goto out_flush; +break ; }if (current_node ==NUMA_NO_NODE ) {
@@ -1676,9 +1676,9 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,*/ err =store_status (status, i, err ? : current_node,1 );if (err) - goto out_flush; +break ; } -out_flush: +/* Make sure we do not overwrite the existing error */ err1 =move_pages_and_store_status (mm, current_node, &pagelist, status, start, i - start - need_move);
Signature · 1 line
2.17.1
Re: [PATCH 8/8] mm/migrate.c: use break instead of goto out_flush
Quoted 1 line · Wei Yang
Label
out_flushis just outside the loop, so break the loop is enough.
I find the explicit label easier to follow than breaking out of the loop. Especially when there is another break out of the loop with a different label.
While the patch is correct I find the resulting code worse readable.
Quoted 51 lines
Signed-off-by Wei Yang <[email protected]>--- mm/migrate.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)mm/migrate.c +7
-7@@ -1621,22 +1621,22 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, err = -EFAULT ;if (get_user (p, pages + i)) - goto out_flush; +break ;if (get_user (node, nodes + i)) - goto out_flush; +break ; addr = (unsigned long )untagged_addr (p);/* Check node if it is not checked. */ if (current_node ==NUMA_NO_NODE || node != current_node) { err = -ENODEV ;if (node <0 || node >=MAX_NUMNODES ) - goto out_flush; +break ;if (!node_state (node,N_MEMORY )) - goto out_flush; +break ; err = -EACCES ;if (!node_isset (node, task_nodes)) - goto out_flush; +break ; }if (current_node ==NUMA_NO_NODE ) {@@ -1676,9 +1676,9 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,*/ err =store_status (status, i, err ? : current_node,1 );if (err) - goto out_flush; +break ; } -out_flush: +/* Make sure we do not overwrite the existing error */ err1 =move_pages_and_store_status (mm, current_node, &pagelist, status, start, i - start - need_move);Signature · 1 line
2.17.1
Signature · 2 lines
Michal Hocko SUSE Labs
Re: [PATCH 8/8] mm/migrate.c: use break instead of goto out_flush
Quoted 9 lines · Michal Hocko
Quoted 1 line · Wei Yang
Label
out_flushis just outside the loop, so break the loop is enough.I find the explicit label easier to follow than breaking out of the loop. Especially when there is another break out of the loop with a different label.
While the patch is correct I find the resulting code worse readable.
ok, as you wish.
Signature · 2 lines
Wei Yang Help you, Help me