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(-)
--
2.17.1
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(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index c3ef70de5876..4a63fb8fbb6d 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -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:
--
2.17.1
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.
Signed-off-by: Wei Yang <[email protected]>
---
mm/migrate.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index ba7cf4fa43a0..c3ef70de5876 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -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:
--
2.17.1
When 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.
Signed-off-by: Wei Yang <[email protected]>
---
mm/migrate.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index 46a5697b7fc6..aee5aeb082c4 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -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;
--
2.17.1
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_move to be an indication. After
this, we only need to call move_pages_and_store_status() twice.
Signed-off-by: Wei Yang <[email protected]>
---
mm/migrate.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index aee5aeb082c4..2a857fec65b6 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -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:
--
2.17.1
Usually 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.
Signed-off-by: Wei Yang <[email protected]>
---
mm/migrate.c | 34 +++++++++++++++++++---------------
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index 4a63fb8fbb6d..dec147d3a4dd 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -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:
--
2.17.1
When pagelist is empty, it is not necessary to do the move and store.
Also it consolidate the empty list check in one place.
Signed-off-by: Wei Yang <[email protected]>
---
mm/migrate.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index dec147d3a4dd..46a5697b7fc6 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -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);
--
2.17.1
Label out_flush is just outside the loop, so break the loop is enough.
Signed-off-by: Wei Yang <[email protected]>
---
mm/migrate.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/mm/migrate.c b/mm/migrate.c
index 2a857fec65b6..59bfae11b9d6 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -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);
--
2.17.1
On Sun, 19 Jan 2020, Wei Yang wrote:
> diff --git a/mm/migrate.c b/mm/migrate.c
> index ba7cf4fa43a0..c3ef70de5876 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -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:
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?
On Sun, Jan 19, 2020 at 02:14:28PM -0800, David Rientjes wrote:
>On Sun, 19 Jan 2020, Wei Yang wrote:
>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index ba7cf4fa43a0..c3ef70de5876 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -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:
>
>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.
--
Wei Yang
Help you, Help me
On Sun 19-01-20 11:06:30, Wei Yang wrote:
> 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.
> Signed-off-by: Wei Yang <[email protected]>
Acked-by: Michal Hocko <[email protected]>
Thanks!
> ---
> mm/migrate.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index ba7cf4fa43a0..c3ef70de5876 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -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:
> --
> 2.17.1
--
Michal Hocko
SUSE Labs
On Sun 19-01-20 11:06:31, Wei Yang wrote:
> 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(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index c3ef70de5876..4a63fb8fbb6d 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -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);
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.
> if (err >= 0)
> err = err1;
> out:
> --
> 2.17.1
--
Michal Hocko
SUSE Labs
On Sun 19-01-20 11:06:32, Wei Yang wrote:
> Usually 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.
The helper helps here indeed. Thanks this makes the code easier to read.
>
> Signed-off-by: Wei Yang <[email protected]>
Acked-by: Michal Hocko <[email protected]>
> ---
> mm/migrate.c | 34 +++++++++++++++++++---------------
> 1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 4a63fb8fbb6d..dec147d3a4dd 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -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:
> --
> 2.17.1
>
--
Michal Hocko
SUSE Labs
On Sun 19-01-20 11:06:33, Wei Yang wrote:
> 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.
> Signed-off-by: Wei Yang <[email protected]>
Acked-by: Michal Hocko <[email protected]>
> ---
> mm/migrate.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index dec147d3a4dd..46a5697b7fc6 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -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);
> --
> 2.17.1
>
--
Michal Hocko
SUSE Labs
On Sun 19-01-20 11:06:35, Wei Yang wrote:
> 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_move to be an indication. After
> this, we only need to call move_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.
> Signed-off-by: Wei Yang <[email protected]>
> ---
> mm/migrate.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index aee5aeb082c4..2a857fec65b6 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -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:
> --
> 2.17.1
>
--
Michal Hocko
SUSE Labs
On Sun 19-01-20 11:06:34, Wei Yang wrote:
> When 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.
Yeah, the improvement is really marginal. I do not feel strongly one way
or another.
> Signed-off-by: Wei Yang <[email protected]>
Acked-by: Michal Hocko <[email protected]>
> ---
> mm/migrate.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 46a5697b7fc6..aee5aeb082c4 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -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;
>
> --
> 2.17.1
>
--
Michal Hocko
SUSE Labs
On Sun 19-01-20 11:06:36, Wei Yang wrote:
> Label out_flush is 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.
> Signed-off-by: Wei Yang <[email protected]>
> ---
> mm/migrate.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 2a857fec65b6..59bfae11b9d6 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -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);
> --
> 2.17.1
--
Michal Hocko
SUSE Labs
On Mon, Jan 20, 2020 at 10:46:08AM +0100, Michal Hocko wrote:
>On Sun 19-01-20 11:06:31, Wei Yang wrote:
>> 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(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index c3ef70de5876..4a63fb8fbb6d 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -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);
>
>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.
>> if (err >= 0)
>> err = err1;
>> out:
>> --
>> 2.17.1
>
>--
>Michal Hocko
>SUSE Labs
--
Wei Yang
Help you, Help me
On Mon, Jan 20, 2020 at 11:03:28AM +0100, Michal Hocko wrote:
>On Sun 19-01-20 11:06:36, Wei Yang wrote:
>> Label out_flush is 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.
--
Wei Yang
Help you, Help me
On Mon, Jan 20, 2020 at 11:02:03AM +0100, Michal Hocko wrote:
>On Sun 19-01-20 11:06:35, Wei Yang wrote:
>> 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_move to be an indication. After
>> this, we only need to call move_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.
>> Signed-off-by: Wei Yang <[email protected]>
>> ---
>> mm/migrate.c | 18 +++++++++---------
>> 1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/mm/migrate.c b/mm/migrate.c
>> index aee5aeb082c4..2a857fec65b6 100644
>> --- a/mm/migrate.c
>> +++ b/mm/migrate.c
>> @@ -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:
>> --
>> 2.17.1
>>
>
>--
>Michal Hocko
>SUSE Labs
--
Wei Yang
Help you, Help me
On Tue 21-01-20 09:22:00, Wei Yang wrote:
> On Mon, Jan 20, 2020 at 11:02:03AM +0100, Michal Hocko wrote:
> >On Sun 19-01-20 11:06:35, Wei Yang wrote:
> >> 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_move to be an indication. After
> >> this, we only need to call move_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?
--
Michal Hocko
SUSE Labs
On Tue, Jan 21, 2020 at 09:43:52AM +0100, Michal Hocko wrote:
>On Tue 21-01-20 09:22:00, Wei Yang wrote:
>> On Mon, Jan 20, 2020 at 11:02:03AM +0100, Michal Hocko wrote:
>> >On Sun 19-01-20 11:06:35, Wei Yang wrote:
>> >> 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_move to be an indication. After
>> >> this, we only need to call move_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?
Yes, more clear.
I hope you are angry with me. Sorry for my poor English.
>--
>Michal Hocko
>SUSE Labs
--
Wei Yang
Help you, Help me
On Wed 22-01-20 08:40:12, Wei Yang wrote:
> On Tue, Jan 21, 2020 at 09:43:52AM +0100, Michal Hocko wrote:
> >On Tue 21-01-20 09:22:00, Wei Yang wrote:
> >> On Mon, Jan 20, 2020 at 11:02:03AM +0100, Michal Hocko wrote:
> >> >On Sun 19-01-20 11:06:35, Wei Yang wrote:
> >> >> 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_move to be an indication. After
> >> >> this, we only need to call move_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?
>
> Yes, more clear.
Great
> I hope you are angry with me. Sorry for my poor English.
Heh, not at all.
--
Michal Hocko
SUSE Labs