cleanup on do_pages_move()

PATCH Superseded Wei Yang 2 participants 16 messages Jan 22, 2020 – Jan 29, 2020

Subsystems (2)

Memory management MM migration

lore

Share & tools

lore

Wei Yang

author

2020-01-22 01:17 UTC #0 lore raw

v2 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.

After this, we can do a better fix.

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 | 57 +++++++++++++++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 27 deletions(-)

Signature · 1 line

2.17.1

Wei Yang

author

2020-01-22 01:17 UTC #1 lore raw

PATCH v2 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.

Signed-off-by Wei Yang <[email protected]>

Acked-by Michal Hocko <[email protected]>

---
 mm/migrate.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

mm/migrate.c +3 -5

index 430fdccc733e..4c2a21856717 100644
@@ -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

David Hildenbrand

Reviewed-by on v2 1/4

2020-01-28 10:04 UTC #2 lore raw

Re: [Patch v2 1/4] mm/migrate.c: not necessary to check start and i

Quoted 2 lines · Wei Yang

Till here, i must no less than start. And if i equals to start, store_status() would always return 0.

I'd suggest

"
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.
"

Reviewed-by David Hildenbrand <[email protected]>

Quoted 30 lines

Remove some unnecessary check to make it easy to read and prepare for further cleanup.

Signed-off-by Wei Yang <[email protected]>

Acked-by Michal Hocko <[email protected]>

---
 mm/migrate.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

mm/migrate.c +3 -5

index 430fdccc733e..4c2a21856717 100644
@@ -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 · 3 lines

Thanks,

David / dhildenb

Wei Yang

author

2020-01-29 00:31 UTC #3 lore raw

Re: [Patch v2 1/4] mm/migrate.c: not necessary to check start and i

Quoted 6 lines · David Hildenbrand

Quoted 2 lines · Wei Yang
Till here, i must no less than start. And if i equals to start, store_status() would always return 0.
I'd suggest

Quoted 9 lines

"
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.
"
Reviewed-by David Hildenbrand <[email protected]>

Thanks, I took it.

Signature · 2 lines

Wei Yang
Help you, Help me

Wei Yang

author

2020-01-22 01:17 UTC #4 lore raw

PATCH v2 2/4 mm/migrate.c: wrap do_move_pages_to_node() and store_status()

In reply to Wei Yang

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]>

Acked-by Michal Hocko <[email protected]>

---
 mm/migrate.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

mm/migrate.c +19 ~~-11~~

index 4c2a21856717..a4d3bd6475e1 100644
@@ -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.

@@ -1626,10 +1639,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 +1669,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 +1680,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

David Hildenbrand

Reviewed-by on v2 2/4

2020-01-28 10:15 UTC #5 lore raw

Re: [Patch v2 2/4] mm/migrate.c: wrap do_move_pages_to_node() and store_status()

Quoted 2 lines · Wei Yang

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.

I'd suggest

"
Usually, 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).
"

Quoted 17 lines

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]>
Acked-by Michal Hocko <[email protected]>
---
 mm/migrate.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)
mm/migrate.c +0 -0
index 4c2a21856717..a4d3bd6475e1 100644

mm/migrate.c static int add_page_for_migration(struct mm_struct *mm, unsigned long addr, · line 1588

Quoted 6 lines

mm/migrate.c +3 -0

@@ -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)

nit: indentation

mm/migrate.c static int add_page_for_migration(struct mm_struct *mm, unsigned long addr, · line 1596

Quoted 8 lines

mm/migrate.c +8 -0

+{
+	int err;
+
+	err = do_move_pages_to_node(mm, pagelist, node);
+	if (err)
+		return err;
+	err = store_status(status, start, node, nr);
+	return err;

return store_status(status, start, node, nr);

directly

Apart from that (and some more indentation nits)

Reviewed-by David Hildenbrand <[email protected]>

Signature · 3 lines

Thanks,

David / dhildenb

Wei Yang

author

2020-01-29 00:38 UTC #6 lore raw

Re: [Patch v2 2/4] mm/migrate.c: wrap do_move_pages_to_node() and store_status()

Quoted 10 lines · David Hildenbrand

Quoted 2 lines · Wei Yang
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.
I'd suggest
"
Usually, do_move_pages_to_node() and store_status() are used in combination. We have three similar call sites.

Quoted 5 lines

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).
"

Looks good.

Quoted 21 lines

Quoted 21 lines
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]>
Acked-by Michal Hocko <[email protected]>
---
 mm/migrate.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)
