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
