mm: correct status code which move_pages() returns for zero page
Subsystems (2)
RFC PATCH 1/1 mm: correct status code which move_pages() returns for zero page
move_pages(2) declears that status code for zero page is supposed to be -EFAULT. But now it (LTP/move_pages04 test) gets -EPERM, the root cause is that not goto out_flush after store_status() saves the err which add_page_for_migration() returns for zero page.
LTP move_pages04:
TFAIL : move_pages04.c:143: status[1] is EPERM, expected EFAULT
--- mm/migrate.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
mm/migrate.c +1 -1
@@ -1608,7 +1608,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,continue ; err =store_status (status, i, err,1 ); -if (err) +if (!err) goto out_flush; err =do_move_pages_to_node (mm, &pagelist, current_node);
Signature · 1 line
2.9.5
On Tue 17-04-18 19:06:15, Li Wang wrote:
[...]
mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1612
Quoted 11 lines
mm/migrate.c +1
-1@@ -1608,7 +1608,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,continue ; err =store_status (status, i, err,1 ); -if (err) +if (!err) goto out_flush;
This change just doesn't make any sense to me. Why should we bail out if the store_status is successul? I am trying to wrap my head around the test case. 6b9d757ecafc ("mm, numa: rework do_pages_move") tried to explain that move_pages has some semantic issues and the new implementation might be not 100% replacement. Anyway I am studying the test case to come up with a proper fix.
Quoted 5 lines
mm/migrate.c +0
-0err =do_move_pages_to_node (mm, &pagelist, current_node);Signature · 1 line
2.9.5
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 9 lines · Michal Hocko
On Tue 17-04-18 19:06:15, Li Wang wrote:
[...]Quoted 7 lines
mm/migrate.c +0
-0@@ -1608,7 +1608,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,continue ;
Quoted 11 lines
mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1612
Quoted 4 lines
mm/migrate.c +1
-1err =store_status (status, i, err,1 ); -if (err) +if (!err) goto out_flush;This change just doesn't make any sense to me. Why should we bail out if the
store_statusis successul? I am trying to wrap my head around the test case.6b9d757ecafc("mm, numa: reworkdo_pages_move") tried to explain thatmove_pageshas some semantic issues and the new implementation might be not 100% replacement. Anyway I am studying the test case to come up with a proper fix.
OK, I get what the test cases does. I've failed to see the subtle difference between alloc_pages_on_node and numa_alloc_onnode. The later doesn't faul in anything.
Why are we getting EPERM is quite not yet clear to me. add_page_for_migration uses FOLL_DUMP which should return EFAULT on zero pages (no_page_table()).
err = PTR_ERR(page); if (IS_ERR(page)) goto out;
therefore bails out from add_page_for_migration and store_status should store that value. There shouldn't be any EPERM on the way.
Let me try to reproduce and see what is going on. Btw. which kernel do you try this on?
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 31 lines · Michal Hocko
Quoted 7 lines · Michal Hocko
On Tue 17-04-18 19:06:15, Li Wang wrote:
[...]mm/migrate.c static int do_pages_move(struct mm_struct *mm,
Quoted 5 lines
mm/migrate.c +0
-0@@ -1608,7 +1608,7 @@ static int do_pages_move(struct mm_struct *mm,
nodemask_ttask_nodes,Quoted 7 lines
mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1612
Quoted 6 lines
mm/migrate.c +1
-1continue ; err =store_status (status, i, err,1 ); -if (err) +if (!err) goto out_flush;Quoted 6 lines
This change just doesn't make any sense to me. Why should we bail out if the
store_statusis successul? I am trying to wrap my head around the test case.6b9d757ecafc("mm, numa: reworkdo_pages_move") tried to explain thatmove_pageshas some semantic issues and the new implementation might be not 100% replacement. Anyway I am studying the test case to come up with a proper fix.OK, I get what the test cases does. I've failed to see the subtle difference between
alloc_pages_on_nodeandnuma_alloc_onnode. The later doesn't faul in anything.Why are we getting EPERM is quite not yet clear to me.
add_page_for_migrationusesFOLL_DUMPwhich should return EFAULT on zero pages (no_page_table()).
Quoted 7 lines
err = PTR_ERR(page); if (IS_ERR(page)) goto out;therefore bails out from
add_page_for_migrationandstore_statusshould store that value. There shouldn't be any EPERM on the way.
Yes, I print the the return value and confirmed the add_page_for_migration()
do right things for zero page. and after store_status(...) the status saves -EFAULT.
So I did the change above.
Quoted 4 lines
Let me try to reproduce and see what is going on. Btw. which kernel do you try this on?
The latest mainline kernel-4.17-rc1.
Signature · 5 lines
Li Wang [email protected] -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.linux.it/pipermail/ltp/attachments/20180417/9d58d972/attachment.html>
Quoted 41 lines · Li Wang
Quoted 31 lines · Michal Hocko
Quoted 7 lines · Michal Hocko
On Tue 17-04-18 19:06:15, Li Wang wrote:
[...]mm/migrate.c static int do_pages_move(struct mm_struct *mm,
Quoted 5 lines
mm/migrate.c +0
-0@@ -1608,7 +1608,7 @@ static int do_pages_move(struct mm_struct *mm,
nodemask_ttask_nodes,Quoted 7 lines
mm/migrate.c static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, · line 1612
Quoted 6 lines
mm/migrate.c +1
-1continue ; err =store_status (status, i, err,1 ); -if (err) +if (!err) goto out_flush;Quoted 6 lines
This change just doesn't make any sense to me. Why should we bail out if the
store_statusis successul? I am trying to wrap my head around the test case.6b9d757ecafc("mm, numa: reworkdo_pages_move") tried to explain thatmove_pageshas some semantic issues and the new implementation might be not 100% replacement. Anyway I am studying the test case to come up with a proper fix.OK, I get what the test cases does. I've failed to see the subtle difference between
alloc_pages_on_nodeandnuma_alloc_onnode. The later doesn't faul in anything.Why are we getting EPERM is quite not yet clear to me.
add_page_for_migrationusesFOLL_DUMPwhich should return EFAULT on zero pages (no_page_table()).Quoted 7 lines
err = PTR_ERR(page); if (IS_ERR(page)) goto out;therefore bails out from
add_page_for_migrationandstore_statusshould store that value. There shouldn't be any EPERM on the way.
Quoted 5 lines
Yes, I print the the return value and confirmed the add_page_for_migration()​
do right things for zero page. and afterstore_status(...)the status saves-EFAULT.
So I did the change above.
OK, I guess I knnow what is going on. I must be overwriting the status on the way out by
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);
This error handling is rather fragile and I was quite unhappy about it at the time I was developing it. I have to remember all the details why I've done it that way but I would bet my hat this is it. More on this tomorrow.
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 57 lines · Michal Hocko
Quoted 41 lines · Li Wang
Quoted 31 lines · Michal Hocko
Quoted 7 lines · Michal Hocko
On Tue 17-04-18 19:06:15, Li Wang wrote:
[...]mm/migrate.c static int do_pages_move(struct mm_struct *mm,
Quoted 5 lines
mm/migrate.c +0
-0@@ -1608,7 +1608,7 @@ static int do_pages_move(struct mm_struct *mm,
nodemask_ttask_nodes,Quoted 7 lines
mm/migrate.c static int do_pages_move(struct mm_struct *mm, · line 1612
Quoted 6 lines
mm/migrate.c +1
-1continue ; err =store_status (status, i, err,1 ); -if (err) +if (!err) goto out_flush;Quoted 6 lines
This change just doesn't make any sense to me. Why should we bail out if the
store_statusis successul? I am trying to wrap my head around the test case.6b9d757ecafc("mm, numa: reworkdo_pages_move") tried to explain thatmove_pageshas some semantic issues and the new implementation might be not 100% replacement. Anyway I am studying the test case to come up with a proper fix.OK, I get what the test cases does. I've failed to see the subtle difference between
alloc_pages_on_nodeandnuma_alloc_onnode. The later doesn't faul in anything.Why are we getting EPERM is quite not yet clear to me.
add_page_for_migrationusesFOLL_DUMPwhich should return EFAULT on zero pages (no_page_table()).Quoted 7 lines
err = PTR_ERR(page); if (IS_ERR(page)) goto out;therefore bails out from
add_page_for_migrationandstore_statusshould store that value. There shouldn't be any EPERM on the way.Quoted 5 lines
Yes, I print the the return value and confirmed the add_page_for_migration()​
do right things for zero page. and afterstore_status(...)the status saves-EFAULT.
So I did the change above.OK, I guess I knnow what is going on. I must be overwriting the status on the way out by
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);
Quoted 4 lines
This error handling is rather fragile and I was quite unhappy about it at the time I was developing it. I have to remember all the details why I've done it that way but I would bet my hat this is it. More on this tomorrow.
Hi Michal and Li,
The problem is that the variable start is not set properly after store_status(), like the "start = i;" after the first store_status().
The following patch should fix the problem (it has passed all move_pages test cases from ltp on my machine):
mm/migrate.c +2 -0
@@ -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.
--
Best Regards
Yan Zi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 496 bytes
Desc: OpenPGP digital signature
URL: <http://lists.linux.it/pipermail/ltp/attachments/20180417/ea924d1a/attachment-0001.sig>
Quoted 86 lines · Zi Yan
Quoted 57 lines · Michal Hocko
Quoted 41 lines · Li Wang
Quoted 31 lines · Michal Hocko
Quoted 7 lines · Michal Hocko
On Tue 17-04-18 19:06:15, Li Wang wrote:
[...]mm/migrate.c static int do_pages_move(struct mm_struct *mm,
Quoted 5 lines
mm/migrate.c +0
-0@@ -1608,7 +1608,7 @@ static int do_pages_move(struct mm_struct *mm,
nodemask_ttask_nodes,Quoted 7 lines
mm/migrate.c static int do_pages_move(struct mm_struct *mm, · line 1612
Quoted 6 lines
mm/migrate.c +1
-1continue ; err =store_status (status, i, err,1 ); -if (err) +if (!err) goto out_flush;Quoted 6 lines
This change just doesn't make any sense to me. Why should we bail out if the
store_statusis successul? I am trying to wrap my head around the test case.6b9d757ecafc("mm, numa: reworkdo_pages_move") tried to explain thatmove_pageshas some semantic issues and the new implementation might be not 100% replacement. Anyway I am studying the test case to come up with a proper fix.OK, I get what the test cases does. I've failed to see the subtle difference between
alloc_pages_on_nodeandnuma_alloc_onnode. The later doesn't faul in anything.Why are we getting EPERM is quite not yet clear to me.
add_page_for_migrationusesFOLL_DUMPwhich should return EFAULT on zero pages (no_page_table()).Quoted 7 lines
err = PTR_ERR(page); if (IS_ERR(page)) goto out;therefore bails out from
add_page_for_migrationandstore_statusshould store that value. There shouldn't be any EPERM on the way.Quoted 5 lines
Yes, I print the the return value and confirmed the add_page_for_migration()​
do right things for zero page. and afterstore_status(...)the status saves-EFAULT.
So I did the change above.OK, I guess I knnow what is going on. I must be overwriting the status on the way out by
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);Quoted 4 lines
This error handling is rather fragile and I was quite unhappy about it at the time I was developing it. I have to remember all the details why I've done it that way but I would bet my hat this is it. More on this tomorrow.
Hi Michal and Li,
The problem is that the variable start is not set properly after
store_status(), like the "start = i;" after the firststore_status().The following patch should fix the problem (it has passed all
move_pagestest cases from ltp on my machine):mm/migrate.c +2
-0@@ -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:
Quoted 1 line
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.
Thanks!
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 1 line · Michal Hocko
On Tue 17-04-18 16:09:33, Zi Yan wrote:
[...]
Quoted 22 lines
Quoted 14 lines
mm/migrate.c +2
-0@@ -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:Quoted 1 line
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.
Quoted 1 line
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.
---
mm/migrate.c +8 -6
@@ -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; }
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 27 lines · Michal Hocko
Quoted 1 line · Michal Hocko
On Tue 17-04-18 16:09:33, Zi Yan wrote:
[...]
Quoted 5 lines
mm/migrate.c static int do_pages_move(struct mm_struct *mm,
Quoted 5 lines
mm/migrate.c +0
-0@@ -1619,6 +1619,8 @@ static int do_pages_move(struct mm_struct *mm,
nodemask_ttask_nodes,Quoted 4 lines
mm/migrate.c static int do_pages_move(struct mm_struct *mm, · line 1622
Quoted 4 lines
mm/migrate.c +1
-0if (err) goto out; } +/* Move to next page (i+1), after we have saved page status (until i) */
Quoted 13 lines
Quoted 6 lines
mm/migrate.c +1
-0+ 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.
Quoted 7 lines
Quoted 1 line
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_emptyon the page list. It is a bit larger diff but maybe that would be tiny bit cleaner because there is simply no point to calldo_move_pages_to_nodeon an empty list in the first place.
​Hi Michal, Zi
I tried your patch separately, both of them works fine to me.
Signature · 5 lines
Li Wang [email protected] -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.linux.it/pipermail/ltp/attachments/20180418/2cec07f6/attachment.html>
Quoted 37 lines · Li Wang
Quoted 27 lines · Michal Hocko
Quoted 1 line · Michal Hocko
On Tue 17-04-18 16:09:33, Zi Yan wrote:
[...]
Quoted 5 lines
mm/migrate.c static int do_pages_move(struct mm_struct *mm,
Quoted 5 lines
mm/migrate.c +0
-0@@ -1619,6 +1619,8 @@ static int do_pages_move(struct mm_struct *mm,
nodemask_ttask_nodes,Quoted 4 lines
mm/migrate.c static int do_pages_move(struct mm_struct *mm, · line 1622
Quoted 4 lines
mm/migrate.c +1
-0if (err) goto out; } +/* Move to next page (i+1), after we have saved page status (until i) */
Quoted 13 lines
Quoted 6 lines
mm/migrate.c +1
-0+ 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.
Quoted 7 lines
Quoted 1 line
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_emptyon the page list. It is a bit larger diff but maybe that would be tiny bit cleaner because there is simply no point to calldo_move_pages_to_nodeon an empty list in the first place.
Quoted 3 lines
​Hi Michal, Zi
I tried your patch separately, both of them works fine to me.
Thanks for retesting! Do you plan to post a patch with the changelog or should I do it?
Signature · 2 lines
Michal Hocko SUSE Labs
Quoted 43 lines · On Wed, Apr 18, 2018, 19:29 Michal Hocko <[email protected]> wrote:
Quoted 10 lines · Li Wang
Quoted 8 lines · Michal Hocko
Quoted 1 line · Michal Hocko
On Tue 17-04-18 16:09:33, Zi Yan wrote:
[...]
Quoted 5 lines
mm/migrate.c static int do_pages_move(struct mm_struct
Quoted 5 lines
mm/migrate.c +0
-0@@ -1619,6 +1619,8 @@ static int do_pages_move(struct mm_struct*mm,
Quoted 5 lines
Quoted 5 lines
nodemask_ttask_nodes,Quoted 4 lines
Quoted 4 lines
mm/migrate.c +1
-0if (err) goto out; } +/* Move to next page (i+1), after we have saved page
Quoted 9 lines
Quoted 9 lines
status (until i) */
Quoted 8 lines
Quoted 6 lines
mm/migrate.c +1
-0+ 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
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
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
Quoted 1 line
Quoted 1 line
Quoted 1 line
ways and that we want to batch as much as possible I do not see an
easy
Quoted 10 lines
Quoted 9 lines
Quoted 3 lines
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_emptyon the page list. It is a bit larger diff but maybe that would be tiny bit cleaner because there is simply no point to calldo_move_pages_to_nodeon an empty list in the first place.
Quoted 7 lines
Quoted 3 lines
​Hi Michal, Zi
I tried your patch separately, both of them works fine to me.
Thanks for retesting! Do you plan to post a patch with the changelog or should I do it?
You better.
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20180418/43a89c60/attachment.html>