Re: [RFC PATCH 1/3] mm, numa: rework do_pages_move

[Michal Hocko]

On Wed 13-12-17 15:47:31, Kirill A. Shutemov wrote:
> On Wed, Dec 13, 2017 at 01:17:03PM +0100, Michal Hocko wrote:
> > On Wed 13-12-17 15:07:33, Kirill A. Shutemov wrote:
> > [...]
> > > The approach looks fine to me.
> > > 
> > > But patch is rather large and hard to review. And how git mixed add/remove
> > > lines doesn't help too. Any chance to split it up further?
> > 
> > I was trying to do that but this is a drop in replacement so it is quite
> > hard to do in smaller pieces. I've already put the allocation callback
> > cleanup into a separate one but this is about all that I figured how to
> > split. If you have any suggestions I am willing to try them out.
> 
> "git diff --patience" seems generate more readable output for the patch.

Hmm, I wasn't aware of this option. Are you suggesting I should use it
to general the patch to send?

> > > One nitpick: I don't think 'chunk' terminology should go away with the
> > > patch.
> > 
> > Not sure what you mean here. I have kept chunk_start, chunk_node, so I
> > am not really changing that terminology
> 
> We don't really have chunks anymore, right? We still *may* have per-node
> batching, but..
> 
> Maybe just 'start' and 'current_node'?

Ohh, I've read your response that you want to preserve the naming. I can
certainly do the rename.
---
diff --git a/mm/migrate.c b/mm/migrate.c
index 9d7252ea2acd..5491045b60f9 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1556,14 +1556,14 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
 			 const int __user *nodes,
 			 int __user *status, int flags)
 {
-	int chunk_node = NUMA_NO_NODE;
+	int current_node = NUMA_NO_NODE;
 	LIST_HEAD(pagelist);
-	int chunk_start, i;
+	int start, i;
 	int err = 0, err1;
 
 	migrate_prep();
 
-	for (i = chunk_start = 0; i < nr_pages; i++) {
+	for (i = start = 0; i < nr_pages; i++) {
 		const void __user *p;
 		unsigned long addr;
 		int node;
@@ -1585,25 +1585,25 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
 		if (!node_isset(node, task_nodes))
 			goto out_flush;
 
-		if (chunk_node == NUMA_NO_NODE) {
-			chunk_node = node;
-			chunk_start = i;
-		} else if (node != chunk_node) {
-			err = do_move_pages_to_node(mm, &pagelist, chunk_node);
+		if (current_node == NUMA_NO_NODE) {
+			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, chunk_start, chunk_node, i - chunk_start);
+			err = store_status(status, start, current_node, i - start);
 			if (err)
 				goto out;
-			chunk_start = i;
-			chunk_node = node;
+			start = i;
+			current_node = node;
 		}
 
 		/*
 		 * Errors in the page lookup or isolation are not fatal and we simply
 		 * report them via status
 		 */
-		err = add_page_for_migration(mm, addr, chunk_node,
+		err = add_page_for_migration(mm, addr, current_node,
 				&pagelist, flags & MPOL_MF_MOVE_ALL);
 		if (!err)
 			continue;
@@ -1612,22 +1612,22 @@ 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, chunk_node);
+		err = do_move_pages_to_node(mm, &pagelist, current_node);
 		if (err)
 			goto out;
-		if (i > chunk_start) {
-			err = store_status(status, chunk_start, chunk_node, i - chunk_start);
+		if (i > start) {
+			err = store_status(status, start, current_node, i - start);
 			if (err)
 				goto out;
 		}
-		chunk_node = NUMA_NO_NODE;
+		current_node = NUMA_NO_NODE;
 	}
 	err = 0;
 out_flush:
 	/* Make sure we do not overwrite the existing error */
-	err1 = do_move_pages_to_node(mm, &pagelist, chunk_node);
+	err1 = do_move_pages_to_node(mm, &pagelist, current_node);
 	if (!err1)
-		err1 = store_status(status, chunk_start, chunk_node, i - chunk_start);
+		err1 = store_status(status, start, current_node, i - start);
 	if (!err)
 		err = err1;
 out:
-- 
Michal Hocko
SUSE Labs