| Message ID | 20200122011647.13636-4-richardw.yang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cleanup on do_pages_move() | expand |
On 22.01.20 02:16, Wei Yang wrote: > When pagelist is empty, it is not necessary to do the move and store. > Also it consolidate the empty list check in one place. > > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> > Acked-by: Michal Hocko <mhocko@suse.com> > --- > mm/migrate.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/mm/migrate.c b/mm/migrate.c > index a4d3bd6475e1..80d2bba57265 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -1499,9 +1499,6 @@ static int do_move_pages_to_node(struct mm_struct *mm, > { > int err; > > - if (list_empty(pagelist)) > - return 0; > - > err = migrate_pages(pagelist, alloc_new_node_page, NULL, node, > MIGRATE_SYNC, MR_SYSCALL); > if (err) > @@ -1589,6 +1586,9 @@ static int move_pages_and_store_status(struct mm_struct *mm, int node, > { > int err; > > + if (list_empty(pagelist)) > + return 0; > + > err = do_move_pages_to_node(mm, pagelist, node); > if (err) > return err; > @@ -1676,9 +1676,6 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, > current_node = NUMA_NO_NODE; > } > out_flush: > - if (list_empty(&pagelist)) > - return err; Am I wrong or are you discarding an error here? (err could be !0)
On Tue, Jan 28, 2020 at 11:21:13AM +0100, David Hildenbrand wrote: >On 22.01.20 02:16, Wei Yang wrote: >> When pagelist is empty, it is not necessary to do the move and store. >> Also it consolidate the empty list check in one place. >> >> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> >> Acked-by: Michal Hocko <mhocko@suse.com> >> --- >> mm/migrate.c | 9 +++------ >> 1 file changed, 3 insertions(+), 6 deletions(-) >> >> diff --git a/mm/migrate.c b/mm/migrate.c >> index a4d3bd6475e1..80d2bba57265 100644 >> --- a/mm/migrate.c >> +++ b/mm/migrate.c >> @@ -1499,9 +1499,6 @@ static int do_move_pages_to_node(struct mm_struct *mm, >> { >> int err; >> >> - if (list_empty(pagelist)) >> - return 0; >> - >> err = migrate_pages(pagelist, alloc_new_node_page, NULL, node, >> MIGRATE_SYNC, MR_SYSCALL); >> if (err) >> @@ -1589,6 +1586,9 @@ static int move_pages_and_store_status(struct mm_struct *mm, int node, >> { >> int err; >> >> + if (list_empty(pagelist)) >> + return 0; >> + >> err = do_move_pages_to_node(mm, pagelist, node); >> if (err) >> return err; >> @@ -1676,9 +1676,6 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, >> current_node = NUMA_NO_NODE; >> } >> out_flush: >> - if (list_empty(&pagelist)) >> - return err; > > >Am I wrong or are you discarding an error here? (err could be !0) > This is not obvious in this code snippet. If you look into the source code, it might help you get the whole picture. The following comment says: Make sure we do not overwrite the existing error So we didn't eat error here. > >-- >Thanks, > >David / dhildenb
On 29.01.20 01:46, Wei Yang wrote: > On Tue, Jan 28, 2020 at 11:21:13AM +0100, David Hildenbrand wrote: >> On 22.01.20 02:16, Wei Yang wrote: >>> When pagelist is empty, it is not necessary to do the move and store. >>> Also it consolidate the empty list check in one place. >>> >>> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> >>> Acked-by: Michal Hocko <mhocko@suse.com> >>> --- >>> mm/migrate.c | 9 +++------ >>> 1 file changed, 3 insertions(+), 6 deletions(-) >>> >>> diff --git a/mm/migrate.c b/mm/migrate.c >>> index a4d3bd6475e1..80d2bba57265 100644 >>> --- a/mm/migrate.c >>> +++ b/mm/migrate.c >>> @@ -1499,9 +1499,6 @@ static int do_move_pages_to_node(struct mm_struct *mm, >>> { >>> int err; >>> >>> - if (list_empty(pagelist)) >>> - return 0; >>> - >>> err = migrate_pages(pagelist, alloc_new_node_page, NULL, node, >>> MIGRATE_SYNC, MR_SYSCALL); >>> if (err) >>> @@ -1589,6 +1586,9 @@ static int move_pages_and_store_status(struct mm_struct *mm, int node, >>> { >>> int err; >>> >>> + if (list_empty(pagelist)) >>> + return 0; >>> + >>> err = do_move_pages_to_node(mm, pagelist, node); >>> if (err) >>> return err; >>> @@ -1676,9 +1676,6 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, >>> current_node = NUMA_NO_NODE; >>> } >>> out_flush: >>> - if (list_empty(&pagelist)) >>> - return err; >> >> >> Am I wrong or are you discarding an error here? (err could be !0) >> > > This is not obvious in this code snippet. If you look into the source code, it > might help you get the whole picture. > > The following comment says: > > Make sure we do not overwrite the existing error > > So we didn't eat error here. My fault, I misread the "if (err >= 0)" (and thought it was a "if (err1 >= 0)") Makes perfect sense, thanks! Reviewed-by: David Hildenbrand <david@redhat.com>
diff --git a/mm/migrate.c b/mm/migrate.c index a4d3bd6475e1..80d2bba57265 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -1499,9 +1499,6 @@ static int do_move_pages_to_node(struct mm_struct *mm, { int err; - if (list_empty(pagelist)) - return 0; - err = migrate_pages(pagelist, alloc_new_node_page, NULL, node, MIGRATE_SYNC, MR_SYSCALL); if (err) @@ -1589,6 +1586,9 @@ static int move_pages_and_store_status(struct mm_struct *mm, int node, { int err; + if (list_empty(pagelist)) + return 0; + err = do_move_pages_to_node(mm, pagelist, node); if (err) return err; @@ -1676,9 +1676,6 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes, current_node = NUMA_NO_NODE; } out_flush: - if (list_empty(&pagelist)) - return err; - /* Make sure we do not overwrite the existing error */ err1 = move_pages_and_store_status(mm, current_node, &pagelist, status, start, i - start);