| Message ID | 1573755869-106954-1-git-send-email-yang.shi@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mm: migrate: handle freed page at the first place | expand |
On Fri 15-11-19 02:24:29, Yang Shi wrote: > When doing migration if the freed page is met, we just return without > migrating it since it is pointless to migrate a freed page. But, the > current code allocates target page unconditionally before handling freed > page, if the page is freed, the newly allocated will be just freed. It > doesn't make too much sense and is just a waste of time although > migrating freed page is rare. > > So, handle freed page at the before that to avoid unnecessary page > allocation and free. > > Cc: Michal Hocko <mhocko@suse.com> > Cc: Mel Gorman <mgorman@techsingularity.net> > Cc: Vlastimil Babka <vbabka@suse.cz> > Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> I would be really surprised if this led to any runtime visible effect but I do agree that one less put_page path looks slightly better. For that reason Acked-by: Michal Hocko <mhocko@suse.com> > --- > v2: * Keep thp migration support check before handling freed page per Michal Hocko > * Fixed the build warning reported by 0-day > > mm/migrate.c | 14 +++++--------- > 1 file changed, 5 insertions(+), 9 deletions(-) > > diff --git a/mm/migrate.c b/mm/migrate.c > index 4fe45d1..a8f87cb 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -1168,15 +1168,11 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page, > enum migrate_reason reason) > { > int rc = MIGRATEPAGE_SUCCESS; > - struct page *newpage; > + struct page *newpage = NULL; > > if (!thp_migration_supported() && PageTransHuge(page)) > return -ENOMEM; > > - newpage = get_new_page(page, private); > - if (!newpage) > - return -ENOMEM; > - > if (page_count(page) == 1) { > /* page was freed from under us. So we are done. */ > ClearPageActive(page); > @@ -1187,13 +1183,13 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page, > __ClearPageIsolated(page); > unlock_page(page); > } > - if (put_new_page) > - put_new_page(newpage, private); > - else > - put_page(newpage); > goto out; > } > > + newpage = get_new_page(page, private); > + if (!newpage) > + return -ENOMEM; > + > rc = __unmap_and_move(page, newpage, force, mode); > if (rc == MIGRATEPAGE_SUCCESS) > set_page_owner_migrate_reason(newpage, reason); > -- > 1.8.3.1 >
On 11/14/19 10:56 AM, Michal Hocko wrote: > On Fri 15-11-19 02:24:29, Yang Shi wrote: >> When doing migration if the freed page is met, we just return without >> migrating it since it is pointless to migrate a freed page. But, the >> current code allocates target page unconditionally before handling freed >> page, if the page is freed, the newly allocated will be just freed. It >> doesn't make too much sense and is just a waste of time although >> migrating freed page is rare. >> >> So, handle freed page at the before that to avoid unnecessary page >> allocation and free. >> >> Cc: Michal Hocko <mhocko@suse.com> >> Cc: Mel Gorman <mgorman@techsingularity.net> >> Cc: Vlastimil Babka <vbabka@suse.cz> >> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> > I would be really surprised if this led to any runtime visible effect > but I do agree that one less put_page path looks slightly better. For > that reason > Acked-by: Michal Hocko <mhocko@suse.com> Thanks! > >> --- >> v2: * Keep thp migration support check before handling freed page per Michal Hocko >> * Fixed the build warning reported by 0-day >> >> mm/migrate.c | 14 +++++--------- >> 1 file changed, 5 insertions(+), 9 deletions(-) >> >> diff --git a/mm/migrate.c b/mm/migrate.c >> index 4fe45d1..a8f87cb 100644 >> --- a/mm/migrate.c >> +++ b/mm/migrate.c >> @@ -1168,15 +1168,11 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page, >> enum migrate_reason reason) >> { >> int rc = MIGRATEPAGE_SUCCESS; >> - struct page *newpage; >> + struct page *newpage = NULL; >> >> if (!thp_migration_supported() && PageTransHuge(page)) >> return -ENOMEM; >> >> - newpage = get_new_page(page, private); >> - if (!newpage) >> - return -ENOMEM; >> - >> if (page_count(page) == 1) { >> /* page was freed from under us. So we are done. */ >> ClearPageActive(page); >> @@ -1187,13 +1183,13 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page, >> __ClearPageIsolated(page); >> unlock_page(page); >> } >> - if (put_new_page) >> - put_new_page(newpage, private); >> - else >> - put_page(newpage); >> goto out; >> } >> >> + newpage = get_new_page(page, private); >> + if (!newpage) >> + return -ENOMEM; >> + >> rc = __unmap_and_move(page, newpage, force, mode); >> if (rc == MIGRATEPAGE_SUCCESS) >> set_page_owner_migrate_reason(newpage, reason); >> -- >> 1.8.3.1 >>
On 2019-11-15 at 04:58 Yang Shi wrote: > > >On 11/14/19 10:56 AM, Michal Hocko wrote: >> On Fri 15-11-19 02:24:29, Yang Shi wrote: >>> When doing migration if the freed page is met, we just return without >>> migrating it since it is pointless to migrate a freed page. But, the >>> current code allocates target page unconditionally before handling freed >>> page, if the page is freed, the newly allocated will be just freed. It >>> doesn't make too much sense and is just a waste of time although >>> migrating freed page is rare. >>> >>> So, handle freed page at the before that to avoid unnecessary page >>> allocation and free. >>> >>> Cc: Michal Hocko <mhocko@suse.com> >>> Cc: Mel Gorman <mgorman@techsingularity.net> >>> Cc: Vlastimil Babka <vbabka@suse.cz> >>> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> >> I would be really surprised if this led to any runtime visible effect >> but I do agree that one less put_page path looks slightly better. For >> that reason >> Acked-by: Michal Hocko <mhocko@suse.com> > >Thanks! > I think the original design wants to complete all preparation work before real migration starts with lock on both 'page' and 'newpage', and any race condition is caught right before real work start. (with this patch, page_count(page)==1 still possible after allocate 'newpage', but we don't check it. Actually, freeing 'page' would be evenly distributed during the whole procedure of the preparation phase.) The __unmap_and_move() is called right after page_count() check, and where locking 'page' and 'newpage' is done immediately. - Xinhai >> >>> --- >>> v2: * Keep thp migration support check before handling freed page per Michal Hocko >>> * Fixed the build warning reported by 0-day >>> >>> mm/migrate.c | 14 +++++--------- >>> 1 file changed, 5 insertions(+), 9 deletions(-) >>> >>> diff --git a/mm/migrate.c b/mm/migrate.c >>> index 4fe45d1..a8f87cb 100644 >>> --- a/mm/migrate.c >>> +++ b/mm/migrate.c >>> @@ -1168,15 +1168,11 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page, >>> enum migrate_reason reason) >>> { >>> int rc = MIGRATEPAGE_SUCCESS; >>> - struct page *newpage; >>> + struct page *newpage = NULL; >>> >>> if (!thp_migration_supported() && PageTransHuge(page)) >>> return -ENOMEM; >>> >>> - newpage = get_new_page(page, private); >>> - if (!newpage) >>> - return -ENOMEM; >>> - >>> if (page_count(page) == 1) { >>> /* page was freed from under us. So we are done. */ >>> ClearPageActive(page); >>> @@ -1187,13 +1183,13 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page, >>> __ClearPageIsolated(page); >>> unlock_page(page); >>> } >>> - if (put_new_page) >>> - put_new_page(newpage, private); >>> - else >>> - put_page(newpage); >>> goto out; >>> } >>> >>> + newpage = get_new_page(page, private); >>> + if (!newpage) >>> + return -ENOMEM; >>> + >>> rc = __unmap_and_move(page, newpage, force, mode); >>> if (rc == MIGRATEPAGE_SUCCESS) >>> set_page_owner_migrate_reason(newpage, reason); >>> -- >>> 1.8.3.1 >>> > >
diff --git a/mm/migrate.c b/mm/migrate.c index 4fe45d1..a8f87cb 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -1168,15 +1168,11 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page, enum migrate_reason reason) { int rc = MIGRATEPAGE_SUCCESS; - struct page *newpage; + struct page *newpage = NULL; if (!thp_migration_supported() && PageTransHuge(page)) return -ENOMEM; - newpage = get_new_page(page, private); - if (!newpage) - return -ENOMEM; - if (page_count(page) == 1) { /* page was freed from under us. So we are done. */ ClearPageActive(page); @@ -1187,13 +1183,13 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page, __ClearPageIsolated(page); unlock_page(page); } - if (put_new_page) - put_new_page(newpage, private); - else - put_page(newpage); goto out; } + newpage = get_new_page(page, private); + if (!newpage) + return -ENOMEM; + rc = __unmap_and_move(page, newpage, force, mode); if (rc == MIGRATEPAGE_SUCCESS) set_page_owner_migrate_reason(newpage, reason);
When doing migration if the freed page is met, we just return without migrating it since it is pointless to migrate a freed page. But, the current code allocates target page unconditionally before handling freed page, if the page is freed, the newly allocated will be just freed. It doesn't make too much sense and is just a waste of time although migrating freed page is rare. So, handle freed page at the before that to avoid unnecessary page allocation and free. Cc: Michal Hocko <mhocko@suse.com> Cc: Mel Gorman <mgorman@techsingularity.net> Cc: Vlastimil Babka <vbabka@suse.cz> Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com> --- v2: * Keep thp migration support check before handling freed page per Michal Hocko * Fixed the build warning reported by 0-day mm/migrate.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-)