On Thu, Apr 01, 2021 at 11:32:23AM -0700, Dave Hansen wrote:
>
> From: Yang Shi <yang.shi@xxxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxx>
> Signed-off-by: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> Reviewed-by: Yang Shi <shy828301@xxxxxxxxx>
> Cc: Wei Xu <weixugc@xxxxxxxxxx>
> Cc: Huang Ying <ying.huang@xxxxxxxxx>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> Cc: David Hildenbrand <david@xxxxxxxxxx>
> Cc: osalvador <osalvador@xxxxxxx>
>
...
> 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?
--
Oscar Salvador
SUSE L3
