* [PATCH] mm: move_pages: fix the return value if there are not-migrated pages @ 2020-01-18 5:26 Yang Shi 2020-01-19 2:37 ` Wei Yang 2020-01-20 13:06 ` Michal Hocko 0 siblings, 2 replies; 14+ messages in thread From: Yang Shi @ 2020-01-18 5:26 UTC (permalink / raw) To: mhocko, richardw.yang, akpm; +Cc: yang.shi, linux-mm, linux-kernel, stable The do_move_pages_to_node() might return > 0 value, the number of pages that are not migrated, then the value will be returned to userspace directly. But, move_pages() syscall would just return 0 or errno. So, we need reset the return value to 0 for such case as what pre-v4.17 did. Fixes: a49bd4d71637 ("mm, numa: rework do_pages_move") Cc: Michal Hocko <mhocko@suse.com> Cc: Wei Yang <richardw.yang@linux.intel.com> Cc: <stable@vger.kernel.org> [4.17+] Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> --- mm/migrate.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/mm/migrate.c b/mm/migrate.c index 86873b6..3e75432 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -1659,8 +1659,11 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, goto out_flush; err = do_move_pages_to_node(mm, &pagelist, current_node); - if (err) + if (err) { + if (err > 0) + err = 0; goto out; + } if (i > start) { err = store_status(status, start, current_node, i - start); if (err) -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] mm: move_pages: fix the return value if there are not-migrated pages 2020-01-18 5:26 [PATCH] mm: move_pages: fix the return value if there are not-migrated pages Yang Shi @ 2020-01-19 2:37 ` Wei Yang 2020-01-19 2:57 ` Wei Yang 2020-01-19 5:44 ` Yang Shi 2020-01-20 13:06 ` Michal Hocko 1 sibling, 2 replies; 14+ messages in thread From: Wei Yang @ 2020-01-19 2:37 UTC (permalink / raw) To: Yang Shi; +Cc: mhocko, richardw.yang, akpm, linux-mm, linux-kernel, stable On Sat, Jan 18, 2020 at 01:26:43PM +0800, Yang Shi wrote: >The do_move_pages_to_node() might return > 0 value, the number of pages >that are not migrated, then the value will be returned to userspace >directly. But, move_pages() syscall would just return 0 or errno. So, >we need reset the return value to 0 for such case as what pre-v4.17 did. > >Fixes: a49bd4d71637 ("mm, numa: rework do_pages_move") >Cc: Michal Hocko <mhocko@suse.com> >Cc: Wei Yang <richardw.yang@linux.intel.com> >Cc: <stable@vger.kernel.org> [4.17+] >Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> >--- > mm/migrate.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > >diff --git a/mm/migrate.c b/mm/migrate.c >index 86873b6..3e75432 100644 >--- a/mm/migrate.c >+++ b/mm/migrate.c >@@ -1659,8 +1659,11 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, > goto out_flush; > > err = do_move_pages_to_node(mm, &pagelist, current_node); >- if (err) >+ if (err) { >+ if (err > 0) >+ err = 0; > goto out; >+ } > if (i > start) { > err = store_status(status, start, current_node, i - start); > if (err) >-- >1.8.3.1 Hey, I am afraid you missed something. There are three calls of do_move_pages_to_node() in do_pages_move(). Why you just handle one return value? How about the other two? -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mm: move_pages: fix the return value if there are not-migrated pages 2020-01-19 2:37 ` Wei Yang @ 2020-01-19 2:57 ` Wei Yang 2020-01-19 5:47 ` Yang Shi 2020-01-19 5:44 ` Yang Shi 1 sibling, 1 reply; 14+ messages in thread From: Wei Yang @ 2020-01-19 2:57 UTC (permalink / raw) To: Wei Yang; +Cc: Yang Shi, mhocko, akpm, linux-mm, linux-kernel, stable On Sun, Jan 19, 2020 at 10:37:20AM +0800, Wei Yang wrote: >On Sat, Jan 18, 2020 at 01:26:43PM +0800, Yang Shi wrote: >>The do_move_pages_to_node() might return > 0 value, the number of pages >>that are not migrated, then the value will be returned to userspace >>directly. But, move_pages() syscall would just return 0 or errno. So, >>we need reset the return value to 0 for such case as what pre-v4.17 did. >> >>Fixes: a49bd4d71637 ("mm, numa: rework do_pages_move") >>Cc: Michal Hocko <mhocko@suse.com> >>Cc: Wei Yang <richardw.yang@linux.intel.com> >>Cc: <stable@vger.kernel.org> [4.17+] >>Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> >>--- >> mm/migrate.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >>diff --git a/mm/migrate.c b/mm/migrate.c >>index 86873b6..3e75432 100644 >>--- a/mm/migrate.c >>+++ b/mm/migrate.c >>@@ -1659,8 +1659,11 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, >> goto out_flush; >> >> err = do_move_pages_to_node(mm, &pagelist, current_node); >>- if (err) >>+ if (err) { >>+ if (err > 0) >>+ err = 0; >> goto out; >>+ } >> if (i > start) { >> err = store_status(status, start, current_node, i - start); >> if (err) >>-- >>1.8.3.1 > > >Hey, I am afraid you missed something. There are three calls of >do_move_pages_to_node() in do_pages_move(). Why you just handle one return >value? How about the other two? > Well, current logic in do_pages_move() is a little complicated to read. I did a cleanup to make it easy to read and also friendly to do this fix. If they look good to you, you could rebase your fix on top of them. >-- >Wei Yang >Help you, Help me -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mm: move_pages: fix the return value if there are not-migrated pages 2020-01-19 2:57 ` Wei Yang @ 2020-01-19 5:47 ` Yang Shi 0 siblings, 0 replies; 14+ messages in thread From: Yang Shi @ 2020-01-19 5:47 UTC (permalink / raw) To: Wei Yang; +Cc: mhocko, akpm, linux-mm, linux-kernel, stable On 1/18/20 6:57 PM, Wei Yang wrote: > On Sun, Jan 19, 2020 at 10:37:20AM +0800, Wei Yang wrote: >> On Sat, Jan 18, 2020 at 01:26:43PM +0800, Yang Shi wrote: >>> The do_move_pages_to_node() might return > 0 value, the number of pages >>> that are not migrated, then the value will be returned to userspace >>> directly. But, move_pages() syscall would just return 0 or errno. So, >>> we need reset the return value to 0 for such case as what pre-v4.17 did. >>> >>> Fixes: a49bd4d71637 ("mm, numa: rework do_pages_move") >>> Cc: Michal Hocko <mhocko@suse.com> >>> Cc: Wei Yang <richardw.yang@linux.intel.com> >>> Cc: <stable@vger.kernel.org> [4.17+] >>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> >>> --- >>> mm/migrate.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/mm/migrate.c b/mm/migrate.c >>> index 86873b6..3e75432 100644 >>> --- a/mm/migrate.c >>> +++ b/mm/migrate.c >>> @@ -1659,8 +1659,11 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, >>> goto out_flush; >>> >>> err = do_move_pages_to_node(mm, &pagelist, current_node); >>> - if (err) >>> + if (err) { >>> + if (err > 0) >>> + err = 0; >>> goto out; >>> + } >>> if (i > start) { >>> err = store_status(status, start, current_node, i - start); >>> if (err) >>> -- >>> 1.8.3.1 >> >> Hey, I am afraid you missed something. There are three calls of >> do_move_pages_to_node() in do_pages_move(). Why you just handle one return >> value? How about the other two? >> > Well, current logic in do_pages_move() is a little complicated to read. > > I did a cleanup to make it easy to read and also friendly to do this fix. > > If they look good to you, you could rebase your fix on top of them. Regression fix typically has higher priority. Since we already spotted the regressions and proposed fixes, I'd suggest we fix them in 5.5-rc, then aim any cleanup for 5.6 or 5.7. This should also make review easier IMHO. > >> -- >> Wei Yang >> Help you, Help me ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mm: move_pages: fix the return value if there are not-migrated pages 2020-01-19 2:37 ` Wei Yang 2020-01-19 2:57 ` Wei Yang @ 2020-01-19 5:44 ` Yang Shi 1 sibling, 0 replies; 14+ messages in thread From: Yang Shi @ 2020-01-19 5:44 UTC (permalink / raw) To: Wei Yang; +Cc: mhocko, akpm, linux-mm, linux-kernel, stable On 1/18/20 6:37 PM, Wei Yang wrote: > On Sat, Jan 18, 2020 at 01:26:43PM +0800, Yang Shi wrote: >> The do_move_pages_to_node() might return > 0 value, the number of pages >> that are not migrated, then the value will be returned to userspace >> directly. But, move_pages() syscall would just return 0 or errno. So, >> we need reset the return value to 0 for such case as what pre-v4.17 did. >> >> Fixes: a49bd4d71637 ("mm, numa: rework do_pages_move") >> Cc: Michal Hocko <mhocko@suse.com> >> Cc: Wei Yang <richardw.yang@linux.intel.com> >> Cc: <stable@vger.kernel.org> [4.17+] >> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> >> --- >> mm/migrate.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/mm/migrate.c b/mm/migrate.c >> index 86873b6..3e75432 100644 >> --- a/mm/migrate.c >> +++ b/mm/migrate.c >> @@ -1659,8 +1659,11 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, >> goto out_flush; >> >> err = do_move_pages_to_node(mm, &pagelist, current_node); >> - if (err) >> + if (err) { >> + if (err > 0) >> + err = 0; >> goto out; >> + } >> if (i > start) { >> err = store_status(status, start, current_node, i - start); >> if (err) >> -- >> 1.8.3.1 > > Hey, I am afraid you missed something. There are three calls of > do_move_pages_to_node() in do_pages_move(). Why you just handle one return > value? How about the other two? A late night patch... I just simply missed those two. Thanks for catching it, will resolve in v2. > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mm: move_pages: fix the return value if there are not-migrated pages 2020-01-18 5:26 [PATCH] mm: move_pages: fix the return value if there are not-migrated pages Yang Shi 2020-01-19 2:37 ` Wei Yang @ 2020-01-20 13:06 ` Michal Hocko 2020-01-20 13:17 ` Michal Hocko 1 sibling, 1 reply; 14+ messages in thread From: Michal Hocko @ 2020-01-20 13:06 UTC (permalink / raw) To: Yang Shi; +Cc: richardw.yang, akpm, linux-mm, linux-kernel, stable On Sat 18-01-20 13:26:43, Yang Shi wrote: > The do_move_pages_to_node() might return > 0 value, the number of pages > that are not migrated, then the value will be returned to userspace > directly. But, move_pages() syscall would just return 0 or errno. So, > we need reset the return value to 0 for such case as what pre-v4.17 did. The patch is wrong. migrate_pages returns the number of pages it _hasn't_ migrated or -errno. Yeah that semantic sucks but... So err != 0 is always an error. Except err > 0 doesn't really provide any useful information to the userspace. I cannot really remember what was the actual behavior before my rework because there were some gotchas hidden there. If you want to fix this properly then you have to query node status of each page unmigrated when migrate_pages fails with > 0. This would be easier if the fix is done on the latest cleanup posted to the list which consolidates all do_move_pages_to_node and store_status calls to a single function. > Fixes: a49bd4d71637 ("mm, numa: rework do_pages_move") > Cc: Michal Hocko <mhocko@suse.com> > Cc: Wei Yang <richardw.yang@linux.intel.com> > Cc: <stable@vger.kernel.org> [4.17+] > Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> > --- > mm/migrate.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/mm/migrate.c b/mm/migrate.c > index 86873b6..3e75432 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -1659,8 +1659,11 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, > goto out_flush; > > err = do_move_pages_to_node(mm, &pagelist, current_node); > - if (err) > + if (err) { > + if (err > 0) > + err = 0; > goto out; > + } > if (i > start) { > err = store_status(status, start, current_node, i - start); > if (err) > -- > 1.8.3.1 -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mm: move_pages: fix the return value if there are not-migrated pages 2020-01-20 13:06 ` Michal Hocko @ 2020-01-20 13:17 ` Michal Hocko 2020-01-21 1:44 ` Wei Yang 0 siblings, 1 reply; 14+ messages in thread From: Michal Hocko @ 2020-01-20 13:17 UTC (permalink / raw) To: Yang Shi; +Cc: richardw.yang, akpm, linux-mm, linux-kernel, stable On Mon 20-01-20 14:06:26, Michal Hocko wrote: > On Sat 18-01-20 13:26:43, Yang Shi wrote: > > The do_move_pages_to_node() might return > 0 value, the number of pages > > that are not migrated, then the value will be returned to userspace > > directly. But, move_pages() syscall would just return 0 or errno. So, > > we need reset the return value to 0 for such case as what pre-v4.17 did. > > The patch is wrong. migrate_pages returns the number of pages it > _hasn't_ migrated or -errno. Yeah that semantic sucks but... > So err != 0 is always an error. Except err > 0 doesn't really provide > any useful information to the userspace. I cannot really remember what > was the actual behavior before my rework because there were some gotchas > hidden there. OK, so I've double checked. do_move_page_to_node_array would carry the error code over to do_pages_move and it would store the status stored in the pm array. It contains page_to_nid(page) so the resulting code indeed behaves properly before my change and this is a regression. I have a very vague recollection that this has been brought up already. <...looks in notes...> Found it! The report is http://lkml.kernel.org/r/0329efa0984b9b0252ef166abb4498c0795fab36.1535113317.git.jstancek@redhat.com and my proposed workaround was http://lkml.kernel.org/r/20180829145537.GZ10223@dhcp22.suse.cz > If you want to fix this properly then you have to query node status of > each page unmigrated when migrate_pages fails with > 0. This would be > easier if the fix is done on the latest cleanup posted to the list which > consolidates all do_move_pages_to_node and store_status calls to a > single function. Sorry forgot to put a reference to the patch: http://lkml.kernel.org/r/20200119030636.11899-5-richardw.yang@linux.intel.com -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mm: move_pages: fix the return value if there are not-migrated pages 2020-01-20 13:17 ` Michal Hocko @ 2020-01-21 1:44 ` Wei Yang 2020-01-21 8:40 ` Michal Hocko 0 siblings, 1 reply; 14+ messages in thread From: Wei Yang @ 2020-01-21 1:44 UTC (permalink / raw) To: Michal Hocko Cc: Yang Shi, richardw.yang, akpm, linux-mm, linux-kernel, stable On Mon, Jan 20, 2020 at 02:17:44PM +0100, Michal Hocko wrote: >On Mon 20-01-20 14:06:26, Michal Hocko wrote: >> On Sat 18-01-20 13:26:43, Yang Shi wrote: >> > The do_move_pages_to_node() might return > 0 value, the number of pages >> > that are not migrated, then the value will be returned to userspace >> > directly. But, move_pages() syscall would just return 0 or errno. So, >> > we need reset the return value to 0 for such case as what pre-v4.17 did. >> >> The patch is wrong. migrate_pages returns the number of pages it >> _hasn't_ migrated or -errno. Yeah that semantic sucks but... >> So err != 0 is always an error. Except err > 0 doesn't really provide >> any useful information to the userspace. I cannot really remember what >> was the actual behavior before my rework because there were some gotchas >> hidden there. > >OK, so I've double checked. do_move_page_to_node_array would carry the >error code over to do_pages_move and it would store the status stored >in the pm array. It contains page_to_nid(page) so the resulting code >indeed behaves properly before my change and this is a regression. I Thanks, I see the change. >have a very vague recollection that this has been brought up already. ><...looks in notes...> >Found it! The report is >http://lkml.kernel.org/r/0329efa0984b9b0252ef166abb4498c0795fab36.1535113317.git.jstancek@redhat.com >and my proposed workaround was http://lkml.kernel.org/r/20180829145537.GZ10223@dhcp22.suse.cz Well, the above two links return 404. > >> If you want to fix this properly then you have to query node status of >> each page unmigrated when migrate_pages fails with > 0. This would be >> easier if the fix is done on the latest cleanup posted to the list which >> consolidates all do_move_pages_to_node and store_status calls to a >> single function. > >Sorry forgot to put a reference to the patch: http://lkml.kernel.org/r/20200119030636.11899-5-richardw.yang@linux.intel.com > >-- >Michal Hocko >SUSE Labs -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mm: move_pages: fix the return value if there are not-migrated pages 2020-01-21 1:44 ` Wei Yang @ 2020-01-21 8:40 ` Michal Hocko 2020-01-21 19:01 ` Yang Shi 0 siblings, 1 reply; 14+ messages in thread From: Michal Hocko @ 2020-01-21 8:40 UTC (permalink / raw) To: Wei Yang; +Cc: Yang Shi, akpm, linux-mm, linux-kernel, stable On Tue 21-01-20 09:44:16, Wei Yang wrote: > On Mon, Jan 20, 2020 at 02:17:44PM +0100, Michal Hocko wrote: > >On Mon 20-01-20 14:06:26, Michal Hocko wrote: > >> On Sat 18-01-20 13:26:43, Yang Shi wrote: > >> > The do_move_pages_to_node() might return > 0 value, the number of pages > >> > that are not migrated, then the value will be returned to userspace > >> > directly. But, move_pages() syscall would just return 0 or errno. So, > >> > we need reset the return value to 0 for such case as what pre-v4.17 did. > >> > >> The patch is wrong. migrate_pages returns the number of pages it > >> _hasn't_ migrated or -errno. Yeah that semantic sucks but... > >> So err != 0 is always an error. Except err > 0 doesn't really provide > >> any useful information to the userspace. I cannot really remember what > >> was the actual behavior before my rework because there were some gotchas > >> hidden there. > > > >OK, so I've double checked. do_move_page_to_node_array would carry the > >error code over to do_pages_move and it would store the status stored > >in the pm array. It contains page_to_nid(page) so the resulting code > >indeed behaves properly before my change and this is a regression. I > > Thanks, I see the change. > > >have a very vague recollection that this has been brought up already. > ><...looks in notes...> > >Found it! The report is > >http://lkml.kernel.org/r/0329efa0984b9b0252ef166abb4498c0795fab36.1535113317.git.jstancek@redhat.com > >and my proposed workaround was http://lkml.kernel.org/r/20180829145537.GZ10223@dhcp22.suse.cz > > Well, the above two links return 404. You are right. They are not archived for some reason. Anyway, the patch I was proposing back then is below: commit cfb88c266b645197135cde2905c2bfc82f6d82a9 Author: Michal Hocko <mhocko@suse.com> Date: Wed Nov 14 12:19:09 2018 +0100 mm: fix do_pages_move error reporting a49bd4d71637 ("mm, numa: rework do_pages_move") has changed the way how we report error to layers above. As the changelog mentioned the semantic was quite unclear previously because the return 0 could mean both success and failure. The above mentioned commit didn't get all the way down to fix this completely because it doesn't report pages that we even haven't attempted to migrate and therefore we cannot simply say that the semantic is: - err < 0 - errno - err >= 0 number of non-migrated pages. Fixes: a49bd4d71637 ("mm, numa: rework do_pages_move") Signed-off-by: Michal Hocko <mhocko@suse.com> diff --git a/mm/migrate.c b/mm/migrate.c index f7e4bfdc13b7..aa53ebc523eb 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -1615,8 +1615,16 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, goto out_flush; err = do_move_pages_to_node(mm, &pagelist, current_node); - if (err) + if (err) { + /* + * Possitive err means the number of failed pages to + * migrate. Make sure to report the rest of the + * nr_pages is not migrated as well. + */ + if (err > 0) + err += nr_pages - i - 1; goto out; + } if (i > start) { err = store_status(status, start, current_node, i - start); if (err) -- Michal Hocko SUSE Labs ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] mm: move_pages: fix the return value if there are not-migrated pages 2020-01-21 8:40 ` Michal Hocko @ 2020-01-21 19:01 ` Yang Shi 2020-01-22 8:06 ` Michal Hocko 0 siblings, 1 reply; 14+ messages in thread From: Yang Shi @ 2020-01-21 19:01 UTC (permalink / raw) To: Michal Hocko, Wei Yang; +Cc: akpm, linux-mm, linux-kernel, stable On 1/21/20 12:40 AM, Michal Hocko wrote: > On Tue 21-01-20 09:44:16, Wei Yang wrote: >> On Mon, Jan 20, 2020 at 02:17:44PM +0100, Michal Hocko wrote: >>> On Mon 20-01-20 14:06:26, Michal Hocko wrote: >>>> On Sat 18-01-20 13:26:43, Yang Shi wrote: >>>>> The do_move_pages_to_node() might return > 0 value, the number of pages >>>>> that are not migrated, then the value will be returned to userspace >>>>> directly. But, move_pages() syscall would just return 0 or errno. So, >>>>> we need reset the return value to 0 for such case as what pre-v4.17 did. >>>> The patch is wrong. migrate_pages returns the number of pages it >>>> _hasn't_ migrated or -errno. Yeah that semantic sucks but... >>>> So err != 0 is always an error. Except err > 0 doesn't really provide >>>> any useful information to the userspace. I cannot really remember what >>>> was the actual behavior before my rework because there were some gotchas >>>> hidden there. >>> OK, so I've double checked. do_move_page_to_node_array would carry the >>> error code over to do_pages_move and it would store the status stored >>> in the pm array. It contains page_to_nid(page) so the resulting code >>> indeed behaves properly before my change and this is a regression. I >> Thanks, I see the change. >> >>> have a very vague recollection that this has been brought up already. >>> <...looks in notes...> >>> Found it! The report is >>> http://lkml.kernel.org/r/0329efa0984b9b0252ef166abb4498c0795fab36.1535113317.git.jstancek@redhat.com >>> and my proposed workaround was http://lkml.kernel.org/r/20180829145537.GZ10223@dhcp22.suse.cz >> Well, the above two links return 404. > You are right. They are not archived for some reason. Anyway, the patch > I was proposing back then is below: > > commit cfb88c266b645197135cde2905c2bfc82f6d82a9 > Author: Michal Hocko <mhocko@suse.com> > Date: Wed Nov 14 12:19:09 2018 +0100 > > mm: fix do_pages_move error reporting > > a49bd4d71637 ("mm, numa: rework do_pages_move") has changed the way how > we report error to layers above. As the changelog mentioned the semantic > was quite unclear previously because the return 0 could mean both > success and failure. > > The above mentioned commit didn't get all the way down to fix this > completely because it doesn't report pages that we even haven't > attempted to migrate and therefore we cannot simply say that the > semantic is: > - err < 0 - errno > - err >= 0 number of non-migrated pages. > > Fixes: a49bd4d71637 ("mm, numa: rework do_pages_move") > Signed-off-by: Michal Hocko <mhocko@suse.com> Thanks, Michal. But, it looks this patch still could return > 0 value (the total number of non-migrated pages, including not even attempted pages) too, but the problem we are trying to fix is to make do_pages_move() return <= 0 value only since the man page of move_pages() doesn't allow return > 0 value. And, by looking into the old code (v4.16), I spotted another problem. The migrate_pages() would store the migration failure error code into page_to_node->status. So, When do_move_page_to_node_array() returns > 0 value, the return value would be reset to 0 and the migration error codes for non-migrated pages would be stored into status to return to userspace. But, the rework removed this. I didn't dig into the intention of the rework, is it expected? > > diff --git a/mm/migrate.c b/mm/migrate.c > index f7e4bfdc13b7..aa53ebc523eb 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -1615,8 +1615,16 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, > goto out_flush; > > err = do_move_pages_to_node(mm, &pagelist, current_node); > - if (err) > + if (err) { > + /* > + * Possitive err means the number of failed pages to > + * migrate. Make sure to report the rest of the > + * nr_pages is not migrated as well. > + */ > + if (err > 0) > + err += nr_pages - i - 1; > goto out; > + } > if (i > start) { > err = store_status(status, start, current_node, i - start); > if (err) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mm: move_pages: fix the return value if there are not-migrated pages 2020-01-21 19:01 ` Yang Shi @ 2020-01-22 8:06 ` Michal Hocko 2020-01-22 17:26 ` Yang Shi 2020-01-23 3:29 ` Wei Yang 0 siblings, 2 replies; 14+ messages in thread From: Michal Hocko @ 2020-01-22 8:06 UTC (permalink / raw) To: Yang Shi; +Cc: Wei Yang, akpm, linux-mm, linux-kernel, stable On Tue 21-01-20 11:01:30, Yang Shi wrote: > > > On 1/21/20 12:40 AM, Michal Hocko wrote: > > On Tue 21-01-20 09:44:16, Wei Yang wrote: > > > On Mon, Jan 20, 2020 at 02:17:44PM +0100, Michal Hocko wrote: > > > > On Mon 20-01-20 14:06:26, Michal Hocko wrote: > > > > > On Sat 18-01-20 13:26:43, Yang Shi wrote: > > > > > > The do_move_pages_to_node() might return > 0 value, the number of pages > > > > > > that are not migrated, then the value will be returned to userspace > > > > > > directly. But, move_pages() syscall would just return 0 or errno. So, > > > > > > we need reset the return value to 0 for such case as what pre-v4.17 did. > > > > > The patch is wrong. migrate_pages returns the number of pages it > > > > > _hasn't_ migrated or -errno. Yeah that semantic sucks but... > > > > > So err != 0 is always an error. Except err > 0 doesn't really provide > > > > > any useful information to the userspace. I cannot really remember what > > > > > was the actual behavior before my rework because there were some gotchas > > > > > hidden there. > > > > OK, so I've double checked. do_move_page_to_node_array would carry the > > > > error code over to do_pages_move and it would store the status stored > > > > in the pm array. It contains page_to_nid(page) so the resulting code > > > > indeed behaves properly before my change and this is a regression. I > > > Thanks, I see the change. > > > > > > > have a very vague recollection that this has been brought up already. > > > > <...looks in notes...> > > > > Found it! The report is > > > > http://lkml.kernel.org/r/0329efa0984b9b0252ef166abb4498c0795fab36.1535113317.git.jstancek@redhat.com > > > > and my proposed workaround was http://lkml.kernel.org/r/20180829145537.GZ10223@dhcp22.suse.cz > > > Well, the above two links return 404. > > You are right. They are not archived for some reason. Anyway, the patch > > I was proposing back then is below: > > > > commit cfb88c266b645197135cde2905c2bfc82f6d82a9 > > Author: Michal Hocko <mhocko@suse.com> > > Date: Wed Nov 14 12:19:09 2018 +0100 > > > > mm: fix do_pages_move error reporting > > a49bd4d71637 ("mm, numa: rework do_pages_move") has changed the way how > > we report error to layers above. As the changelog mentioned the semantic > > was quite unclear previously because the return 0 could mean both > > success and failure. > > The above mentioned commit didn't get all the way down to fix this > > completely because it doesn't report pages that we even haven't > > attempted to migrate and therefore we cannot simply say that the > > semantic is: > > - err < 0 - errno > > - err >= 0 number of non-migrated pages. > > Fixes: a49bd4d71637 ("mm, numa: rework do_pages_move") > > Signed-off-by: Michal Hocko <mhocko@suse.com> > > Thanks, Michal. But, it looks this patch still could return > 0 value (the > total number of non-migrated pages, including not even attempted pages) too, > but the problem we are trying to fix is to make do_pages_move() return <= 0 > value only since the man page of move_pages() doesn't allow return > 0 > value. Yes this patch just lives with the changed semantic and tries to make it sensible. So if some page cannot be migrated then we just stop and return the number of non migrated pages at the tail of the given array. This would make error handling slightly easier because you know that count - ret pages of the array can be skipped if ret >= 0. > And, by looking into the old code (v4.16), I spotted another problem. The > migrate_pages() would store the migration failure error code into > page_to_node->status. So, When do_move_page_to_node_array() returns > 0 > value, the return value would be reset to 0 and the migration error codes > for non-migrated pages would be stored into status to return to userspace. > But, the rework removed this. > > I didn't dig into the intention of the rework, is it expected? I have tried to preserve the original semantic as possible. As explained in the changelog there were quite some discrepancies even before. This new one was not really intentional. We have effectively two options here. Either somebody really depend on the former semantic and we have to fix this or we can relax the semantic as the above patch attempts. I would be more inclined for the second option as nobody has complained about the new semantic except for few ltp tests which do not represent real workload. If you have a real usecase then speak up please. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mm: move_pages: fix the return value if there are not-migrated pages 2020-01-22 8:06 ` Michal Hocko @ 2020-01-22 17:26 ` Yang Shi 2020-01-22 17:35 ` Michal Hocko 2020-01-23 3:29 ` Wei Yang 1 sibling, 1 reply; 14+ messages in thread From: Yang Shi @ 2020-01-22 17:26 UTC (permalink / raw) To: Michal Hocko; +Cc: Wei Yang, akpm, linux-mm, linux-kernel, stable On 1/22/20 12:06 AM, Michal Hocko wrote: > On Tue 21-01-20 11:01:30, Yang Shi wrote: >> >> On 1/21/20 12:40 AM, Michal Hocko wrote: >>> On Tue 21-01-20 09:44:16, Wei Yang wrote: >>>> On Mon, Jan 20, 2020 at 02:17:44PM +0100, Michal Hocko wrote: >>>>> On Mon 20-01-20 14:06:26, Michal Hocko wrote: >>>>>> On Sat 18-01-20 13:26:43, Yang Shi wrote: >>>>>>> The do_move_pages_to_node() might return > 0 value, the number of pages >>>>>>> that are not migrated, then the value will be returned to userspace >>>>>>> directly. But, move_pages() syscall would just return 0 or errno. So, >>>>>>> we need reset the return value to 0 for such case as what pre-v4.17 did. >>>>>> The patch is wrong. migrate_pages returns the number of pages it >>>>>> _hasn't_ migrated or -errno. Yeah that semantic sucks but... >>>>>> So err != 0 is always an error. Except err > 0 doesn't really provide >>>>>> any useful information to the userspace. I cannot really remember what >>>>>> was the actual behavior before my rework because there were some gotchas >>>>>> hidden there. >>>>> OK, so I've double checked. do_move_page_to_node_array would carry the >>>>> error code over to do_pages_move and it would store the status stored >>>>> in the pm array. It contains page_to_nid(page) so the resulting code >>>>> indeed behaves properly before my change and this is a regression. I >>>> Thanks, I see the change. >>>> >>>>> have a very vague recollection that this has been brought up already. >>>>> <...looks in notes...> >>>>> Found it! The report is >>>>> http://lkml.kernel.org/r/0329efa0984b9b0252ef166abb4498c0795fab36.1535113317.git.jstancek@redhat.com >>>>> and my proposed workaround was http://lkml.kernel.org/r/20180829145537.GZ10223@dhcp22.suse.cz >>>> Well, the above two links return 404. >>> You are right. They are not archived for some reason. Anyway, the patch >>> I was proposing back then is below: >>> >>> commit cfb88c266b645197135cde2905c2bfc82f6d82a9 >>> Author: Michal Hocko <mhocko@suse.com> >>> Date: Wed Nov 14 12:19:09 2018 +0100 >>> >>> mm: fix do_pages_move error reporting >>> a49bd4d71637 ("mm, numa: rework do_pages_move") has changed the way how >>> we report error to layers above. As the changelog mentioned the semantic >>> was quite unclear previously because the return 0 could mean both >>> success and failure. >>> The above mentioned commit didn't get all the way down to fix this >>> completely because it doesn't report pages that we even haven't >>> attempted to migrate and therefore we cannot simply say that the >>> semantic is: >>> - err < 0 - errno >>> - err >= 0 number of non-migrated pages. >>> Fixes: a49bd4d71637 ("mm, numa: rework do_pages_move") >>> Signed-off-by: Michal Hocko <mhocko@suse.com> >> Thanks, Michal. But, it looks this patch still could return > 0 value (the >> total number of non-migrated pages, including not even attempted pages) too, >> but the problem we are trying to fix is to make do_pages_move() return <= 0 >> value only since the man page of move_pages() doesn't allow return > 0 >> value. > Yes this patch just lives with the changed semantic and tries to make it > sensible. So if some page cannot be migrated then we just stop and > return the number of non migrated pages at the tail of the given array. > This would make error handling slightly easier because you know that > count - ret pages of the array can be skipped if ret >= 0. OK, I see. Returning > 0 value sounds more straightforward for userspace error handling. BTW, we should update manpage to reflect the semantic change to indicate > 0 return value as an error case. > >> And, by looking into the old code (v4.16), I spotted another problem. The >> migrate_pages() would store the migration failure error code into >> page_to_node->status. So, When do_move_page_to_node_array() returns > 0 >> value, the return value would be reset to 0 and the migration error codes >> for non-migrated pages would be stored into status to return to userspace. >> But, the rework removed this. >> >> I didn't dig into the intention of the rework, is it expected? > I have tried to preserve the original semantic as possible. As explained > in the changelog there were quite some discrepancies even before. This > new one was not really intentional. We have effectively two options > here. Either somebody really depend on the former semantic and we have > to fix this or we can relax the semantic as the above patch attempts. > > I would be more inclined for the second option as nobody has complained > about the new semantic except for few ltp tests which do not represent > real workload. If you have a real usecase then speak up please. No, I don't have any real usecase. And, I tend to agree the most users may not care the reason of migration failure at all. Returning the number of non-migrated pages seems more straightforward. I agree we could stick with the new semantic and fix the return value as what your patch did. I'm going to rebase your patch on top of Wei Yang's cleanup if you don't mind. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mm: move_pages: fix the return value if there are not-migrated pages 2020-01-22 17:26 ` Yang Shi @ 2020-01-22 17:35 ` Michal Hocko 0 siblings, 0 replies; 14+ messages in thread From: Michal Hocko @ 2020-01-22 17:35 UTC (permalink / raw) To: Yang Shi; +Cc: Wei Yang, akpm, linux-mm, linux-kernel, stable On Wed 22-01-20 09:26:48, Yang Shi wrote: > > > On 1/22/20 12:06 AM, Michal Hocko wrote: > > On Tue 21-01-20 11:01:30, Yang Shi wrote: > > > > > > On 1/21/20 12:40 AM, Michal Hocko wrote: > > > > On Tue 21-01-20 09:44:16, Wei Yang wrote: > > > > > On Mon, Jan 20, 2020 at 02:17:44PM +0100, Michal Hocko wrote: > > > > > > On Mon 20-01-20 14:06:26, Michal Hocko wrote: > > > > > > > On Sat 18-01-20 13:26:43, Yang Shi wrote: > > > > > > > > The do_move_pages_to_node() might return > 0 value, the number of pages > > > > > > > > that are not migrated, then the value will be returned to userspace > > > > > > > > directly. But, move_pages() syscall would just return 0 or errno. So, > > > > > > > > we need reset the return value to 0 for such case as what pre-v4.17 did. > > > > > > > The patch is wrong. migrate_pages returns the number of pages it > > > > > > > _hasn't_ migrated or -errno. Yeah that semantic sucks but... > > > > > > > So err != 0 is always an error. Except err > 0 doesn't really provide > > > > > > > any useful information to the userspace. I cannot really remember what > > > > > > > was the actual behavior before my rework because there were some gotchas > > > > > > > hidden there. > > > > > > OK, so I've double checked. do_move_page_to_node_array would carry the > > > > > > error code over to do_pages_move and it would store the status stored > > > > > > in the pm array. It contains page_to_nid(page) so the resulting code > > > > > > indeed behaves properly before my change and this is a regression. I > > > > > Thanks, I see the change. > > > > > > > > > > > have a very vague recollection that this has been brought up already. > > > > > > <...looks in notes...> > > > > > > Found it! The report is > > > > > > http://lkml.kernel.org/r/0329efa0984b9b0252ef166abb4498c0795fab36.1535113317.git.jstancek@redhat.com > > > > > > and my proposed workaround was http://lkml.kernel.org/r/20180829145537.GZ10223@dhcp22.suse.cz > > > > > Well, the above two links return 404. > > > > You are right. They are not archived for some reason. Anyway, the patch > > > > I was proposing back then is below: > > > > > > > > commit cfb88c266b645197135cde2905c2bfc82f6d82a9 > > > > Author: Michal Hocko <mhocko@suse.com> > > > > Date: Wed Nov 14 12:19:09 2018 +0100 > > > > > > > > mm: fix do_pages_move error reporting > > > > a49bd4d71637 ("mm, numa: rework do_pages_move") has changed the way how > > > > we report error to layers above. As the changelog mentioned the semantic > > > > was quite unclear previously because the return 0 could mean both > > > > success and failure. > > > > The above mentioned commit didn't get all the way down to fix this > > > > completely because it doesn't report pages that we even haven't > > > > attempted to migrate and therefore we cannot simply say that the > > > > semantic is: > > > > - err < 0 - errno > > > > - err >= 0 number of non-migrated pages. > > > > Fixes: a49bd4d71637 ("mm, numa: rework do_pages_move") > > > > Signed-off-by: Michal Hocko <mhocko@suse.com> > > > Thanks, Michal. But, it looks this patch still could return > 0 value (the > > > total number of non-migrated pages, including not even attempted pages) too, > > > but the problem we are trying to fix is to make do_pages_move() return <= 0 > > > value only since the man page of move_pages() doesn't allow return > 0 > > > value. > > Yes this patch just lives with the changed semantic and tries to make it > > sensible. So if some page cannot be migrated then we just stop and > > return the number of non migrated pages at the tail of the given array. > > This would make error handling slightly easier because you know that > > count - ret pages of the array can be skipped if ret >= 0. > > OK, I see. Returning > 0 value sounds more straightforward for userspace > error handling. > > BTW, we should update manpage to reflect the semantic change to indicate > 0 > return value as an error case. Absolutely. > > > And, by looking into the old code (v4.16), I spotted another problem. The > > > migrate_pages() would store the migration failure error code into > > > page_to_node->status. So, When do_move_page_to_node_array() returns > 0 > > > value, the return value would be reset to 0 and the migration error codes > > > for non-migrated pages would be stored into status to return to userspace. > > > But, the rework removed this. > > > > > > I didn't dig into the intention of the rework, is it expected? > > I have tried to preserve the original semantic as possible. As explained > > in the changelog there were quite some discrepancies even before. This > > new one was not really intentional. We have effectively two options > > here. Either somebody really depend on the former semantic and we have > > to fix this or we can relax the semantic as the above patch attempts. > > > > I would be more inclined for the second option as nobody has complained > > about the new semantic except for few ltp tests which do not represent > > real workload. If you have a real usecase then speak up please. > > No, I don't have any real usecase. And, I tend to agree the most users may > not care the reason of migration failure at all. Returning the number of > non-migrated pages seems more straightforward. > > I agree we could stick with the new semantic and fix the return value as > what your patch did. I'm going to rebase your patch on top of Wei Yang's > cleanup if you don't mind. Go ahead. Thanks a lot! -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] mm: move_pages: fix the return value if there are not-migrated pages 2020-01-22 8:06 ` Michal Hocko 2020-01-22 17:26 ` Yang Shi @ 2020-01-23 3:29 ` Wei Yang 1 sibling, 0 replies; 14+ messages in thread From: Wei Yang @ 2020-01-23 3:29 UTC (permalink / raw) To: Michal Hocko; +Cc: Yang Shi, Wei Yang, akpm, linux-mm, linux-kernel, stable On Wed, Jan 22, 2020 at 09:06:51AM +0100, Michal Hocko wrote: >On Tue 21-01-20 11:01:30, Yang Shi wrote: >> >> >> On 1/21/20 12:40 AM, Michal Hocko wrote: >> > On Tue 21-01-20 09:44:16, Wei Yang wrote: >> > > On Mon, Jan 20, 2020 at 02:17:44PM +0100, Michal Hocko wrote: >> > > > On Mon 20-01-20 14:06:26, Michal Hocko wrote: >> > > > > On Sat 18-01-20 13:26:43, Yang Shi wrote: >> > > > > > The do_move_pages_to_node() might return > 0 value, the number of pages >> > > > > > that are not migrated, then the value will be returned to userspace >> > > > > > directly. But, move_pages() syscall would just return 0 or errno. So, >> > > > > > we need reset the return value to 0 for such case as what pre-v4.17 did. >> > > > > The patch is wrong. migrate_pages returns the number of pages it >> > > > > _hasn't_ migrated or -errno. Yeah that semantic sucks but... >> > > > > So err != 0 is always an error. Except err > 0 doesn't really provide >> > > > > any useful information to the userspace. I cannot really remember what >> > > > > was the actual behavior before my rework because there were some gotchas >> > > > > hidden there. >> > > > OK, so I've double checked. do_move_page_to_node_array would carry the >> > > > error code over to do_pages_move and it would store the status stored >> > > > in the pm array. It contains page_to_nid(page) so the resulting code >> > > > indeed behaves properly before my change and this is a regression. I >> > > Thanks, I see the change. >> > > >> > > > have a very vague recollection that this has been brought up already. >> > > > <...looks in notes...> >> > > > Found it! The report is >> > > > http://lkml.kernel.org/r/0329efa0984b9b0252ef166abb4498c0795fab36.1535113317.git.jstancek@redhat.com >> > > > and my proposed workaround was http://lkml.kernel.org/r/20180829145537.GZ10223@dhcp22.suse.cz >> > > Well, the above two links return 404. >> > You are right. They are not archived for some reason. Anyway, the patch >> > I was proposing back then is below: >> > >> > commit cfb88c266b645197135cde2905c2bfc82f6d82a9 >> > Author: Michal Hocko <mhocko@suse.com> >> > Date: Wed Nov 14 12:19:09 2018 +0100 >> > >> > mm: fix do_pages_move error reporting >> > a49bd4d71637 ("mm, numa: rework do_pages_move") has changed the way how >> > we report error to layers above. As the changelog mentioned the semantic >> > was quite unclear previously because the return 0 could mean both >> > success and failure. >> > The above mentioned commit didn't get all the way down to fix this >> > completely because it doesn't report pages that we even haven't >> > attempted to migrate and therefore we cannot simply say that the >> > semantic is: >> > - err < 0 - errno >> > - err >= 0 number of non-migrated pages. >> > Fixes: a49bd4d71637 ("mm, numa: rework do_pages_move") >> > Signed-off-by: Michal Hocko <mhocko@suse.com> >> >> Thanks, Michal. But, it looks this patch still could return > 0 value (the >> total number of non-migrated pages, including not even attempted pages) too, >> but the problem we are trying to fix is to make do_pages_move() return <= 0 >> value only since the man page of move_pages() doesn't allow return > 0 >> value. > >Yes this patch just lives with the changed semantic and tries to make it >sensible. So if some page cannot be migrated then we just stop and >return the number of non migrated pages at the tail of the given array. >This would make error handling slightly easier because you know that >count - ret pages of the array can be skipped if ret >= 0. > Got some different idea for this. Replied in the patch thread. >> And, by looking into the old code (v4.16), I spotted another problem. The >> migrate_pages() would store the migration failure error code into >> page_to_node->status. So, When do_move_page_to_node_array() returns > 0 >> value, the return value would be reset to 0 and the migration error codes >> for non-migrated pages would be stored into status to return to userspace. >> But, the rework removed this. >> >> I didn't dig into the intention of the rework, is it expected? > >I have tried to preserve the original semantic as possible. As explained >in the changelog there were quite some discrepancies even before. This >new one was not really intentional. We have effectively two options >here. Either somebody really depend on the former semantic and we have >to fix this or we can relax the semantic as the above patch attempts. > >I would be more inclined for the second option as nobody has complained >about the new semantic except for few ltp tests which do not represent >real workload. If you have a real usecase then speak up please. >-- >Michal Hocko >SUSE Labs -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-01-23 3:29 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-18 5:26 [PATCH] mm: move_pages: fix the return value if there are not-migrated pages Yang Shi 2020-01-19 2:37 ` Wei Yang 2020-01-19 2:57 ` Wei Yang 2020-01-19 5:47 ` Yang Shi 2020-01-19 5:44 ` Yang Shi 2020-01-20 13:06 ` Michal Hocko 2020-01-20 13:17 ` Michal Hocko 2020-01-21 1:44 ` Wei Yang 2020-01-21 8:40 ` Michal Hocko 2020-01-21 19:01 ` Yang Shi 2020-01-22 8:06 ` Michal Hocko 2020-01-22 17:26 ` Yang Shi 2020-01-22 17:35 ` Michal Hocko 2020-01-23 3:29 ` Wei Yang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).