mm/migrate.c +0 -0
index 4c2a21856717..a4d3bd6475e1 100644
@@ -1583,6 +1583,19 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
 	return err;
 }
 

Quoted 6 lines

mm/migrate.c static int add_page_for_migration(struct mm_struct *mm, unsigned long addr, · line 1588
Quoted 3 lines
mm/migrate.c +3 -0
+static int move_pages_and_store_status(struct mm_struct *mm, int node,
+		struct list_head *pagelist, int __user *status,
+		int start, int nr)
nit: indentation

You mean indent like this?

static int move_pages_and_store_status(struct mm_struct *mm, int node,
struct list_head *pagelist, int __user *status,

This would be along list and I am afraid this is not the only valid code style?

Quoted 11 lines

mm/migrate.c static int add_page_for_migration(struct mm_struct *mm, unsigned long addr, · line 1596
Quoted 8 lines
mm/migrate.c +8 -0
+{
+	int err;
+
+	err = do_move_pages_to_node(mm, pagelist, node);
+	if (err)
+		return err;
+	err = store_status(status, start, node, nr);
+	return err;
return store_status(status, start, node, nr);

Quoted 2 lines

directly

Quoted 11 lines

Apart from that (and some more indentation nits)
Reviewed-by David Hildenbrand <[email protected]>
Signature · 3 lines
Thanks,

David / dhildenb

Signature · 2 lines

Wei Yang
Help you, Help me

David Hildenbrand

2020-01-29 09:29 UTC #7 lore raw

Re: [Patch v2 2/4] mm/migrate.c: wrap do_move_pages_to_node() and store_status()

Quoted 47 lines · Wei Yang

Quoted 10 lines · David Hildenbrand
Quoted 2 lines · Wei Yang
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.
I'd suggest
"
Usually, do_move_pages_to_node() and store_status() are used in combination. We have three similar call sites.
Quoted 5 lines
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).
"
Looks good.
Quoted 21 lines
Quoted 21 lines
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]>
Acked-by Michal Hocko <[email protected]>
---
 mm/migrate.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)
mm/migrate.c +0 -0
index 4c2a21856717..a4d3bd6475e1 100644
@@ -1583,6 +1583,19 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
 	return err;
 }
 
Quoted 6 lines
mm/migrate.c static int add_page_for_migration(struct mm_struct *mm, unsigned long addr, · line 1588
Quoted 3 lines
mm/migrate.c +3 -0
+static int move_pages_and_store_status(struct mm_struct *mm, int node,
+		struct list_head *pagelist, int __user *status,
+		int start, int nr)
nit: indentation

Quoted 8 lines

You mean indent like this?
static int move_pages_and_store_status(struct mm_struct *mm, int node,
struct list_head *pagelist, int __user *status,
This would be along list and I am afraid this is not the only valid code style?

Yes, that's what I meant. Documentation/process/coding-style.rst doesn't mention any specific way, but this is the most commonly used one.

Indentation in this file mostly sticks to something like this as well, but yeah, it's often a mess and not consistent.

That's why I note it whenever I see it, to make it eventually more consistent (and only make it a nit) :)

Signature · 3 lines

Thanks,

David / dhildenb

Wei Yang

author

2020-01-29 22:00 UTC #8 lore raw

Re: [Patch v2 2/4] mm/migrate.c: wrap do_move_pages_to_node() and store_status()

Quoted 57 lines · David Hildenbrand

Quoted 47 lines · Wei Yang
Quoted 10 lines · David Hildenbrand
Quoted 2 lines · Wei Yang
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.
I'd suggest
"
Usually, do_move_pages_to_node() and store_status() are used in combination. We have three similar call sites.
Quoted 5 lines
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).
"
Looks good.
Quoted 21 lines
Quoted 21 lines
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]>
Acked-by Michal Hocko <[email protected]>
---
 mm/migrate.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)
