cleanup on do_pages_move()
Subsystems (2)
v4 cover 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.
v4:
* rephrase changelog based on suggestion from David Hildenbrand
* trivial change on code style and comment
v3:
* rebase on top of Yang Shi's fix "mm: move_pages: report the number of non-attempted pages"
v2:
* remove some unnecessary cleanup
Wei Yang (4):
mm/migrate.c: no need to check for i > start in do_pages_move() 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: unify "not queued for migration" handling in
do_pages_move()
mm/migrate.c | 54 ++++++++++++++++++++++++++-------------------------- 1 file changed, 27 insertions(+), 27 deletions(-)
Signature · 1 line
2.17.1
PATCH v4 1/4 mm/migrate.c: no need to check for i > start in do_pages_move()
At this point, we always have i >= start. If i == start, store_status() will return 0. So we can drop the check for i > start.
[[email protected] rephrase changelog]
--- mm/migrate.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
mm/migrate.c +3 -5
@@ -1661,11 +1661,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
PATCH v4 2/4 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() are used in combination. We have three similar call sites.
Let's provide a wrapper for both function calls - move_pages_and_store_status - to make the calling code easier to maintain and fix (as noted by Yang Shi, the return value handling of do_move_pages_to_node() has a flaw).
[[email protected] rephrase changelog]
--- mm/migrate.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-)
mm/migrate.c +18 -11
@@ -1583,6 +1583,18 @@ 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; +return store_status (status, start, node, nr); +} +/* * Migrate an array of page address onto an array of nodes and fill * the corresponding array of status.
@@ -1626,10 +1638,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;
@@ -1658,10 +1668,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 ;
@@ -1671,9 +1679,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) - 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
PATCH v4 3/4 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;
@@ -1675,9 +1675,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
PATCH v4 4/4 mm/migrate.c: unify "not queued for migration" handling in do_pages_move()
In reply to Wei YangIt can currently happen that we store the status of a page twice:
* Once we detect that it is already on the target node
* Once we moved a bunch of pages, and a page that's already on the target node is contained in the current interval.
Let's simplify the code and always call do_move_pages_to_node() in case we did not queue a page for migration. Note that pages that are already on the target node are not added to the pagelist and are, therefore, ignored by do_move_pages_to_node() - there is no functional change.
The status of such a page is now only stored once.
[[email protected] rephrase changelog]
--- mm/migrate.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)
mm/migrate.c +6 -8
@@ -1653,18 +1653,16 @@ 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 ); +/* +* If the page is already on the target node (!err), store the +* node, otherwise, store the err. +*/ + err =store_status (status, i, err ? : current_node,1 );if (err) goto out_flush;
Signature · 1 line
2.17.1