| Message ID | 1537798495-4996-1-git-send-email-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/migrate: Split only transparent huge pages when allocation fails | expand |
On Mon 24-09-18 19:44:55, Anshuman Khandual wrote: > When unmap_and_move[_huge_page] function fails due to lack of memory, the > splitting should happen only for transparent huge pages not for HugeTLB > pages. PageTransHuge() returns true for both THP and HugeTLB pages. Hence > the conditonal check should test PagesHuge() flag to make sure that given > pages is not a HugeTLB one. Well spotted! Have you actually seen this happening or this is review driven? I am wondering what would be the real effect of this mismatch? I have tried to follow to code path but I suspect split_huge_page_to_list would fail for hugetlbfs pages. If there is a more serious effect then we should mark the patch for stable as well. > > Fixes: 94723aafb9 ("mm: unclutter THP migration") > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> Acked-by: Michal Hocko <mhocko@suse.com> > --- > mm/migrate.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/migrate.c b/mm/migrate.c > index d6a2e89..d2297fe 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -1411,7 +1411,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page, > * we encounter them after the rest of the list > * is processed. > */ > - if (PageTransHuge(page)) { > + if (PageTransHuge(page) && !PageHuge(page)) { > lock_page(page); > rc = split_huge_page_to_list(page, from); > unlock_page(page); > -- > 2.7.4
On 09/24/2018 08:00 PM, Michal Hocko wrote: > On Mon 24-09-18 19:44:55, Anshuman Khandual wrote: >> When unmap_and_move[_huge_page] function fails due to lack of memory, the >> splitting should happen only for transparent huge pages not for HugeTLB >> pages. PageTransHuge() returns true for both THP and HugeTLB pages. Hence >> the conditonal check should test PagesHuge() flag to make sure that given >> pages is not a HugeTLB one. > > Well spotted! Have you actually seen this happening or this is review > driven? I am wondering what would be the real effect of this mismatch? > I have tried to follow to code path but I suspect > split_huge_page_to_list would fail for hugetlbfs pages. If there is a > more serious effect then we should mark the patch for stable as well. split_huge_page_to_list() fails on HugeTLB pages. I was experimenting around moving 32MB contig HugeTLB pages on arm64 (with a debug patch applied) hit the following stack trace when the kernel crashed. [ 3732.462797] Call trace: [ 3732.462835] split_huge_page_to_list+0x3b0/0x858 [ 3732.462913] migrate_pages+0x728/0xc20 [ 3732.462999] soft_offline_page+0x448/0x8b0 [ 3732.463097] __arm64_sys_madvise+0x724/0x850 [ 3732.463197] el0_svc_handler+0x74/0x110 [ 3732.463297] el0_svc+0x8/0xc [ 3732.463347] Code: d1000400 f90b0e60 f2fbd5a2 a94982a1 (f9000420)
On Mon 24-09-18 22:10:30, Anshuman Khandual wrote: > > > On 09/24/2018 08:00 PM, Michal Hocko wrote: > > On Mon 24-09-18 19:44:55, Anshuman Khandual wrote: > >> When unmap_and_move[_huge_page] function fails due to lack of memory, the > >> splitting should happen only for transparent huge pages not for HugeTLB > >> pages. PageTransHuge() returns true for both THP and HugeTLB pages. Hence > >> the conditonal check should test PagesHuge() flag to make sure that given > >> pages is not a HugeTLB one. > > > > Well spotted! Have you actually seen this happening or this is review > > driven? I am wondering what would be the real effect of this mismatch? > > I have tried to follow to code path but I suspect > > split_huge_page_to_list would fail for hugetlbfs pages. If there is a > > more serious effect then we should mark the patch for stable as well. > > split_huge_page_to_list() fails on HugeTLB pages. I was experimenting around > moving 32MB contig HugeTLB pages on arm64 (with a debug patch applied) hit > the following stack trace when the kernel crashed. > > [ 3732.462797] Call trace: > [ 3732.462835] split_huge_page_to_list+0x3b0/0x858 > [ 3732.462913] migrate_pages+0x728/0xc20 > [ 3732.462999] soft_offline_page+0x448/0x8b0 > [ 3732.463097] __arm64_sys_madvise+0x724/0x850 > [ 3732.463197] el0_svc_handler+0x74/0x110 > [ 3732.463297] el0_svc+0x8/0xc > [ 3732.463347] Code: d1000400 f90b0e60 f2fbd5a2 a94982a1 (f9000420) Please make sure this makes it to the changelog and mark the patch for stable.
diff --git a/mm/migrate.c b/mm/migrate.c index d6a2e89..d2297fe 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -1411,7 +1411,7 @@ int migrate_pages(struct list_head *from, new_page_t get_new_page, * we encounter them after the rest of the list * is processed. */ - if (PageTransHuge(page)) { + if (PageTransHuge(page) && !PageHuge(page)) { lock_page(page); rc = split_huge_page_to_list(page, from); unlock_page(page);
When unmap_and_move[_huge_page] function fails due to lack of memory, the splitting should happen only for transparent huge pages not for HugeTLB pages. PageTransHuge() returns true for both THP and HugeTLB pages. Hence the conditonal check should test PagesHuge() flag to make sure that given pages is not a HugeTLB one. Fixes: 94723aafb9 ("mm: unclutter THP migration") Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> --- mm/migrate.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)