mm/migrate.c +0 -0
index 4c2a21856717..a4d3bd6475e1 100644
@@ -1583,6 +1583,19 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
 	return err;
 }
 
Quoted 6 lines
mm/migrate.c static int add_page_for_migration(struct mm_struct *mm, unsigned long addr, · line 1588
Quoted 3 lines
mm/migrate.c +3 -0
+static int move_pages_and_store_status(struct mm_struct *mm, int node,
+		struct list_head *pagelist, int __user *status,
+		int start, int nr)
nit: indentation
Quoted 8 lines
You mean indent like this?
static int move_pages_and_store_status(struct mm_struct *mm, int node,
struct list_head *pagelist, int __user *status,
This would be along list and I am afraid this is not the only valid code style?

Quoted 9 lines

Yes, that's what I meant. Documentation/process/coding-style.rst doesn't mention any specific way, but this is the most commonly used one.
Indentation in this file mostly sticks to something like this as well, but yeah, it's often a mess and not consistent.
That's why I note it whenever I see it, to make it eventually more consistent (and only make it a nit) :)

You are right. I would leave as it now, if someone else still have the same suggestion to fix it.

Thanks :-)

Quoted 4 lines

Signature · 3 lines
Thanks,

David / dhildenb

Signature · 2 lines

Wei Yang
Help you, Help me

Wei Yang

author

2020-01-22 01:17 UTC #9 lore raw

PATCH v2 3/4 mm/migrate.c: check pagelist in move_pages_and_store_status()

In reply to 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.

Signed-off-by Wei Yang <[email protected]>

Acked-by Michal Hocko <[email protected]>

---
 mm/migrate.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

mm/migrate.c +3 -6

index a4d3bd6475e1..80d2bba57265 100644
@@ -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;

@@ -1676,9 +1676,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

David Hildenbrand

2020-01-28 10:21 UTC #10 lore raw

Re: [Patch v2 3/4] mm/migrate.c: check pagelist in move_pages_and_store_status()

Quoted 34 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.

Signed-off-by Wei Yang <[email protected]>

Acked-by Michal Hocko <[email protected]>

---
 mm/migrate.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

mm/migrate.c +3 -3

index a4d3bd6475e1..80d2bba57265 100644
@@ -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;

mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1680

Quoted 5 lines

mm/migrate.c +0 -2

@@ -1676,9 +1676,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;

Am I wrong or are you discarding an error here? (err could be !0)

Signature · 3 lines

Thanks,

David / dhildenb

Wei Yang

author

2020-01-29 00:46 UTC #11 lore raw

Re: [Patch v2 3/4] mm/migrate.c: check pagelist in move_pages_and_store_status()

Quoted 28 lines · David Hildenbrand

Quoted 27 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.
Signed-off-by Wei Yang <[email protected]>
Acked-by Michal Hocko <[email protected]>
---
 mm/migrate.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)
mm/migrate.c +0 -3
index a4d3bd6475e1..80d2bba57265 100644
@@ -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;
 

Quoted 16 lines

mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1680
Quoted 12 lines
mm/migrate.c +3 -2
+	if (list_empty(pagelist))
+		return 0;
+
 	err = do_move_pages_to_node(mm, pagelist, node);
 	if (err)
 		return err;
@@ -1676,9 +1676,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;
Am I wrong or are you discarding an error here? (err could be !0)

This is not obvious in this code snippet. If you look into the source code, it might help you get the whole picture.

The following comment says:

Make sure we do not overwrite the existing error

So we didn't eat error here.

Quoted 5 lines

Signature · 4 lines
-- 
Thanks,

David / dhildenb

Signature · 2 lines

Wei Yang
Help you, Help me

David Hildenbrand

Reviewed-by

2020-01-29 09:37 UTC #12 lore raw

Re: [Patch v2 3/4] mm/migrate.c: check pagelist in move_pages_and_store_status()

