| Message ID | 20210401183223.80F1E291@viggo.jf.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Migrate Pages in lieu of discard | expand |
A small suggestion: Given that migrate_pages() requires that *nr_succeeded should be initialized to 0 when it is called due to its use of *nr_succeeded in count_vm_events() and trace_mm_migrate_pages(), it would be less error-prone if migrate_pages() initializes *nr_succeeded itself. On Thu, Apr 1, 2021 at 11:35 AM Dave Hansen <dave.hansen@linux.intel.com> wrote: > > From: Yang Shi <yang.shi@linux.alibaba.com> > > The migrate_pages() returns the number of pages that were not migrated, > or an error code. When returning an error code, there is no way to know > how many pages were migrated or not migrated. > > In the following patch, migrate_pages() is used to demote pages to PMEM > node, we need account how many pages are reclaimed (demoted) since page > reclaim behavior depends on this. Add *nr_succeeded parameter to make > migrate_pages() return how many pages are demoted successfully for all > cases. > > Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> > Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com> > Reviewed-by: Yang Shi <shy828301@gmail.com> > Cc: Wei Xu <weixugc@google.com> > Cc: Huang Ying <ying.huang@intel.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: David Hildenbrand <david@redhat.com> > Cc: osalvador <osalvador@suse.de> > > -- > > Note: Yang Shi originally wrote the patch, thus the SoB. There was > also a Reviewed-by provided since there were some modifications made > to this after the original work. > > Changes since 20210302: > * Fix definition of CONFIG_MIGRATION=n stub migrate_pages(). Its > parameters were wrong, but oddly enough did not generate any > compile errors. > > Changes since 20200122: > * Fix migrate_pages() to manipulate nr_succeeded *value* > rather than the pointer. > --- > > b/include/linux/migrate.h | 5 +++-- > b/mm/compaction.c | 3 ++- > b/mm/gup.c | 3 ++- > b/mm/memory-failure.c | 4 +++- > b/mm/memory_hotplug.c | 4 +++- > b/mm/mempolicy.c | 8 ++++++-- > b/mm/migrate.c | 19 +++++++++++-------- > b/mm/page_alloc.c | 9 ++++++--- > 8 files changed, 36 insertions(+), 19 deletions(-) > > diff -puN include/linux/migrate.h~migrate_pages-add-success-return > include/linux/migrate.h > --- a/include/linux/migrate.h~migrate_pages-add-success-return 2021-03-31 > 15:17:14.144000255 -0700 > +++ b/include/linux/migrate.h 2021-03-31 15:17:14.182000255 -0700 > @@ -40,7 +40,8 @@ extern int migrate_page(struct address_s > struct page *newpage, struct page *page, > enum migrate_mode mode); > extern int migrate_pages(struct list_head *l, new_page_t new, free_page_t > free, > - unsigned long private, enum migrate_mode mode, int reason); > + unsigned long private, enum migrate_mode mode, int reason, > + unsigned int *nr_succeeded); > extern struct page *alloc_migration_target(struct page *page, unsigned > long private); > extern int isolate_movable_page(struct page *page, isolate_mode_t mode); > extern void putback_movable_page(struct page *page); > @@ -58,7 +59,7 @@ extern int migrate_page_move_mapping(str > static inline void putback_movable_pages(struct list_head *l) {} > static inline int migrate_pages(struct list_head *l, new_page_t new, > free_page_t free, unsigned long private, enum migrate_mode > mode, > - int reason) > + int reason, unsigned int *nr_succeeded) > { return -ENOSYS; } > static inline struct page *alloc_migration_target(struct page *page, > unsigned long private) > diff -puN mm/compaction.c~migrate_pages-add-success-return mm/compaction.c > --- a/mm/compaction.c~migrate_pages-add-success-return 2021-03-31 > 15:17:14.146000255 -0700 > +++ b/mm/compaction.c 2021-03-31 15:17:14.186000255 -0700 > @@ -2247,6 +2247,7 @@ compact_zone(struct compact_control *cc, > unsigned long last_migrated_pfn; > const bool sync = cc->mode != MIGRATE_ASYNC; > bool update_cached; > + unsigned int nr_succeeded = 0; > > /* > * These counters track activities during zone compaction. > Initialize > @@ -2364,7 +2365,7 @@ compact_zone(struct compact_control *cc, > > err = migrate_pages(&cc->migratepages, compaction_alloc, > compaction_free, (unsigned long)cc, > cc->mode, > - MR_COMPACTION); > + MR_COMPACTION, &nr_succeeded); > > trace_mm_compaction_migratepages(cc->nr_migratepages, err, > &cc->migratepages); > diff -puN mm/gup.c~migrate_pages-add-success-return mm/gup.c > --- a/mm/gup.c~migrate_pages-add-success-return 2021-03-31 > 15:17:14.150000255 -0700 > +++ b/mm/gup.c 2021-03-31 15:17:14.190000255 -0700 > @@ -1550,6 +1550,7 @@ static long check_and_migrate_cma_pages( > unsigned long i; > unsigned long step; > bool drain_allow = true; > + unsigned int nr_succeeded = 0; > bool migrate_allow = true; > LIST_HEAD(cma_page_list); > long ret = nr_pages; > @@ -1606,7 +1607,7 @@ check_again: > put_page(pages[i]); > > if (migrate_pages(&cma_page_list, alloc_migration_target, > NULL, > - (unsigned long)&mtc, MIGRATE_SYNC, > MR_CONTIG_RANGE)) { > + (unsigned long)&mtc, MIGRATE_SYNC, > MR_CONTIG_RANGE, &nr_succeeded)) { > /* > * some of the pages failed migration. Do > get_user_pages > * without migration. > diff -puN mm/memory-failure.c~migrate_pages-add-success-return > mm/memory-failure.c > --- a/mm/memory-failure.c~migrate_pages-add-success-return 2021-03-31 > 15:17:14.155000255 -0700 > +++ b/mm/memory-failure.c 2021-03-31 15:17:14.194000255 -0700 > @@ -1809,6 +1809,7 @@ static int __soft_offline_page(struct pa > unsigned long pfn = page_to_pfn(page); > struct page *hpage = compound_head(page); > char const *msg_page[] = {"page", "hugepage"}; > + unsigned int nr_succeeded = 0; > bool huge = PageHuge(page); > LIST_HEAD(pagelist); > struct migration_target_control mtc = { > @@ -1852,7 +1853,8 @@ static int __soft_offline_page(struct pa > > if (isolate_page(hpage, &pagelist)) { > ret = migrate_pages(&pagelist, alloc_migration_target, > NULL, > - (unsigned long)&mtc, MIGRATE_SYNC, > MR_MEMORY_FAILURE); > + (unsigned long)&mtc, MIGRATE_SYNC, > MR_MEMORY_FAILURE, > + &nr_succeeded); > if (!ret) { > bool release = !huge; > > diff -puN mm/memory_hotplug.c~migrate_pages-add-success-return > mm/memory_hotplug.c > --- a/mm/memory_hotplug.c~migrate_pages-add-success-return 2021-03-31 > 15:17:14.160000255 -0700 > +++ b/mm/memory_hotplug.c 2021-03-31 15:17:14.197000255 -0700 > @@ -1392,6 +1392,7 @@ do_migrate_range(unsigned long start_pfn > unsigned long pfn; > struct page *page, *head; > int ret = 0; > + unsigned int nr_succeeded = 0; > LIST_HEAD(source); > > for (pfn = start_pfn; pfn < end_pfn; pfn++) { > @@ -1466,7 +1467,8 @@ do_migrate_range(unsigned long start_pfn > if (nodes_empty(nmask)) > node_set(mtc.nid, nmask); > ret = migrate_pages(&source, alloc_migration_target, NULL, > - (unsigned long)&mtc, MIGRATE_SYNC, > MR_MEMORY_HOTPLUG); > + (unsigned long)&mtc, MIGRATE_SYNC, > MR_MEMORY_HOTPLUG, > + &nr_succeeded); > if (ret) { > list_for_each_entry(page, &source, lru) { > pr_warn("migrating pfn %lx failed ret:%d ", > diff -puN mm/mempolicy.c~migrate_pages-add-success-return mm/mempolicy.c > --- a/mm/mempolicy.c~migrate_pages-add-success-return 2021-03-31 > 15:17:14.163000255 -0700 > +++ b/mm/mempolicy.c 2021-03-31 15:17:14.203000255 -0700 > @@ -1081,6 +1081,7 @@ static int migrate_page_add(struct page > static int migrate_to_node(struct mm_struct *mm, int source, int dest, > int flags) > { > + unsigned int nr_succeeded = 0; > nodemask_t nmask; > LIST_HEAD(pagelist); > int err = 0; > @@ -1103,7 +1104,8 @@ static int migrate_to_node(struct mm_str > > if (!list_empty(&pagelist)) { > err = migrate_pages(&pagelist, alloc_migration_target, > NULL, > - (unsigned long)&mtc, MIGRATE_SYNC, > MR_SYSCALL); > + (unsigned long)&mtc, MIGRATE_SYNC, > MR_SYSCALL, > + &nr_succeeded); > if (err) > putback_movable_pages(&pagelist); > } > @@ -1278,6 +1280,7 @@ static long do_mbind(unsigned long start > nodemask_t *nmask, unsigned long flags) > { > struct mm_struct *mm = current->mm; > + unsigned int nr_succeeded = 0; > struct mempolicy *new; > unsigned long end; > int err; > @@ -1355,7 +1358,8 @@ static long do_mbind(unsigned long start > if (!list_empty(&pagelist)) { > WARN_ON_ONCE(flags & MPOL_MF_LAZY); > nr_failed = migrate_pages(&pagelist, new_page, > NULL, > - start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND); > + start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND, > + &nr_succeeded); > if (nr_failed) > putback_movable_pages(&pagelist); > } > diff -puN mm/migrate.c~migrate_pages-add-success-return mm/migrate.c > --- a/mm/migrate.c~migrate_pages-add-success-return 2021-03-31 > 15:17:14.168000255 -0700 > +++ b/mm/migrate.c 2021-03-31 15:17:14.207000255 -0700 > @@ -1493,6 +1493,7 @@ static inline int try_split_thp(struct p > * @mode: The migration mode that specifies the constraints > for > * page migration, if any. > * @reason: The reason for page migration. > + * @nr_succeeded: The number of pages migrated successfully. > * > * The function returns after 10 attempts or if no pages are movable any > more > * because the list has become empty or no retryable pages exist any more. > @@ -1503,12 +1504,11 @@ static inline int try_split_thp(struct p > */ > int migrate_pages(struct list_head *from, new_page_t get_new_page, > free_page_t put_new_page, unsigned long private, > - enum migrate_mode mode, int reason) > + enum migrate_mode mode, int reason, unsigned int > *nr_succeeded) > { > int retry = 1; > int thp_retry = 1; > int nr_failed = 0; > - int nr_succeeded = 0; > int nr_thp_succeeded = 0; > int nr_thp_failed = 0; > int nr_thp_split = 0; > @@ -1611,10 +1611,10 @@ retry: > case MIGRATEPAGE_SUCCESS: > if (is_thp) { > nr_thp_succeeded++; > - nr_succeeded += nr_subpages; > + *nr_succeeded += nr_subpages; > break; > } > - nr_succeeded++; > + (*nr_succeeded)++; > break; > default: > /* > @@ -1643,12 +1643,12 @@ out: > */ > list_splice(&ret_pages, from); > > - count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded); > + count_vm_events(PGMIGRATE_SUCCESS, *nr_succeeded); > count_vm_events(PGMIGRATE_FAIL, nr_failed); > count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded); > count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed); > count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split); > - trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded, > + trace_mm_migrate_pages(*nr_succeeded, nr_failed, nr_thp_succeeded, > nr_thp_failed, nr_thp_split, mode, reason); > > if (!swapwrite) > @@ -1716,6 +1716,7 @@ static int store_status(int __user *stat > static int do_move_pages_to_node(struct mm_struct *mm, > struct list_head *pagelist, int node) > { > + unsigned int nr_succeeded = 0; > int err; > struct migration_target_control mtc = { > .nid = node, > @@ -1723,7 +1724,8 @@ static int do_move_pages_to_node(struct > }; > > err = migrate_pages(pagelist, alloc_migration_target, NULL, > - (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL); > + (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL, > + &nr_succeeded); > if (err) > putback_movable_pages(pagelist); > return err; > @@ -2207,6 +2209,7 @@ int migrate_misplaced_page(struct page * > pg_data_t *pgdat = NODE_DATA(node); > int isolated; > int nr_remaining; > + unsigned int nr_succeeded = 0; > LIST_HEAD(migratepages); > > /* > @@ -2230,7 +2233,7 @@ int migrate_misplaced_page(struct page * > list_add(&page->lru, &migratepages); > nr_remaining = migrate_pages(&migratepages, > alloc_misplaced_dst_page, > NULL, node, MIGRATE_ASYNC, > - MR_NUMA_MISPLACED); > + MR_NUMA_MISPLACED, &nr_succeeded); > if (nr_remaining) { > if (!list_empty(&migratepages)) { > list_del(&page->lru); > diff -puN mm/page_alloc.c~migrate_pages-add-success-return mm/page_alloc.c > --- a/mm/page_alloc.c~migrate_pages-add-success-return 2021-03-31 > 15:17:14.178000255 -0700 > +++ b/mm/page_alloc.c 2021-03-31 15:17:14.213000255 -0700 > @@ -8452,7 +8452,8 @@ static unsigned long pfn_max_align_up(un > > /* [start, end) must belong to a single zone. */ > static int __alloc_contig_migrate_range(struct compact_control *cc, > - unsigned long start, unsigned long > end) > + unsigned long start, unsigned long > end, > + unsigned int *nr_succeeded) > { > /* This function is based on compact_zone() from compaction.c. */ > unsigned int nr_reclaimed; > @@ -8490,7 +8491,8 @@ static int __alloc_contig_migrate_range( > cc->nr_migratepages -= nr_reclaimed; > > ret = migrate_pages(&cc->migratepages, > alloc_migration_target, > - NULL, (unsigned long)&mtc, cc->mode, > MR_CONTIG_RANGE); > + NULL, (unsigned long)&mtc, cc->mode, > MR_CONTIG_RANGE, > + nr_succeeded); > } > if (ret < 0) { > putback_movable_pages(&cc->migratepages); > @@ -8526,6 +8528,7 @@ int alloc_contig_range(unsigned long sta > unsigned long outer_start, outer_end; > unsigned int order; > int ret = 0; > + unsigned int nr_succeeded = 0; > > struct compact_control cc = { > .nr_migratepages = 0, > @@ -8580,7 +8583,7 @@ int alloc_contig_range(unsigned long sta > * allocated. So, if we fall through be sure to clear ret so that > * -EBUSY is not accidentally used or returned to caller. > */ > - ret = __alloc_contig_migrate_range(&cc, start, end); > + ret = __alloc_contig_migrate_range(&cc, start, end, &nr_succeeded); > if (ret && ret != -EBUSY) > goto done; > ret =0; > _ >
A small suggestion: Given that migrate_pages() requires that *nr_succeeded should be initialized to 0 when it is called due to its use of *nr_succeeded in count_vm_events() and trace_mm_migrate_pages(), it would be less error-prone if migrate_pages() initializes *nr_succeeded itself. On Thu, Apr 1, 2021 at 11:35 AM Dave Hansen <dave.hansen@linux.intel.com> wrote: > > > From: Yang Shi <yang.shi@linux.alibaba.com> > > The migrate_pages() returns the number of pages that were not migrated, > or an error code. When returning an error code, there is no way to know > how many pages were migrated or not migrated. > > In the following patch, migrate_pages() is used to demote pages to PMEM > node, we need account how many pages are reclaimed (demoted) since page > reclaim behavior depends on this. Add *nr_succeeded parameter to make > migrate_pages() return how many pages are demoted successfully for all > cases. > > Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> > Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com> > Reviewed-by: Yang Shi <shy828301@gmail.com> > Cc: Wei Xu <weixugc@google.com> > Cc: Huang Ying <ying.huang@intel.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: David Hildenbrand <david@redhat.com> > Cc: osalvador <osalvador@suse.de> > > -- > > Note: Yang Shi originally wrote the patch, thus the SoB. There was > also a Reviewed-by provided since there were some modifications made > to this after the original work. > > Changes since 20210302: > * Fix definition of CONFIG_MIGRATION=n stub migrate_pages(). Its > parameters were wrong, but oddly enough did not generate any > compile errors. > > Changes since 20200122: > * Fix migrate_pages() to manipulate nr_succeeded *value* > rather than the pointer. > --- > > b/include/linux/migrate.h | 5 +++-- > b/mm/compaction.c | 3 ++- > b/mm/gup.c | 3 ++- > b/mm/memory-failure.c | 4 +++- > b/mm/memory_hotplug.c | 4 +++- > b/mm/mempolicy.c | 8 ++++++-- > b/mm/migrate.c | 19 +++++++++++-------- > b/mm/page_alloc.c | 9 ++++++--- > 8 files changed, 36 insertions(+), 19 deletions(-) > > diff -puN include/linux/migrate.h~migrate_pages-add-success-return include/linux/migrate.h > --- a/include/linux/migrate.h~migrate_pages-add-success-return 2021-03-31 15:17:14.144000255 -0700 > +++ b/include/linux/migrate.h 2021-03-31 15:17:14.182000255 -0700 > @@ -40,7 +40,8 @@ extern int migrate_page(struct address_s > struct page *newpage, struct page *page, > enum migrate_mode mode); > extern int migrate_pages(struct list_head *l, new_page_t new, free_page_t free, > - unsigned long private, enum migrate_mode mode, int reason); > + unsigned long private, enum migrate_mode mode, int reason, > + unsigned int *nr_succeeded); > extern struct page *alloc_migration_target(struct page *page, unsigned long private); > extern int isolate_movable_page(struct page *page, isolate_mode_t mode); > extern void putback_movable_page(struct page *page); > @@ -58,7 +59,7 @@ extern int migrate_page_move_mapping(str > static inline void putback_movable_pages(struct list_head *l) {} > static inline int migrate_pages(struct list_head *l, new_page_t new, > free_page_t free, unsigned long private, enum migrate_mode mode, > - int reason) > + int reason, unsigned int *nr_succeeded) > { return -ENOSYS; } > static inline struct page *alloc_migration_target(struct page *page, > unsigned long private) > diff -puN mm/compaction.c~migrate_pages-add-success-return mm/compaction.c > --- a/mm/compaction.c~migrate_pages-add-success-return 2021-03-31 15:17:14.146000255 -0700 > +++ b/mm/compaction.c 2021-03-31 15:17:14.186000255 -0700 > @@ -2247,6 +2247,7 @@ compact_zone(struct compact_control *cc, > unsigned long last_migrated_pfn; > const bool sync = cc->mode != MIGRATE_ASYNC; > bool update_cached; > + unsigned int nr_succeeded = 0; > > /* > * These counters track activities during zone compaction. Initialize > @@ -2364,7 +2365,7 @@ compact_zone(struct compact_control *cc, > > err = migrate_pages(&cc->migratepages, compaction_alloc, > compaction_free, (unsigned long)cc, cc->mode, > - MR_COMPACTION); > + MR_COMPACTION, &nr_succeeded); > > trace_mm_compaction_migratepages(cc->nr_migratepages, err, > &cc->migratepages); > diff -puN mm/gup.c~migrate_pages-add-success-return mm/gup.c > --- a/mm/gup.c~migrate_pages-add-success-return 2021-03-31 15:17:14.150000255 -0700 > +++ b/mm/gup.c 2021-03-31 15:17:14.190000255 -0700 > @@ -1550,6 +1550,7 @@ static long check_and_migrate_cma_pages( > unsigned long i; > unsigned long step; > bool drain_allow = true; > + unsigned int nr_succeeded = 0; > bool migrate_allow = true; > LIST_HEAD(cma_page_list); > long ret = nr_pages; > @@ -1606,7 +1607,7 @@ check_again: > put_page(pages[i]); > > if (migrate_pages(&cma_page_list, alloc_migration_target, NULL, > - (unsigned long)&mtc, MIGRATE_SYNC, MR_CONTIG_RANGE)) { > + (unsigned long)&mtc, MIGRATE_SYNC, MR_CONTIG_RANGE, &nr_succeeded)) { > /* > * some of the pages failed migration. Do get_user_pages > * without migration. > diff -puN mm/memory-failure.c~migrate_pages-add-success-return mm/memory-failure.c > --- a/mm/memory-failure.c~migrate_pages-add-success-return 2021-03-31 15:17:14.155000255 -0700 > +++ b/mm/memory-failure.c 2021-03-31 15:17:14.194000255 -0700 > @@ -1809,6 +1809,7 @@ static int __soft_offline_page(struct pa > unsigned long pfn = page_to_pfn(page); > struct page *hpage = compound_head(page); > char const *msg_page[] = {"page", "hugepage"}; > + unsigned int nr_succeeded = 0; > bool huge = PageHuge(page); > LIST_HEAD(pagelist); > struct migration_target_control mtc = { > @@ -1852,7 +1853,8 @@ static int __soft_offline_page(struct pa > > if (isolate_page(hpage, &pagelist)) { > ret = migrate_pages(&pagelist, alloc_migration_target, NULL, > - (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE); > + (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE, > + &nr_succeeded); > if (!ret) { > bool release = !huge; > > diff -puN mm/memory_hotplug.c~migrate_pages-add-success-return mm/memory_hotplug.c > --- a/mm/memory_hotplug.c~migrate_pages-add-success-return 2021-03-31 15:17:14.160000255 -0700 > +++ b/mm/memory_hotplug.c 2021-03-31 15:17:14.197000255 -0700 > @@ -1392,6 +1392,7 @@ do_migrate_range(unsigned long start_pfn > unsigned long pfn; > struct page *page, *head; > int ret = 0; > + unsigned int nr_succeeded = 0; > LIST_HEAD(source); > > for (pfn = start_pfn; pfn < end_pfn; pfn++) { > @@ -1466,7 +1467,8 @@ do_migrate_range(unsigned long start_pfn > if (nodes_empty(nmask)) > node_set(mtc.nid, nmask); > ret = migrate_pages(&source, alloc_migration_target, NULL, > - (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_HOTPLUG); > + (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_HOTPLUG, > + &nr_succeeded); > if (ret) { > list_for_each_entry(page, &source, lru) { > pr_warn("migrating pfn %lx failed ret:%d ", > diff -puN mm/mempolicy.c~migrate_pages-add-success-return mm/mempolicy.c > --- a/mm/mempolicy.c~migrate_pages-add-success-return 2021-03-31 15:17:14.163000255 -0700 > +++ b/mm/mempolicy.c 2021-03-31 15:17:14.203000255 -0700 > @@ -1081,6 +1081,7 @@ static int migrate_page_add(struct page > static int migrate_to_node(struct mm_struct *mm, int source, int dest, > int flags) > { > + unsigned int nr_succeeded = 0; > nodemask_t nmask; > LIST_HEAD(pagelist); > int err = 0; > @@ -1103,7 +1104,8 @@ static int migrate_to_node(struct mm_str > > if (!list_empty(&pagelist)) { > err = migrate_pages(&pagelist, alloc_migration_target, NULL, > - (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL); > + (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL, > + &nr_succeeded); > if (err) > putback_movable_pages(&pagelist); > } > @@ -1278,6 +1280,7 @@ static long do_mbind(unsigned long start > nodemask_t *nmask, unsigned long flags) > { > struct mm_struct *mm = current->mm; > + unsigned int nr_succeeded = 0; > struct mempolicy *new; > unsigned long end; > int err; > @@ -1355,7 +1358,8 @@ static long do_mbind(unsigned long start > if (!list_empty(&pagelist)) { > WARN_ON_ONCE(flags & MPOL_MF_LAZY); > nr_failed = migrate_pages(&pagelist, new_page, NULL, > - start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND); > + start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND, > + &nr_succeeded); > if (nr_failed) > putback_movable_pages(&pagelist); > } > diff -puN mm/migrate.c~migrate_pages-add-success-return mm/migrate.c > --- a/mm/migrate.c~migrate_pages-add-success-return 2021-03-31 15:17:14.168000255 -0700 > +++ b/mm/migrate.c 2021-03-31 15:17:14.207000255 -0700 > @@ -1493,6 +1493,7 @@ static inline int try_split_thp(struct p > * @mode: The migration mode that specifies the constraints for > * page migration, if any. > * @reason: The reason for page migration. > + * @nr_succeeded: The number of pages migrated successfully. > * > * The function returns after 10 attempts or if no pages are movable any more > * because the list has become empty or no retryable pages exist any more. > @@ -1503,12 +1504,11 @@ static inline int try_split_thp(struct p > */ > int migrate_pages(struct list_head *from, new_page_t get_new_page, > free_page_t put_new_page, unsigned long private, > - enum migrate_mode mode, int reason) > + enum migrate_mode mode, int reason, unsigned int *nr_succeeded) > { > int retry = 1; > int thp_retry = 1; > int nr_failed = 0; > - int nr_succeeded = 0; > int nr_thp_succeeded = 0; > int nr_thp_failed = 0; > int nr_thp_split = 0; > @@ -1611,10 +1611,10 @@ retry: > case MIGRATEPAGE_SUCCESS: > if (is_thp) { > nr_thp_succeeded++; > - nr_succeeded += nr_subpages; > + *nr_succeeded += nr_subpages; > break; > } > - nr_succeeded++; > + (*nr_succeeded)++; > break; > default: > /* > @@ -1643,12 +1643,12 @@ out: > */ > list_splice(&ret_pages, from); > > - count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded); > + count_vm_events(PGMIGRATE_SUCCESS, *nr_succeeded); > count_vm_events(PGMIGRATE_FAIL, nr_failed); > count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded); > count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed); > count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split); > - trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded, > + trace_mm_migrate_pages(*nr_succeeded, nr_failed, nr_thp_succeeded, > nr_thp_failed, nr_thp_split, mode, reason); > > if (!swapwrite) > @@ -1716,6 +1716,7 @@ static int store_status(int __user *stat > static int do_move_pages_to_node(struct mm_struct *mm, > struct list_head *pagelist, int node) > { > + unsigned int nr_succeeded = 0; > int err; > struct migration_target_control mtc = { > .nid = node, > @@ -1723,7 +1724,8 @@ static int do_move_pages_to_node(struct > }; > > err = migrate_pages(pagelist, alloc_migration_target, NULL, > - (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL); > + (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL, > + &nr_succeeded); > if (err) > putback_movable_pages(pagelist); > return err; > @@ -2207,6 +2209,7 @@ int migrate_misplaced_page(struct page * > pg_data_t *pgdat = NODE_DATA(node); > int isolated; > int nr_remaining; > + unsigned int nr_succeeded = 0; > LIST_HEAD(migratepages); > > /* > @@ -2230,7 +2233,7 @@ int migrate_misplaced_page(struct page * > list_add(&page->lru, &migratepages); > nr_remaining = migrate_pages(&migratepages, alloc_misplaced_dst_page, > NULL, node, MIGRATE_ASYNC, > - MR_NUMA_MISPLACED); > + MR_NUMA_MISPLACED, &nr_succeeded); > if (nr_remaining) { > if (!list_empty(&migratepages)) { > list_del(&page->lru); > diff -puN mm/page_alloc.c~migrate_pages-add-success-return mm/page_alloc.c > --- a/mm/page_alloc.c~migrate_pages-add-success-return 2021-03-31 15:17:14.178000255 -0700 > +++ b/mm/page_alloc.c 2021-03-31 15:17:14.213000255 -0700 > @@ -8452,7 +8452,8 @@ static unsigned long pfn_max_align_up(un > > /* [start, end) must belong to a single zone. */ > static int __alloc_contig_migrate_range(struct compact_control *cc, > - unsigned long start, unsigned long end) > + unsigned long start, unsigned long end, > + unsigned int *nr_succeeded) > { > /* This function is based on compact_zone() from compaction.c. */ > unsigned int nr_reclaimed; > @@ -8490,7 +8491,8 @@ static int __alloc_contig_migrate_range( > cc->nr_migratepages -= nr_reclaimed; > > ret = migrate_pages(&cc->migratepages, alloc_migration_target, > - NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE); > + NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE, > + nr_succeeded); > } > if (ret < 0) { > putback_movable_pages(&cc->migratepages); > @@ -8526,6 +8528,7 @@ int alloc_contig_range(unsigned long sta > unsigned long outer_start, outer_end; > unsigned int order; > int ret = 0; > + unsigned int nr_succeeded = 0; > > struct compact_control cc = { > .nr_migratepages = 0, > @@ -8580,7 +8583,7 @@ int alloc_contig_range(unsigned long sta > * allocated. So, if we fall through be sure to clear ret so that > * -EBUSY is not accidentally used or returned to caller. > */ > - ret = __alloc_contig_migrate_range(&cc, start, end); > + ret = __alloc_contig_migrate_range(&cc, start, end, &nr_succeeded); > if (ret && ret != -EBUSY) > goto done; > ret =0; > _
On 4/1/21 3:35 PM, Wei Xu wrote: > A small suggestion: Given that migrate_pages() requires that > *nr_succeeded should be initialized to 0 when it is called due to its > use of *nr_succeeded in count_vm_events()Â and trace_mm_migrate_pages(), > it would be less error-prone if migrate_pages() initializes > *nr_succeeded itself. That's a good point, especially if a caller made multiple migrate_pages() calls without resetting it, which a number of callers do. That could really have caused some interesting problems. Thanks for catching that! I'll do what you suggested.
On Thu, Apr 01, 2021 at 11:32:23AM -0700, Dave Hansen wrote: > > From: Yang Shi <yang.shi@linux.alibaba.com> > > The migrate_pages() returns the number of pages that were not migrated, > or an error code. When returning an error code, there is no way to know > how many pages were migrated or not migrated. > > In the following patch, migrate_pages() is used to demote pages to PMEM > node, we need account how many pages are reclaimed (demoted) since page > reclaim behavior depends on this. Add *nr_succeeded parameter to make > migrate_pages() return how many pages are demoted successfully for all > cases. > > Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> > Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com> > Reviewed-by: Yang Shi <shy828301@gmail.com> > Cc: Wei Xu <weixugc@google.com> > Cc: Huang Ying <ying.huang@intel.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: David Hildenbrand <david@redhat.com> > Cc: osalvador <osalvador@suse.de> > ... > int migrate_pages(struct list_head *from, new_page_t get_new_page, > free_page_t put_new_page, unsigned long private, > - enum migrate_mode mode, int reason) > + enum migrate_mode mode, int reason, unsigned int *nr_succeeded) > { > int retry = 1; > int thp_retry = 1; > int nr_failed = 0; > - int nr_succeeded = 0; > int nr_thp_succeeded = 0; > int nr_thp_failed = 0; > int nr_thp_split = 0; > @@ -1611,10 +1611,10 @@ retry: > case MIGRATEPAGE_SUCCESS: > if (is_thp) { > nr_thp_succeeded++; > - nr_succeeded += nr_subpages; > + *nr_succeeded += nr_subpages; > break; > } > - nr_succeeded++; > + (*nr_succeeded)++; > break; > default: > /* > @@ -1643,12 +1643,12 @@ out: > */ > list_splice(&ret_pages, from); > > - count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded); > + count_vm_events(PGMIGRATE_SUCCESS, *nr_succeeded); > count_vm_events(PGMIGRATE_FAIL, nr_failed); > count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded); > count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed); > count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split); > - trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded, > + trace_mm_migrate_pages(*nr_succeeded, nr_failed, nr_thp_succeeded, > nr_thp_failed, nr_thp_split, mode, reason); It seems that reclaiming is the only user who cared about how many pages could we migrated, could not do the following instead: diff --git a/mm/migrate.c b/mm/migrate.c index 695a594e5860..d4170b7ea2fe 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -1503,7 +1503,7 @@ static inline int try_split_thp(struct page *page, struct page **page2, */ int migrate_pages(struct list_head *from, new_page_t get_new_page, free_page_t put_new_page, unsigned long private, - enum migrate_mode mode, int reason) + enum migrate_mode mode, int reason, unsigned int *ret_succeeded) { int retry = 1; int thp_retry = 1; @@ -1654,6 +1654,9 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page, if (!swapwrite) current->flags &= ~PF_SWAPWRITE; + if (ret_succedded) + *ret_succedded = nr_succedded; + return rc; } And pass only a valid pointer from demote_page_list() and NULL from all the others? I was just wondered after all those "unsigned int nr_succedded" in all other functions. This would also solve the "be careful to initialize nr_succedded" problem?
On Thu, Apr 8, 2021 at 3:14 AM Oscar Salvador <osalvador@suse.de> wrote: > > On Thu, Apr 01, 2021 at 11:32:23AM -0700, Dave Hansen wrote: > > > > From: Yang Shi <yang.shi@linux.alibaba.com> > > > > The migrate_pages() returns the number of pages that were not migrated, > > or an error code. When returning an error code, there is no way to know > > how many pages were migrated or not migrated. > > > > In the following patch, migrate_pages() is used to demote pages to PMEM > > node, we need account how many pages are reclaimed (demoted) since page > > reclaim behavior depends on this. Add *nr_succeeded parameter to make > > migrate_pages() return how many pages are demoted successfully for all > > cases. > > > > Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> > > Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com> > > Reviewed-by: Yang Shi <shy828301@gmail.com> > > Cc: Wei Xu <weixugc@google.com> > > Cc: Huang Ying <ying.huang@intel.com> > > Cc: Dan Williams <dan.j.williams@intel.com> > > Cc: David Hildenbrand <david@redhat.com> > > Cc: osalvador <osalvador@suse.de> > > > > ... > > int migrate_pages(struct list_head *from, new_page_t get_new_page, > > free_page_t put_new_page, unsigned long private, > > - enum migrate_mode mode, int reason) > > + enum migrate_mode mode, int reason, unsigned int *nr_succeeded) > > { > > int retry = 1; > > int thp_retry = 1; > > int nr_failed = 0; > > - int nr_succeeded = 0; > > int nr_thp_succeeded = 0; > > int nr_thp_failed = 0; > > int nr_thp_split = 0; > > @@ -1611,10 +1611,10 @@ retry: > > case MIGRATEPAGE_SUCCESS: > > if (is_thp) { > > nr_thp_succeeded++; > > - nr_succeeded += nr_subpages; > > + *nr_succeeded += nr_subpages; > > break; > > } > > - nr_succeeded++; > > + (*nr_succeeded)++; > > break; > > default: > > /* > > @@ -1643,12 +1643,12 @@ out: > > */ > > list_splice(&ret_pages, from); > > > > - count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded); > > + count_vm_events(PGMIGRATE_SUCCESS, *nr_succeeded); > > count_vm_events(PGMIGRATE_FAIL, nr_failed); > > count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded); > > count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed); > > count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split); > > - trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded, > > + trace_mm_migrate_pages(*nr_succeeded, nr_failed, nr_thp_succeeded, > > nr_thp_failed, nr_thp_split, mode, reason); > > It seems that reclaiming is the only user who cared about how many pages > could we migrated, could not do the following instead: > > diff --git a/mm/migrate.c b/mm/migrate.c > index 695a594e5860..d4170b7ea2fe 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -1503,7 +1503,7 @@ static inline int try_split_thp(struct page *page, struct page **page2, > */ > int migrate_pages(struct list_head *from, new_page_t get_new_page, > free_page_t put_new_page, unsigned long private, > - enum migrate_mode mode, int reason) > + enum migrate_mode mode, int reason, unsigned int *ret_succeeded) > { > int retry = 1; > int thp_retry = 1; > @@ -1654,6 +1654,9 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page, > if (!swapwrite) > current->flags &= ~PF_SWAPWRITE; > > + if (ret_succedded) > + *ret_succedded = nr_succedded; > + > return rc; > } > > And pass only a valid pointer from demote_page_list() and NULL from all > the others? > I was just wondered after all those "unsigned int nr_succedded" in all > other functions. > This would also solve the "be careful to initialize nr_succedded" > problem? Thanks, Oscar. Yes, kind of. But we have to remember to initialize "nr_succedded" pointer properly for every migrate_pages() callsite, right? And it doesn't prevent from returning wrong value if migrate_pages() is called multiple times by one caller although there might be not such case (calls migrate_pages() multiple times and care about nr_succeded) for now. So IMHO I do prefer Wei's suggestion to have migrate_pages() initialize nr_succeeded. This seems simpler. > > > -- > Oscar Salvador > SUSE L3
On Thu, Apr 08, 2021 at 10:26:54AM -0700, Yang Shi wrote: > Thanks, Oscar. Yes, kind of. But we have to remember to initialize > "nr_succedded" pointer properly for every migrate_pages() callsite, > right? And it doesn't prevent from returning wrong value if > migrate_pages() is called multiple times by one caller although there > might be not such case (calls migrate_pages() multiple times and care > about nr_succeded) for now. Hi Yang, I might be missing something but AFAICS you only need to initialize nr_succeded pointer where it matters. The local nr_succeeded in migrate_pages() doesn't go, and so it gets initialized every time you call in it to 0. And if you pass a valid pointer, *ret_succeeded == nr_succedeed. I am talking about this (not even compile-tested): diff --git a/include/linux/migrate.h b/include/linux/migrate.h index 3a389633b68f..fd661cb2ce13 100644 --- a/include/linux/migrate.h +++ b/include/linux/migrate.h @@ -40,7 +40,8 @@ extern int migrate_page(struct address_space *mapping, struct page *newpage, struct page *page, enum migrate_mode mode); extern int migrate_pages(struct list_head *l, new_page_t new, free_page_t free, - unsigned long private, enum migrate_mode mode, int reason); + unsigned long private, enum migrate_mode mode, int reason, + unsigned int *ret_succeeded); extern struct page *alloc_migration_target(struct page *page, unsigned long private); extern int isolate_movable_page(struct page *page, isolate_mode_t mode); extern void putback_movable_page(struct page *page); @@ -58,7 +59,7 @@ extern int migrate_page_move_mapping(struct address_space *mapping, static inline void putback_movable_pages(struct list_head *l) {} static inline int migrate_pages(struct list_head *l, new_page_t new, free_page_t free, unsigned long private, enum migrate_mode mode, - int reason) + int reason, unsigned int *ret_succeeded) { return -ENOSYS; } static inline struct page *alloc_migration_target(struct page *page, unsigned long private) diff --git a/mm/compaction.c b/mm/compaction.c index e04f4476e68e..7238e8faff04 100644 --- a/mm/compaction.c +++ b/mm/compaction.c @@ -2364,7 +2364,7 @@ compact_zone(struct compact_control *cc, struct capture_control *capc) err = migrate_pages(&cc->migratepages, compaction_alloc, compaction_free, (unsigned long)cc, cc->mode, - MR_COMPACTION); + MR_COMPACTION, NULL); trace_mm_compaction_migratepages(cc->nr_migratepages, err, &cc->migratepages); diff --git a/mm/gup.c b/mm/gup.c index e40579624f10..b70d463aa1fc 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -1606,7 +1606,7 @@ static long check_and_migrate_cma_pages(struct mm_struct *mm, put_page(pages[i]); if (migrate_pages(&cma_page_list, alloc_migration_target, NULL, - (unsigned long)&mtc, MIGRATE_SYNC, MR_CONTIG_RANGE)) { + (unsigned long)&mtc, MIGRATE_SYNC, MR_CONTIG_RANGE, NULL)) { /* * some of the pages failed migration. Do get_user_pages * without migration. diff --git a/mm/memory-failure.c b/mm/memory-failure.c index 24210c9bd843..a17e0f039076 100644 --- a/mm/memory-failure.c +++ b/mm/memory-failure.c @@ -1852,7 +1852,8 @@ static int __soft_offline_page(struct page *page) if (isolate_page(hpage, &pagelist)) { ret = migrate_pages(&pagelist, alloc_migration_target, NULL, - (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE); + (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE, + NULL); if (!ret) { bool release = !huge; diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index 0cdbbfbc5757..28496376de94 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -1466,7 +1466,8 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) if (nodes_empty(nmask)) node_set(mtc.nid, nmask); ret = migrate_pages(&source, alloc_migration_target, NULL, - (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_HOTPLUG); + (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_HOTPLUG, + NULL); if (ret) { list_for_each_entry(page, &source, lru) { pr_warn("migrating pfn %lx failed ret:%d ", diff --git a/mm/mempolicy.c b/mm/mempolicy.c index ab51132547b8..df260ed12102 100644 --- a/mm/mempolicy.c +++ b/mm/mempolicy.c @@ -1103,7 +1103,8 @@ static int migrate_to_node(struct mm_struct *mm, int source, int dest, if (!list_empty(&pagelist)) { err = migrate_pages(&pagelist, alloc_migration_target, NULL, - (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL); + (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL, + NULL); if (err) putback_movable_pages(&pagelist); } @@ -1355,7 +1356,8 @@ static long do_mbind(unsigned long start, unsigned long len, if (!list_empty(&pagelist)) { WARN_ON_ONCE(flags & MPOL_MF_LAZY); nr_failed = migrate_pages(&pagelist, new_page, NULL, - start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND); + start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND, + NULL); if (nr_failed) putback_movable_pages(&pagelist); } diff --git a/mm/migrate.c b/mm/migrate.c index 695a594e5860..087ed407b3ce 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -1493,6 +1493,9 @@ static inline int try_split_thp(struct page *page, struct page **page2, * @mode: The migration mode that specifies the constraints for * page migration, if any. * @reason: The reason for page migration. + * @ret_succeeded: A pointer to place the value of the number of pages + * migrated successfully. The caller must pass a valid pointer + * if they care about it. * * The function returns after 10 attempts or if no pages are movable any more * because the list has become empty or no retryable pages exist any more. @@ -1503,7 +1506,7 @@ static inline int try_split_thp(struct page *page, struct page **page2, */ int migrate_pages(struct list_head *from, new_page_t get_new_page, free_page_t put_new_page, unsigned long private, - enum migrate_mode mode, int reason) + enum migrate_mode mode, int reason, unsigned int *ret_succeeded) { int retry = 1; int thp_retry = 1; @@ -1654,6 +1657,9 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page, if (!swapwrite) current->flags &= ~PF_SWAPWRITE; + if (ret_succeeded) + *ret_succeeded = nr_succeeded; + return rc; } @@ -1723,7 +1729,8 @@ static int do_move_pages_to_node(struct mm_struct *mm, }; err = migrate_pages(pagelist, alloc_migration_target, NULL, - (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL); + (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL, + NULL); if (err) putback_movable_pages(pagelist); return err; @@ -2230,7 +2237,7 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma, list_add(&page->lru, &migratepages); nr_remaining = migrate_pages(&migratepages, alloc_misplaced_dst_page, NULL, node, MIGRATE_ASYNC, - MR_NUMA_MISPLACED); + MR_NUMA_MISPLACED, NULL); if (nr_remaining) { if (!list_empty(&migratepages)) { list_del(&page->lru); diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 46f3d594369d..0c1bbadd5ca3 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -8490,7 +8490,8 @@ static int __alloc_contig_migrate_range(struct compact_control *cc, cc->nr_migratepages -= nr_reclaimed; ret = migrate_pages(&cc->migratepages, alloc_migration_target, - NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE); + NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE, + NULL); } if (ret < 0) { putback_movable_pages(&cc->migratepages); As I said I might be missing a point here, but I cannot see the problem you describe here.
On Thu, Apr 08, 2021 at 08:17:26PM +0200, Oscar Salvador wrote: > diff --git a/include/linux/migrate.h b/include/linux/migrate.h > index 3a389633b68f..fd661cb2ce13 100644 > --- a/include/linux/migrate.h > +++ b/include/linux/migrate.h > @@ -40,7 +40,8 @@ extern int migrate_page(struct address_space *mapping, > struct page *newpage, struct page *page, > enum migrate_mode mode); > extern int migrate_pages(struct list_head *l, new_page_t new, free_page_t free, > - unsigned long private, enum migrate_mode mode, int reason); > + unsigned long private, enum migrate_mode mode, int reason, > + unsigned int *ret_succeeded); > extern struct page *alloc_migration_target(struct page *page, unsigned long private); > extern int isolate_movable_page(struct page *page, isolate_mode_t mode); > extern void putback_movable_page(struct page *page); > @@ -58,7 +59,7 @@ extern int migrate_page_move_mapping(struct address_space *mapping, > static inline void putback_movable_pages(struct list_head *l) {} > static inline int migrate_pages(struct list_head *l, new_page_t new, > free_page_t free, unsigned long private, enum migrate_mode mode, > - int reason) > + int reason, unsigned int *ret_succeeded) > { return -ENOSYS; } > static inline struct page *alloc_migration_target(struct page *page, > unsigned long private) > diff --git a/mm/compaction.c b/mm/compaction.c > index e04f4476e68e..7238e8faff04 100644 > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -2364,7 +2364,7 @@ compact_zone(struct compact_control *cc, struct capture_control *capc) > > err = migrate_pages(&cc->migratepages, compaction_alloc, > compaction_free, (unsigned long)cc, cc->mode, > - MR_COMPACTION); > + MR_COMPACTION, NULL); > > trace_mm_compaction_migratepages(cc->nr_migratepages, err, > &cc->migratepages); > diff --git a/mm/gup.c b/mm/gup.c > index e40579624f10..b70d463aa1fc 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -1606,7 +1606,7 @@ static long check_and_migrate_cma_pages(struct mm_struct *mm, > put_page(pages[i]); > > if (migrate_pages(&cma_page_list, alloc_migration_target, NULL, > - (unsigned long)&mtc, MIGRATE_SYNC, MR_CONTIG_RANGE)) { > + (unsigned long)&mtc, MIGRATE_SYNC, MR_CONTIG_RANGE, NULL)) { > /* > * some of the pages failed migration. Do get_user_pages > * without migration. > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index 24210c9bd843..a17e0f039076 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -1852,7 +1852,8 @@ static int __soft_offline_page(struct page *page) > > if (isolate_page(hpage, &pagelist)) { > ret = migrate_pages(&pagelist, alloc_migration_target, NULL, > - (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE); > + (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE, > + NULL); > if (!ret) { > bool release = !huge; > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > index 0cdbbfbc5757..28496376de94 100644 > --- a/mm/memory_hotplug.c > +++ b/mm/memory_hotplug.c > @@ -1466,7 +1466,8 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) > if (nodes_empty(nmask)) > node_set(mtc.nid, nmask); > ret = migrate_pages(&source, alloc_migration_target, NULL, > - (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_HOTPLUG); > + (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_HOTPLUG, > + NULL); > if (ret) { > list_for_each_entry(page, &source, lru) { > pr_warn("migrating pfn %lx failed ret:%d ", > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > index ab51132547b8..df260ed12102 100644 > --- a/mm/mempolicy.c > +++ b/mm/mempolicy.c > @@ -1103,7 +1103,8 @@ static int migrate_to_node(struct mm_struct *mm, int source, int dest, > > if (!list_empty(&pagelist)) { > err = migrate_pages(&pagelist, alloc_migration_target, NULL, > - (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL); > + (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL, > + NULL); > if (err) > putback_movable_pages(&pagelist); > } > @@ -1355,7 +1356,8 @@ static long do_mbind(unsigned long start, unsigned long len, > if (!list_empty(&pagelist)) { > WARN_ON_ONCE(flags & MPOL_MF_LAZY); > nr_failed = migrate_pages(&pagelist, new_page, NULL, > - start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND); > + start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND, > + NULL); > if (nr_failed) > putback_movable_pages(&pagelist); > } > diff --git a/mm/migrate.c b/mm/migrate.c > index 695a594e5860..087ed407b3ce 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -1493,6 +1493,9 @@ static inline int try_split_thp(struct page *page, struct page **page2, > * @mode: The migration mode that specifies the constraints for > * page migration, if any. > * @reason: The reason for page migration. > + * @ret_succeeded: A pointer to place the value of the number of pages > + * migrated successfully. The caller must pass a valid pointer > + * if they care about it. > * > * The function returns after 10 attempts or if no pages are movable any more > * because the list has become empty or no retryable pages exist any more. > @@ -1503,7 +1506,7 @@ static inline int try_split_thp(struct page *page, struct page **page2, > */ > int migrate_pages(struct list_head *from, new_page_t get_new_page, > free_page_t put_new_page, unsigned long private, > - enum migrate_mode mode, int reason) > + enum migrate_mode mode, int reason, unsigned int *ret_succeeded) > { > int retry = 1; > int thp_retry = 1; > @@ -1654,6 +1657,9 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page, > if (!swapwrite) > current->flags &= ~PF_SWAPWRITE; > > + if (ret_succeeded) > + *ret_succeeded = nr_succeeded; > + > return rc; > } > > @@ -1723,7 +1729,8 @@ static int do_move_pages_to_node(struct mm_struct *mm, > }; > > err = migrate_pages(pagelist, alloc_migration_target, NULL, > - (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL); > + (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL, > + NULL); > if (err) > putback_movable_pages(pagelist); > return err; > @@ -2230,7 +2237,7 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma, > list_add(&page->lru, &migratepages); > nr_remaining = migrate_pages(&migratepages, alloc_misplaced_dst_page, > NULL, node, MIGRATE_ASYNC, > - MR_NUMA_MISPLACED); > + MR_NUMA_MISPLACED, NULL); > if (nr_remaining) { > if (!list_empty(&migratepages)) { > list_del(&page->lru); > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 46f3d594369d..0c1bbadd5ca3 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -8490,7 +8490,8 @@ static int __alloc_contig_migrate_range(struct compact_control *cc, > cc->nr_migratepages -= nr_reclaimed; > > ret = migrate_pages(&cc->migratepages, alloc_migration_target, > - NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE); > + NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE, > + NULL); > } > if (ret < 0) { > putback_movable_pages(&cc->migratepages); Of course, to give a full context: +static unsigned int demote_page_list(struct list_head *demote_pages, + struct pglist_data *pgdat, + struct scan_control *sc) +{ + int target_nid = next_demotion_node(pgdat->node_id); + unsigned int nr_succeeded = 0; + int err; + + if (list_empty(demote_pages)) + return 0; + + /* Demotion ignores all cpuset and mempolicy settings */ + err = migrate_pages(demote_pages, alloc_demote_page, NULL, + target_nid, MIGRATE_ASYNC, MR_DEMOTION, + &nr_succeeded); + + return nr_succeeded; +} So, demote_page_list() would be the only function that passes a valid pointer, instead of NULL, because it cares about the nr_succeeded.
On Thu, Apr 8, 2021 at 11:17 AM Oscar Salvador <osalvador@suse.de> wrote: > > On Thu, Apr 08, 2021 at 10:26:54AM -0700, Yang Shi wrote: > > > Thanks, Oscar. Yes, kind of. But we have to remember to initialize > > "nr_succedded" pointer properly for every migrate_pages() callsite, > > right? And it doesn't prevent from returning wrong value if > > migrate_pages() is called multiple times by one caller although there > > might be not such case (calls migrate_pages() multiple times and care > > about nr_succeded) for now. > > Hi Yang, > > I might be missing something but AFAICS you only need to initialize > nr_succeded pointer where it matters. > The local nr_succeeded in migrate_pages() doesn't go, and so it gets > initialized every time you call in it to 0. > And if you pass a valid pointer, *ret_succeeded == nr_succedeed. > > I am talking about this (not even compile-tested): > > diff --git a/include/linux/migrate.h b/include/linux/migrate.h > index 3a389633b68f..fd661cb2ce13 100644 > --- a/include/linux/migrate.h > +++ b/include/linux/migrate.h > @@ -40,7 +40,8 @@ extern int migrate_page(struct address_space *mapping, > struct page *newpage, struct page *page, > enum migrate_mode mode); > extern int migrate_pages(struct list_head *l, new_page_t new, free_page_t free, > - unsigned long private, enum migrate_mode mode, int reason); > + unsigned long private, enum migrate_mode mode, int reason, > + unsigned int *ret_succeeded); > extern struct page *alloc_migration_target(struct page *page, unsigned long private); > extern int isolate_movable_page(struct page *page, isolate_mode_t mode); > extern void putback_movable_page(struct page *page); > @@ -58,7 +59,7 @@ extern int migrate_page_move_mapping(struct address_space *mapping, > static inline void putback_movable_pages(struct list_head *l) {} > static inline int migrate_pages(struct list_head *l, new_page_t new, > free_page_t free, unsigned long private, enum migrate_mode mode, > - int reason) > + int reason, unsigned int *ret_succeeded) > { return -ENOSYS; } > static inline struct page *alloc_migration_target(struct page *page, > unsigned long private) > diff --git a/mm/compaction.c b/mm/compaction.c > index e04f4476e68e..7238e8faff04 100644 > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -2364,7 +2364,7 @@ compact_zone(struct compact_control *cc, struct capture_control *capc) > > err = migrate_pages(&cc->migratepages, compaction_alloc, > compaction_free, (unsigned long)cc, cc->mode, > - MR_COMPACTION); > + MR_COMPACTION, NULL); > > trace_mm_compaction_migratepages(cc->nr_migratepages, err, > &cc->migratepages); > diff --git a/mm/gup.c b/mm/gup.c > index e40579624f10..b70d463aa1fc 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -1606,7 +1606,7 @@ static long check_and_migrate_cma_pages(struct mm_struct *mm, > put_page(pages[i]); > > if (migrate_pages(&cma_page_list, alloc_migration_target, NULL, > - (unsigned long)&mtc, MIGRATE_SYNC, MR_CONTIG_RANGE)) { > + (unsigned long)&mtc, MIGRATE_SYNC, MR_CONTIG_RANGE, NULL)) { > /* > * some of the pages failed migration. Do get_user_pages > * without migration. > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index 24210c9bd843..a17e0f039076 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -1852,7 +1852,8 @@ static int __soft_offline_page(struct page *page) > > if (isolate_page(hpage, &pagelist)) { > ret = migrate_pages(&pagelist, alloc_migration_target, NULL, > - (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE); > + (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE, > + NULL); > if (!ret) { > bool release = !huge; > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > index 0cdbbfbc5757..28496376de94 100644 > --- a/mm/memory_hotplug.c > +++ b/mm/memory_hotplug.c > @@ -1466,7 +1466,8 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn) > if (nodes_empty(nmask)) > node_set(mtc.nid, nmask); > ret = migrate_pages(&source, alloc_migration_target, NULL, > - (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_HOTPLUG); > + (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_HOTPLUG, > + NULL); > if (ret) { > list_for_each_entry(page, &source, lru) { > pr_warn("migrating pfn %lx failed ret:%d ", > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > index ab51132547b8..df260ed12102 100644 > --- a/mm/mempolicy.c > +++ b/mm/mempolicy.c > @@ -1103,7 +1103,8 @@ static int migrate_to_node(struct mm_struct *mm, int source, int dest, > > if (!list_empty(&pagelist)) { > err = migrate_pages(&pagelist, alloc_migration_target, NULL, > - (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL); > + (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL, > + NULL); > if (err) > putback_movable_pages(&pagelist); > } > @@ -1355,7 +1356,8 @@ static long do_mbind(unsigned long start, unsigned long len, > if (!list_empty(&pagelist)) { > WARN_ON_ONCE(flags & MPOL_MF_LAZY); > nr_failed = migrate_pages(&pagelist, new_page, NULL, > - start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND); > + start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND, > + NULL); > if (nr_failed) > putback_movable_pages(&pagelist); > } > diff --git a/mm/migrate.c b/mm/migrate.c > index 695a594e5860..087ed407b3ce 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -1493,6 +1493,9 @@ static inline int try_split_thp(struct page *page, struct page **page2, > * @mode: The migration mode that specifies the constraints for > * page migration, if any. > * @reason: The reason for page migration. > + * @ret_succeeded: A pointer to place the value of the number of pages > + * migrated successfully. The caller must pass a valid pointer > + * if they care about it. > * > * The function returns after 10 attempts or if no pages are movable any more > * because the list has become empty or no retryable pages exist any more. > @@ -1503,7 +1506,7 @@ static inline int try_split_thp(struct page *page, struct page **page2, > */ > int migrate_pages(struct list_head *from, new_page_t get_new_page, > free_page_t put_new_page, unsigned long private, > - enum migrate_mode mode, int reason) > + enum migrate_mode mode, int reason, unsigned int *ret_succeeded) > { > int retry = 1; > int thp_retry = 1; > @@ -1654,6 +1657,9 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page, > if (!swapwrite) > current->flags &= ~PF_SWAPWRITE; > > + if (ret_succeeded) > + *ret_succeeded = nr_succeeded; > + Thanks a lot for the example code. You didn't miss anything. At first glance, I thought your suggestion seemed neater. Actually I misunderstood what Dave said about "That could really have caused some interesting problems." with multiple calls to migrate_pages(). I was thinking about: unsigned long foo() { unsigned long *ret_succeeded; migrate_pages(..., ret_succeeded); migrate_pages(..., ret_succeeded); return *ret_succeeded; } Of course, this is a hypothetical case now. We don't have to care about it at all. > return rc; > } > > @@ -1723,7 +1729,8 @@ static int do_move_pages_to_node(struct mm_struct *mm, > }; > > err = migrate_pages(pagelist, alloc_migration_target, NULL, > - (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL); > + (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL, > + NULL); > if (err) > putback_movable_pages(pagelist); > return err; > @@ -2230,7 +2237,7 @@ int migrate_misplaced_page(struct page *page, struct vm_area_struct *vma, > list_add(&page->lru, &migratepages); > nr_remaining = migrate_pages(&migratepages, alloc_misplaced_dst_page, > NULL, node, MIGRATE_ASYNC, > - MR_NUMA_MISPLACED); > + MR_NUMA_MISPLACED, NULL); > if (nr_remaining) { > if (!list_empty(&migratepages)) { > list_del(&page->lru); > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 46f3d594369d..0c1bbadd5ca3 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -8490,7 +8490,8 @@ static int __alloc_contig_migrate_range(struct compact_control *cc, > cc->nr_migratepages -= nr_reclaimed; > > ret = migrate_pages(&cc->migratepages, alloc_migration_target, > - NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE); > + NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE, > + NULL); > } > if (ret < 0) { > putback_movable_pages(&cc->migratepages); > > > As I said I might be missing a point here, but I cannot see the problem > you describe here. > > > -- > Oscar Salvador > SUSE L3
On Thu, Apr 08, 2021 at 01:40:33PM -0700, Yang Shi wrote: > Thanks a lot for the example code. You didn't miss anything. At first > glance, I thought your suggestion seemed neater. Actually I > misunderstood what Dave said about "That could really have caused some > interesting problems." with multiple calls to migrate_pages(). I was > thinking about: > > unsigned long foo() > { > unsigned long *ret_succeeded; > > migrate_pages(..., ret_succeeded); > > migrate_pages(..., ret_succeeded); > > return *ret_succeeded; > } But that would not be a problem as well. I mean I am not sure what is foo() supposed to do. I assume is supposed to return the *total* number of pages that were migrated? Then could do something like: unsigned long foo() { unsigned long ret_succeeded; unsigned long total_succeeded = 0; migrate_pages(..., &ret_succeeded); total_succeeded += ret_succeeded; migrate_pages(..., &ret_succeeded); total_succeeded += ret_succeeded; return *total_succeeded; } But AFAICS, you would have to do that with Wei Xu's version and with mine, no difference there. IIUC, Dave's concern was that nr_succeeded was only set to 0 at the beginning of the function, and never reset back, which means, we would carry the sum of previous nr_succeeded instead of the nr_succeeded in that round. That would be misleading for e.g: reclaim in case we were to call migrate_pages() several times, as instead of a delta value, nr_succeeded would accumulate. But that won't happen neither with Wei Xu's version nor with mine.
I agree that it is a good further improvement to make nr_succeeded an optional output argument of migrate_pages() given that most callers don't need it. IMHO, the most important thing in this matter is to ensure that nr_succeeded only returns (when its return value is needed) the successfully migrated pages in this round and doesn't accumulate. This is addressed by both proposals. On Thu, Apr 8, 2021 at 10:06 PM Oscar Salvador <osalvador@suse.de> wrote: > > On Thu, Apr 08, 2021 at 01:40:33PM -0700, Yang Shi wrote: > > Thanks a lot for the example code. You didn't miss anything. At first > > glance, I thought your suggestion seemed neater. Actually I > > misunderstood what Dave said about "That could really have caused some > > interesting problems." with multiple calls to migrate_pages(). I was > > thinking about: > > > > unsigned long foo() > > { > > unsigned long *ret_succeeded; > > > > migrate_pages(..., ret_succeeded); > > > > migrate_pages(..., ret_succeeded); > > > > return *ret_succeeded; > > } > > But that would not be a problem as well. I mean I am not sure what is > foo() supposed to do. > I assume is supposed to return the *total* number of pages that were > migrated? > > Then could do something like: > > unsigned long foo() > { > unsigned long ret_succeeded; > unsigned long total_succeeded = 0; > > migrate_pages(..., &ret_succeeded); > total_succeeded += ret_succeeded; > > migrate_pages(..., &ret_succeeded); > total_succeeded += ret_succeeded; > > return *total_succeeded; > } > > But AFAICS, you would have to do that with Wei Xu's version and with > mine, no difference there. > > IIUC, Dave's concern was that nr_succeeded was only set to 0 at the beginning > of the function, and never reset back, which means, we would carry the > sum of previous nr_succeeded instead of the nr_succeeded in that round. > That would be misleading for e.g: reclaim in case we were to call > migrate_pages() several times, as instead of a delta value, nr_succeeded > would accumulate. > > But that won't happen neither with Wei Xu's version nor with mine. > > -- > Oscar Salvador > SUSE L3
On Thu, Apr 8, 2021 at 10:06 PM Oscar Salvador <osalvador@suse.de> wrote: > > On Thu, Apr 08, 2021 at 01:40:33PM -0700, Yang Shi wrote: > > Thanks a lot for the example code. You didn't miss anything. At first > > glance, I thought your suggestion seemed neater. Actually I > > misunderstood what Dave said about "That could really have caused some > > interesting problems." with multiple calls to migrate_pages(). I was > > thinking about: > > > > unsigned long foo() > > { > > unsigned long *ret_succeeded; > > > > migrate_pages(..., ret_succeeded); > > > > migrate_pages(..., ret_succeeded); > > > > return *ret_succeeded; > > } > > But that would not be a problem as well. I mean I am not sure what is > foo() supposed to do. > I assume is supposed to return the *total* number of pages that were > migrated? > > Then could do something like: > > unsigned long foo() > { > unsigned long ret_succeeded; > unsigned long total_succeeded = 0; > > migrate_pages(..., &ret_succeeded); > total_succeeded += ret_succeeded; > > migrate_pages(..., &ret_succeeded); > total_succeeded += ret_succeeded; > > return *total_succeeded; > } > > But AFAICS, you would have to do that with Wei Xu's version and with > mine, no difference there. It is because nr_succeeded is reset for each migrate_pages() call. You could do "*ret_succeeded += nr_succeeded" if we want an accumulated counter, then you don't have to add total_succeeded. And since nr_succeeded is reset for each migrate_pages() call, so both vm counter and trace point are happy. > > IIUC, Dave's concern was that nr_succeeded was only set to 0 at the beginning > of the function, and never reset back, which means, we would carry the > sum of previous nr_succeeded instead of the nr_succeeded in that round. > That would be misleading for e.g: reclaim in case we were to call > migrate_pages() several times, as instead of a delta value, nr_succeeded > would accumulate. I think the most straightforward concern is the vm counter and trace point in migrate_pages(), if migrate_pages() is called multiple times we may see messed up counters if nr_succeeded is not reset properly. Of course both your and Wei's suggestion solve this problem. But if we have usecase which returns nr_succeeded and call migrate_pages() multiple times, I think we do want to return accumulated value IMHO. > > But that won't happen neither with Wei Xu's version nor with mine. > > -- > Oscar Salvador > SUSE L3
On 4/8/21 11:17 AM, Oscar Salvador wrote: > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -8490,7 +8490,8 @@ static int __alloc_contig_migrate_range(struct compact_control *cc, > cc->nr_migratepages -= nr_reclaimed; > > ret = migrate_pages(&cc->migratepages, alloc_migration_target, > - NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE); > + NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE, > + NULL); > } > if (ret < 0) { > putback_movable_pages(&cc->migratepages); I also considered passing NULL to mean "I don't care about nr_succeeded". I mostly avoided it to reduce churn. But, looking at it here, it does seem cleaner. Any objections to moving over to Oscar's suggestion?
On Fri, Apr 9, 2021 at 8:50 AM Dave Hansen <dave.hansen@intel.com> wrote: > I also considered passing NULL to mean "I don't care about > nr_succeeded". I mostly avoided it to reduce churn. But, looking at it > here, it does seem cleaner. > > Any objections to moving over to Oscar's suggestion? I like this approach (making *nr_succeeded an optional argument). No objection here.
On Fri, Apr 9, 2021 at 8:50 AM Dave Hansen <dave.hansen@intel.com> wrote: > > On 4/8/21 11:17 AM, Oscar Salvador wrote: > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -8490,7 +8490,8 @@ static int __alloc_contig_migrate_range(struct compact_control *cc, > > cc->nr_migratepages -= nr_reclaimed; > > > > ret = migrate_pages(&cc->migratepages, alloc_migration_target, > > - NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE); > > + NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE, > > + NULL); > > } > > if (ret < 0) { > > putback_movable_pages(&cc->migratepages); > > I also considered passing NULL to mean "I don't care about > nr_succeeded". I mostly avoided it to reduce churn. But, looking at it > here, it does seem cleaner. > > Any objections to moving over to Oscar's suggestion? No, fine to me.
diff -puN include/linux/migrate.h~migrate_pages-add-success-return include/linux/migrate.h --- a/include/linux/migrate.h~migrate_pages-add-success-return 2021-03-31 15:17:14.144000255 -0700 +++ b/include/linux/migrate.h 2021-03-31 15:17:14.182000255 -0700 @@ -40,7 +40,8 @@ extern int migrate_page(struct address_s struct page *newpage, struct page *page, enum migrate_mode mode); extern int migrate_pages(struct list_head *l, new_page_t new, free_page_t free, - unsigned long private, enum migrate_mode mode, int reason); + unsigned long private, enum migrate_mode mode, int reason, + unsigned int *nr_succeeded); extern struct page *alloc_migration_target(struct page *page, unsigned long private); extern int isolate_movable_page(struct page *page, isolate_mode_t mode); extern void putback_movable_page(struct page *page); @@ -58,7 +59,7 @@ extern int migrate_page_move_mapping(str static inline void putback_movable_pages(struct list_head *l) {} static inline int migrate_pages(struct list_head *l, new_page_t new, free_page_t free, unsigned long private, enum migrate_mode mode, - int reason) + int reason, unsigned int *nr_succeeded) { return -ENOSYS; } static inline struct page *alloc_migration_target(struct page *page, unsigned long private) diff -puN mm/compaction.c~migrate_pages-add-success-return mm/compaction.c --- a/mm/compaction.c~migrate_pages-add-success-return 2021-03-31 15:17:14.146000255 -0700 +++ b/mm/compaction.c 2021-03-31 15:17:14.186000255 -0700 @@ -2247,6 +2247,7 @@ compact_zone(struct compact_control *cc, unsigned long last_migrated_pfn; const bool sync = cc->mode != MIGRATE_ASYNC; bool update_cached; + unsigned int nr_succeeded = 0; /* * These counters track activities during zone compaction. Initialize @@ -2364,7 +2365,7 @@ compact_zone(struct compact_control *cc, err = migrate_pages(&cc->migratepages, compaction_alloc, compaction_free, (unsigned long)cc, cc->mode, - MR_COMPACTION); + MR_COMPACTION, &nr_succeeded); trace_mm_compaction_migratepages(cc->nr_migratepages, err, &cc->migratepages); diff -puN mm/gup.c~migrate_pages-add-success-return mm/gup.c --- a/mm/gup.c~migrate_pages-add-success-return 2021-03-31 15:17:14.150000255 -0700 +++ b/mm/gup.c 2021-03-31 15:17:14.190000255 -0700 @@ -1550,6 +1550,7 @@ static long check_and_migrate_cma_pages( unsigned long i; unsigned long step; bool drain_allow = true; + unsigned int nr_succeeded = 0; bool migrate_allow = true; LIST_HEAD(cma_page_list); long ret = nr_pages; @@ -1606,7 +1607,7 @@ check_again: put_page(pages[i]); if (migrate_pages(&cma_page_list, alloc_migration_target, NULL, - (unsigned long)&mtc, MIGRATE_SYNC, MR_CONTIG_RANGE)) { + (unsigned long)&mtc, MIGRATE_SYNC, MR_CONTIG_RANGE, &nr_succeeded)) { /* * some of the pages failed migration. Do get_user_pages * without migration. diff -puN mm/memory-failure.c~migrate_pages-add-success-return mm/memory-failure.c --- a/mm/memory-failure.c~migrate_pages-add-success-return 2021-03-31 15:17:14.155000255 -0700 +++ b/mm/memory-failure.c 2021-03-31 15:17:14.194000255 -0700 @@ -1809,6 +1809,7 @@ static int __soft_offline_page(struct pa unsigned long pfn = page_to_pfn(page); struct page *hpage = compound_head(page); char const *msg_page[] = {"page", "hugepage"}; + unsigned int nr_succeeded = 0; bool huge = PageHuge(page); LIST_HEAD(pagelist); struct migration_target_control mtc = { @@ -1852,7 +1853,8 @@ static int __soft_offline_page(struct pa if (isolate_page(hpage, &pagelist)) { ret = migrate_pages(&pagelist, alloc_migration_target, NULL, - (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE); + (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_FAILURE, + &nr_succeeded); if (!ret) { bool release = !huge; diff -puN mm/memory_hotplug.c~migrate_pages-add-success-return mm/memory_hotplug.c --- a/mm/memory_hotplug.c~migrate_pages-add-success-return 2021-03-31 15:17:14.160000255 -0700 +++ b/mm/memory_hotplug.c 2021-03-31 15:17:14.197000255 -0700 @@ -1392,6 +1392,7 @@ do_migrate_range(unsigned long start_pfn unsigned long pfn; struct page *page, *head; int ret = 0; + unsigned int nr_succeeded = 0; LIST_HEAD(source); for (pfn = start_pfn; pfn < end_pfn; pfn++) { @@ -1466,7 +1467,8 @@ do_migrate_range(unsigned long start_pfn if (nodes_empty(nmask)) node_set(mtc.nid, nmask); ret = migrate_pages(&source, alloc_migration_target, NULL, - (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_HOTPLUG); + (unsigned long)&mtc, MIGRATE_SYNC, MR_MEMORY_HOTPLUG, + &nr_succeeded); if (ret) { list_for_each_entry(page, &source, lru) { pr_warn("migrating pfn %lx failed ret:%d ", diff -puN mm/mempolicy.c~migrate_pages-add-success-return mm/mempolicy.c --- a/mm/mempolicy.c~migrate_pages-add-success-return 2021-03-31 15:17:14.163000255 -0700 +++ b/mm/mempolicy.c 2021-03-31 15:17:14.203000255 -0700 @@ -1081,6 +1081,7 @@ static int migrate_page_add(struct page static int migrate_to_node(struct mm_struct *mm, int source, int dest, int flags) { + unsigned int nr_succeeded = 0; nodemask_t nmask; LIST_HEAD(pagelist); int err = 0; @@ -1103,7 +1104,8 @@ static int migrate_to_node(struct mm_str if (!list_empty(&pagelist)) { err = migrate_pages(&pagelist, alloc_migration_target, NULL, - (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL); + (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL, + &nr_succeeded); if (err) putback_movable_pages(&pagelist); } @@ -1278,6 +1280,7 @@ static long do_mbind(unsigned long start nodemask_t *nmask, unsigned long flags) { struct mm_struct *mm = current->mm; + unsigned int nr_succeeded = 0; struct mempolicy *new; unsigned long end; int err; @@ -1355,7 +1358,8 @@ static long do_mbind(unsigned long start if (!list_empty(&pagelist)) { WARN_ON_ONCE(flags & MPOL_MF_LAZY); nr_failed = migrate_pages(&pagelist, new_page, NULL, - start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND); + start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND, + &nr_succeeded); if (nr_failed) putback_movable_pages(&pagelist); } diff -puN mm/migrate.c~migrate_pages-add-success-return mm/migrate.c --- a/mm/migrate.c~migrate_pages-add-success-return 2021-03-31 15:17:14.168000255 -0700 +++ b/mm/migrate.c 2021-03-31 15:17:14.207000255 -0700 @@ -1493,6 +1493,7 @@ static inline int try_split_thp(struct p * @mode: The migration mode that specifies the constraints for * page migration, if any. * @reason: The reason for page migration. + * @nr_succeeded: The number of pages migrated successfully. * * The function returns after 10 attempts or if no pages are movable any more * because the list has become empty or no retryable pages exist any more. @@ -1503,12 +1504,11 @@ static inline int try_split_thp(struct p */ int migrate_pages(struct list_head *from, new_page_t get_new_page, free_page_t put_new_page, unsigned long private, - enum migrate_mode mode, int reason) + enum migrate_mode mode, int reason, unsigned int *nr_succeeded) { int retry = 1; int thp_retry = 1; int nr_failed = 0; - int nr_succeeded = 0; int nr_thp_succeeded = 0; int nr_thp_failed = 0; int nr_thp_split = 0; @@ -1611,10 +1611,10 @@ retry: case MIGRATEPAGE_SUCCESS: if (is_thp) { nr_thp_succeeded++; - nr_succeeded += nr_subpages; + *nr_succeeded += nr_subpages; break; } - nr_succeeded++; + (*nr_succeeded)++; break; default: /* @@ -1643,12 +1643,12 @@ out: */ list_splice(&ret_pages, from); - count_vm_events(PGMIGRATE_SUCCESS, nr_succeeded); + count_vm_events(PGMIGRATE_SUCCESS, *nr_succeeded); count_vm_events(PGMIGRATE_FAIL, nr_failed); count_vm_events(THP_MIGRATION_SUCCESS, nr_thp_succeeded); count_vm_events(THP_MIGRATION_FAIL, nr_thp_failed); count_vm_events(THP_MIGRATION_SPLIT, nr_thp_split); - trace_mm_migrate_pages(nr_succeeded, nr_failed, nr_thp_succeeded, + trace_mm_migrate_pages(*nr_succeeded, nr_failed, nr_thp_succeeded, nr_thp_failed, nr_thp_split, mode, reason); if (!swapwrite) @@ -1716,6 +1716,7 @@ static int store_status(int __user *stat static int do_move_pages_to_node(struct mm_struct *mm, struct list_head *pagelist, int node) { + unsigned int nr_succeeded = 0; int err; struct migration_target_control mtc = { .nid = node, @@ -1723,7 +1724,8 @@ static int do_move_pages_to_node(struct }; err = migrate_pages(pagelist, alloc_migration_target, NULL, - (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL); + (unsigned long)&mtc, MIGRATE_SYNC, MR_SYSCALL, + &nr_succeeded); if (err) putback_movable_pages(pagelist); return err; @@ -2207,6 +2209,7 @@ int migrate_misplaced_page(struct page * pg_data_t *pgdat = NODE_DATA(node); int isolated; int nr_remaining; + unsigned int nr_succeeded = 0; LIST_HEAD(migratepages); /* @@ -2230,7 +2233,7 @@ int migrate_misplaced_page(struct page * list_add(&page->lru, &migratepages); nr_remaining = migrate_pages(&migratepages, alloc_misplaced_dst_page, NULL, node, MIGRATE_ASYNC, - MR_NUMA_MISPLACED); + MR_NUMA_MISPLACED, &nr_succeeded); if (nr_remaining) { if (!list_empty(&migratepages)) { list_del(&page->lru); diff -puN mm/page_alloc.c~migrate_pages-add-success-return mm/page_alloc.c --- a/mm/page_alloc.c~migrate_pages-add-success-return 2021-03-31 15:17:14.178000255 -0700 +++ b/mm/page_alloc.c 2021-03-31 15:17:14.213000255 -0700 @@ -8452,7 +8452,8 @@ static unsigned long pfn_max_align_up(un /* [start, end) must belong to a single zone. */ static int __alloc_contig_migrate_range(struct compact_control *cc, - unsigned long start, unsigned long end) + unsigned long start, unsigned long end, + unsigned int *nr_succeeded) { /* This function is based on compact_zone() from compaction.c. */ unsigned int nr_reclaimed; @@ -8490,7 +8491,8 @@ static int __alloc_contig_migrate_range( cc->nr_migratepages -= nr_reclaimed; ret = migrate_pages(&cc->migratepages, alloc_migration_target, - NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE); + NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE, + nr_succeeded); } if (ret < 0) { putback_movable_pages(&cc->migratepages); @@ -8526,6 +8528,7 @@ int alloc_contig_range(unsigned long sta unsigned long outer_start, outer_end; unsigned int order; int ret = 0; + unsigned int nr_succeeded = 0; struct compact_control cc = { .nr_migratepages = 0, @@ -8580,7 +8583,7 @@ int alloc_contig_range(unsigned long sta * allocated. So, if we fall through be sure to clear ret so that * -EBUSY is not accidentally used or returned to caller. */ - ret = __alloc_contig_migrate_range(&cc, start, end); + ret = __alloc_contig_migrate_range(&cc, start, end, &nr_succeeded); if (ret && ret != -EBUSY) goto done; ret =0;