Re: [RFC PATCH] mm: correct status code which move_pages() returns for zero page

[Michal Hocko <mhocko@suse.com>]

On Wed 18-04-18 11:07:22, Michal Hocko wrote:
> On Tue 17-04-18 16:09:33, Zi Yan wrote:
[...]
> > diff --git a/mm/migrate.c b/mm/migrate.c
> > index f65dd69e1fd1..32afa4723e7f 100644
> > --- a/mm/migrate.c
> > +++ b/mm/migrate.c
> > @@ -1619,6 +1619,8 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
> >                         if (err)
> >                                 goto out;
> >                 }
> > +               /* Move to next page (i+1), after we have saved page status (until i) */
> > +               start = i + 1;
> >                 current_node = NUMA_NO_NODE;
> >         }
> >  out_flush:
> > 
> > Feel free to check it by yourselves.
> 
> Yes, you are right. I never update start if the last page in the range
> fails and so we overwrite the whole [start, i] range. I wish the code
> wasn't that ugly and subtle but considering how we can fail in different
> ways and that we want to batch as much as possible I do not see an easy
> way.
> 
> Care to send the patch? I would just drop the comment.

Hmm, thinking about it some more. An alternative would be to check for
list_empty on the page list. It is a bit larger diff but maybe that
would be tiny bit cleaner because there is simply no point to call
do_move_pages_to_node on an empty list in the first place.
---
diff --git a/mm/migrate.c b/mm/migrate.c
index 507cf9ba21bf..46f93b9ba724 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -1634,12 +1634,14 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
 		current_node = NUMA_NO_NODE;
 	}
 out_flush:
-	/* 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 (!err)
-		err = err1;
+	if (!list_empty(&pagelist)) {
+		/* 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 (!err)
+			err = err1;
+	}
 out:
 	return err;
 }
-- 
Michal Hocko
SUSE Labs