cleanup on do_pages_move()
Subsystems (2)
v5 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.
v5:
: rebase on top of latest upstream, "linux/pipe_fs_i.h: fix kernel-doc
warnings after @wait was split"
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 | 88 +++++++++++++++++++++++----------------------------- 1 file changed, 39 insertions(+), 49 deletions(-)
Signature · 1 line
2.17.1
PATCH v5 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
@@ -1675,11 +1675,9 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, err += nr_pages - i -1 ; 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 v5 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 | 61 +++++++++++++++++++++++++--------------------------- 1 file changed, 29 insertions(+), 32 deletions(-)
mm/migrate.c +29 -32
@@ -1583,6 +1583,29 @@ 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 i,unsigned long nr_pages) +{ +int err; + + err =do_move_pages_to_node (mm, pagelist, node); +if (err) { +/* +* Positive err means the number of failed +* pages to migrate. Since we are going to +* abort and return the number of non-migrated +* pages, so need to incude the rest of the +* nr_pages that have not been attempted as +* well. +*/ +if (err >0 ) + err += nr_pages - i -1 ; +return err; + } +return store_status (status, start, node, i - start); +} +/* * Migrate an array of page address onto an array of nodes and fill * the corresponding array of status.
@@ -1626,21 +1649,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) { -/* -* Positive err means the number of failed -* pages to migrate. Since we are going to -* abort and return the number of non-migrated -* pages, so need to incude the rest of the -* nr_pages that have not been attempted as -* well. -*/ -if (err >0 ) - err += nr_pages - i -1 ; - goto out; - } - err =store_status (status, start, current_node, i - start); + err =move_pages_and_store_status (mm, current_node, + &pagelist, status, start, i, nr_pages);if (err) goto out; start = i;
@@ -1669,13 +1679,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) { -if (err >0 ) - err += nr_pages - i -1 ; - goto out; - } - err =store_status (status, start, current_node, i - start); + err =move_pages_and_store_status (mm, current_node, &pagelist, + status, start, i, nr_pages);if (err) goto out; current_node =NUMA_NO_NODE ;
@@ -1685,16 +1690,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); -/* -* Don't have to report non-attempted pages here since: -* - If the above loop is done gracefully all pages have been -* attempted. -* - If the above loop is aborted it means a fatal error -* happened, should return ret. -*/ -if (!err1) - err1 =store_status (status, start, current_node, i - start); + err1 =move_pages_and_store_status (mm, current_node, &pagelist, + status, start, i, nr_pages);if (err >=0 ) err = err1; out:
Signature · 1 line
2.17.1
PATCH v5 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) {/*
@@ -1686,9 +1686,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, nr_pages);
Signature · 1 line
2.17.1
PATCH v5 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
@@ -1664,18 +1664,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
Quoted 4 lines · Wei Yang
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 ondo_move_pages_to_node()andstore_status()with almost the same form.
Sounds this patch set is not picked up yet.
Any thing else I need to do?
Signature · 2 lines
Wei Yang Help you, Help me