cleanup on do_pages_move()
Subsystems (2)
v3 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.
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: not necessary to check start and i 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 | 90 ++++++++++++++++++++++++---------------------------- 1 file changed, 42 insertions(+), 48 deletions(-)
Signature · 1 line
2.17.1
PATCH v3 1/4 mm/migrate.c: not necessary to check start and i
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.
--- mm/migrate.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
mm/migrate.c +3 -5
@@ -1674,11 +1674,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 v3 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() 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.
--- mm/migrate.c | 61 ++++++++++++++++++++++++++-------------------------- 1 file changed, 30 insertions(+), 31 deletions(-)
mm/migrate.c +30 -31
@@ -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, +unsigned long nr_pages,int start,int i) +{ +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 incude the rest of the +* nr_pages that have not attempted as well. +*/ +if (err >0 ) + err += nr_pages - i -1 ; +return err; + } + err =store_status (status, start, node, i - start); +return err; +} +/* * Migrate an array of page address onto an array of nodes and fill * the corresponding array of status.
@@ -1626,20 +1649,9 @@ 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 incude the rest of the -* nr_pages that have not 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, nr_pages, + start, i);if (err) goto out; start = i;
@@ -1668,13 +1680,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, nr_pages, start, i);if (err) goto out; current_node =NUMA_NO_NODE ;
@@ -1684,16 +1691,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 there is not non-attempted -* page. -* - If the above loop is aborted to it means more fatal error -* happened, should return err. -*/ -if (!err1) - err1 =store_status (status, start, current_node, i - start); + err1 =move_pages_and_store_status (mm, current_node, &pagelist, + status, nr_pages, start, i);if (err >=0 ) err = err1; out:
Signature · 1 line
2.17.1
Re: [Patch v3 2/4] 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.
Signed-off-by Wei Yang <[email protected]>
I believe I have already acked this one. Anyway
Quoted 99 lines
--- mm/migrate.c | 61 ++++++++++++++++++++++++++-------------------------- 1 file changed, 30 insertions(+), 31 deletions(-)mm/migrate.c +30
-31@@ -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, +unsigned long nr_pages,int start,int i) +{ +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 incude the rest of the +* nr_pages that have not attempted as well. +*/ +if (err >0 ) + err += nr_pages - i -1 ; +return err; + } + err =store_status (status, start, node, i - start); +return err; +} +/* * Migrate an array of page address onto an array of nodes and fill * the corresponding array of status. @@ -1626,20 +1649,9 @@ 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 incude the rest of the -* nr_pages that have not 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, nr_pages, + start, i);if (err) goto out; start = i;@@ -1668,13 +1680,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, nr_pages, start, i);if (err) goto out; current_node =NUMA_NO_NODE ;@@ -1684,16 +1691,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 there is not non-attempted -* page. -* - If the above loop is aborted to it means more fatal error -* happened, should return err. -*/ -if (!err1) - err1 =store_status (status, start, current_node, i - start); + err1 =move_pages_and_store_status (mm, current_node, &pagelist, + status, nr_pages, start, i);if (err >=0 ) err = err1; out:Signature · 1 line
2.17.1
Signature · 2 lines
Michal Hocko SUSE Labs
Re: [Patch v3 2/4] mm/migrate.c: wrap do_move_pages_to_node() and store_status()
Quoted 4 lines · Michal Hocko
Quoted 3 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.
Quoted 8 lines
Quoted 4 lines
This patch just wrap it to make it friendly to audience and also consolidate the move and store action into one place.
Signed-off-by Wei Yang <[email protected]>I believe I have already acked this one. Anyway
Acked-by Michal Hocko <[email protected]>
Yep, since some change from original code, I dropped your ack.
Thanks
Quoted 103 lines
Quoted 99 lines
--- mm/migrate.c | 61 ++++++++++++++++++++++++++-------------------------- 1 file changed, 30 insertions(+), 31 deletions(-)mm/migrate.c +30
-31@@ -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, +unsigned long nr_pages,int start,int i) +{ +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 incude the rest of the +* nr_pages that have not attempted as well. +*/ +if (err >0 ) + err += nr_pages - i -1 ; +return err; + } + err =store_status (status, start, node, i - start); +return err; +} +/* * Migrate an array of page address onto an array of nodes and fill * the corresponding array of status. @@ -1626,20 +1649,9 @@ 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 incude the rest of the -* nr_pages that have not 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, nr_pages, + start, i);if (err) goto out; start = i;@@ -1668,13 +1680,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, nr_pages, start, i);if (err) goto out; current_node =NUMA_NO_NODE ;@@ -1684,16 +1691,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 there is not non-attempted -* page. -* - If the above loop is aborted to it means more fatal error -* happened, should return err. -*/ -if (!err1) - err1 =store_status (status, start, current_node, i - start); + err1 =move_pages_and_store_status (mm, current_node, &pagelist, + status, nr_pages, start, i);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
PATCH v3 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) {/*
@@ -1687,9 +1687,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, nr_pages, start, i);
Signature · 1 line
2.17.1
PATCH v3 4/4 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
@@ -1665,18 +1665,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