Quoted 46 lines · Wei Yang

Quoted 28 lines · David Hildenbrand
Quoted 27 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.
Signed-off-by Wei Yang <[email protected]>
Acked-by Michal Hocko <[email protected]>
---
 mm/migrate.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)
mm/migrate.c +0 -3
index a4d3bd6475e1..80d2bba57265 100644
@@ -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;
 
Quoted 16 lines
mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1680
Quoted 12 lines
mm/migrate.c +3 -2
+	if (list_empty(pagelist))
+		return 0;
+
 	err = do_move_pages_to_node(mm, pagelist, node);
 	if (err)
 		return err;
@@ -1676,9 +1676,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;
Am I wrong or are you discarding an error here? (err could be !0)

Quoted 8 lines

This is not obvious in this code snippet. If you look into the source code, it might help you get the whole picture.
The following comment says:
Make sure we do not overwrite the existing error
So we didn't eat error here.

My fault, I misread the "if (err >= 0)" (and thought it was a "if (err1

Quoted 1 line

= 0)")

Makes perfect sense, thanks!

Reviewed-by David Hildenbrand <[email protected]>

Signature · 3 lines

Thanks,

David / dhildenb

Wei Yang

author

2020-01-22 01:17 UTC #13 lore raw

PATCH v2 4/4 mm/migrate.c: handle same node and add failure in the same way

In reply to 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 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]>

Acked-by Michal Hocko <[email protected]>

---
 mm/migrate.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

mm/migrate.c +8 -8

index 80d2bba57265..591f2e5caed6 100644
@@ -1654,18 +1654,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

David Hildenbrand

Reviewed-by on v2 4/4

2020-01-29 10:13 UTC #14 lore raw

Re: [Patch v2 4/4] 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 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

Quoted 6 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.

I'd rephrase to

"mm/migrate.c: unify "not queued for migration" handling in do_pages_move()

It 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. "

Quoted 23 lines

Signed-off-by Wei Yang <[email protected]>

Acked-by Michal Hocko <[email protected]>

---
 mm/migrate.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

mm/migrate.c +1 -7

index 80d2bba57265..591f2e5caed6 100644
@@ -1654,18 +1654,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 */

mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1667

Quoted 10 lines

mm/migrate.c +6 -1

 			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
+		 */

I'd shorten that to

/*
* If the page is already on the target node (!err), store the node,
* otherwise, store the err.
*/

mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1671

Quoted 5 lines

mm/migrate.c +1 -0

+		err = store_status(status, i, err ? : current_node, 1);
 		if (err)
 			goto out_flush;

Thanks!

Reviewed-by David Hildenbrand <[email protected]>

Signature · 3 lines

Thanks,

David / dhildenb

Wei Yang

author

2020-01-29 22:07 UTC #15 lore raw

Re: [Patch v2 4/4] mm/migrate.c: handle same node and add failure in the same way

Quoted 91 lines · David Hildenbrand

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 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
Quoted 6 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.
I'd rephrase to
"mm/migrate.c: unify "not queued for migration" handling in do_pages_move()
It 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. "
Quoted 23 lines
Signed-off-by Wei Yang <[email protected]>
Acked-by Michal Hocko <[email protected]>
---
 mm/migrate.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
mm/migrate.c +1 -7
index 80d2bba57265..591f2e5caed6 100644
@@ -1654,18 +1654,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 */
mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1667
Quoted 10 lines
mm/migrate.c +6 -1
 			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
+		 */
I'd shorten that to
/*
* If the page is already on the target node (!err), store the node,
* otherwise, store the err.
*/
mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1671
Quoted 5 lines
mm/migrate.c +1 -0
+		err = store_status(status, i, err ? : current_node, 1);
 		if (err)
 			goto out_flush;
 
Thanks!

Quoted 2 lines

Reviewed-by David Hildenbrand <[email protected]>

Yep, thanks.

I would take this :-)

Quoted 4 lines

Signature · 3 lines
Thanks,

David / dhildenb

Signature · 2 lines

Wei Yang
Help you, Help me