lkml.org 
[lkml]   [2021]   [Apr]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 04/10] mm/migrate: make migrate_pages() return nr_succeeded
    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?

    --
    Oscar Salvador
    SUSE L3

    \
     
     \ /
      Last update: 2021-04-08 12:15    [W:2.633 / U:0.184 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site