| Message ID | 1573510165-113395-1-git-send-email-yang.shi@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: migrate: handle freed page at the first place | expand |
On Tue, 12 Nov 2019 06:09:25 +0800 Yang Shi <yang.shi@linux.alibaba.com> 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 did two things before handling freed page: > > 1. Return -ENOMEM if the page is THP and THP migration is not supported. > 2. Allocate target page unconditionally. > > Both makes not too much sense. If we handle freed page at the first place > we don't have to worry about allocating/freeing target page and split > THP at all. > > For example (worst case) if we are trying to migrate a freed THP without > THP migration supported, the migrate_pages() would just split the THP then > retry to migrate base pages one by one by pointless allocating and freeing > pages, this is just waste of time. > > I didn't run into any actual problem with the current code (or I may > just not notice it yet), it was found by visual inspection. > > > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -1170,13 +1170,6 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page, > int rc = MIGRATEPAGE_SUCCESS; > struct page *newpage; > > - if (!thp_migration_supported() && PageTransHuge(page)) > - return -ENOMEM; > - > - newpage = get_new_page(page, private); > - if (!newpage) > - return -ENOMEM; > - > if (page_count(page) == 1) { Is it possible to have (!thp_migration_supported() && PageTransHuge(page) && page_count(page) == 1)? If so, isn't this new behviour? > /* page was freed from under us. So we are done. */ > ClearPageActive(page); > @@ -1187,13 +1180,16 @@ 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; > } > > + if (!thp_migration_supported() && PageTransHuge(page)) > + return -ENOMEM; > + > + 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);
On 11/11/19 3:18 PM, Andrew Morton wrote: > On Tue, 12 Nov 2019 06:09:25 +0800 Yang Shi <yang.shi@linux.alibaba.com> 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 did two things before handling freed page: >> >> 1. Return -ENOMEM if the page is THP and THP migration is not supported. >> 2. Allocate target page unconditionally. >> >> Both makes not too much sense. If we handle freed page at the first place >> we don't have to worry about allocating/freeing target page and split >> THP at all. >> >> For example (worst case) if we are trying to migrate a freed THP without >> THP migration supported, the migrate_pages() would just split the THP then >> retry to migrate base pages one by one by pointless allocating and freeing >> pages, this is just waste of time. >> >> I didn't run into any actual problem with the current code (or I may >> just not notice it yet), it was found by visual inspection. >> >> >> --- a/mm/migrate.c >> +++ b/mm/migrate.c >> @@ -1170,13 +1170,6 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page, >> int rc = MIGRATEPAGE_SUCCESS; >> struct page *newpage; >> >> - if (!thp_migration_supported() && PageTransHuge(page)) >> - return -ENOMEM; >> - >> - newpage = get_new_page(page, private); >> - if (!newpage) >> - return -ENOMEM; >> - >> if (page_count(page) == 1) { > Is it possible to have (!thp_migration_supported() && > PageTransHuge(page) && page_count(page) == 1)? If so, isn't this new > behviour? IMHO it should be possible on some architectures, i.e. aarch64, with anonymous THP. I just saw PowerPC and x86_64 have CONFIG_ARCH_ENABLE_THP_MIGRATION selected. I'm not quite sure if I miss something. It should be not new behavior since migrate_pages() should just split the THP then retry with base pages one by one. Even though it returns -EBUSY due to THP split failure in the current code, the behavior sounds problematic. We should not return errno for a freed page, right? > >> /* page was freed from under us. So we are done. */ >> ClearPageActive(page); >> @@ -1187,13 +1180,16 @@ 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; >> } >> >> + if (!thp_migration_supported() && PageTransHuge(page)) >> + return -ENOMEM; >> + >> + 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);
On Tue 12-11-19 06:09:25, 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 did two things before handling freed page: > > 1. Return -ENOMEM if the page is THP and THP migration is not supported. > 2. Allocate target page unconditionally. > > Both makes not too much sense. If we handle freed page at the first place > we don't have to worry about allocating/freeing target page and split > THP at all. > > For example (worst case) if we are trying to migrate a freed THP without > THP migration supported, the migrate_pages() would just split the THP then > retry to migrate base pages one by one by pointless allocating and freeing > pages, this is just waste of time. > > I didn't run into any actual problem with the current code (or I may > just not notice it yet), it was found by visual inspection. It would be preferable to accompany a change like this with some actual numbers. A race with page freeing should be a very rare situation. Maybe it is not under some workloads but that would better be checked and documented. I also do not like to do page state changes for THP migration without a support. I cannot really say this is 100% correct from top of my head and I do not see a sufficient justification to go and chase all those tiny details because that is time consuming. > 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> > --- > mm/migrate.c | 18 +++++++----------- > 1 file changed, 7 insertions(+), 11 deletions(-) > > diff --git a/mm/migrate.c b/mm/migrate.c > index 4fe45d1..ef96997 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -1170,13 +1170,6 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page, > int rc = MIGRATEPAGE_SUCCESS; > struct page *newpage; > > - 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 +1180,16 @@ 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; > } > > + if (!thp_migration_supported() && PageTransHuge(page)) > + return -ENOMEM; > + > + 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 Tue 12-11-19 09:04:01, Michal Hocko wrote: > On Tue 12-11-19 06:09:25, 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 did two things before handling freed page: > > > > 1. Return -ENOMEM if the page is THP and THP migration is not supported. > > 2. Allocate target page unconditionally. > > > > Both makes not too much sense. If we handle freed page at the first place > > we don't have to worry about allocating/freeing target page and split > > THP at all. > > > > For example (worst case) if we are trying to migrate a freed THP without > > THP migration supported, the migrate_pages() would just split the THP then > > retry to migrate base pages one by one by pointless allocating and freeing > > pages, this is just waste of time. > > > > I didn't run into any actual problem with the current code (or I may > > just not notice it yet), it was found by visual inspection. > > It would be preferable to accompany a change like this with some actual > numbers. A race with page freeing should be a very rare situation. Maybe > it is not under some workloads but that would better be checked and > documented. I also do not like to do page state changes for THP > migration without a support. I cannot really say this is 100% correct > from top of my head and I do not see a sufficient justification to go > and chase all those tiny details because that is time consuming. And I forgot to mention one thing. I wouldn't be really opposed to moving the allocation after the race check because that makes sense even when the race is rare but moving the thp support check down is far from clear without a much better justification.
On 11/12/19 12:04 AM, Michal Hocko wrote: > On Tue 12-11-19 06:09:25, 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 did two things before handling freed page: >> >> 1. Return -ENOMEM if the page is THP and THP migration is not supported. >> 2. Allocate target page unconditionally. >> >> Both makes not too much sense. If we handle freed page at the first place >> we don't have to worry about allocating/freeing target page and split >> THP at all. >> >> For example (worst case) if we are trying to migrate a freed THP without >> THP migration supported, the migrate_pages() would just split the THP then >> retry to migrate base pages one by one by pointless allocating and freeing >> pages, this is just waste of time. >> >> I didn't run into any actual problem with the current code (or I may >> just not notice it yet), it was found by visual inspection. > It would be preferable to accompany a change like this with some actual > numbers. A race with page freeing should be a very rare situation. Maybe > it is not under some workloads but that would better be checked and > documented. I also do not like to do page state changes for THP > migration without a support. I cannot really say this is 100% correct However that THP will not be migrated actually, just will be freed by put_page(). > from top of my head and I do not see a sufficient justification to go > and chase all those tiny details because that is time consuming. > >> 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> >> --- >> mm/migrate.c | 18 +++++++----------- >> 1 file changed, 7 insertions(+), 11 deletions(-) >> >> diff --git a/mm/migrate.c b/mm/migrate.c >> index 4fe45d1..ef96997 100644 >> --- a/mm/migrate.c >> +++ b/mm/migrate.c >> @@ -1170,13 +1170,6 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page, >> int rc = MIGRATEPAGE_SUCCESS; >> struct page *newpage; >> >> - 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 +1180,16 @@ 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; >> } >> >> + if (!thp_migration_supported() && PageTransHuge(page)) >> + return -ENOMEM; >> + >> + 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..ef96997 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -1170,13 +1170,6 @@ static ICE_noinline int unmap_and_move(new_page_t get_new_page, int rc = MIGRATEPAGE_SUCCESS; struct page *newpage; - 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 +1180,16 @@ 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; } + if (!thp_migration_supported() && PageTransHuge(page)) + return -ENOMEM; + + 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 did two things before handling freed page: 1. Return -ENOMEM if the page is THP and THP migration is not supported. 2. Allocate target page unconditionally. Both makes not too much sense. If we handle freed page at the first place we don't have to worry about allocating/freeing target page and split THP at all. For example (worst case) if we are trying to migrate a freed THP without THP migration supported, the migrate_pages() would just split the THP then retry to migrate base pages one by one by pointless allocating and freeing pages, this is just waste of time. I didn't run into any actual problem with the current code (or I may just not notice it yet), it was found by visual inspection. 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> --- mm/migrate.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-)