> -----Original Message----- > From: Zach O'Keefe <zokeefe@xxxxxxxxxx> > Sent: Tuesday, August 22, 2023 5:19 AM > To: linux-mm@xxxxxxxxx; Yang Shi <shy828301@xxxxxxxxx> > Cc: linux-kernel@xxxxxxxxxxxxxxx; Zach O'Keefe <zokeefe@xxxxxxxxxx>; > Saurabh Singh Sengar <ssengar@xxxxxxxxxxxxx> > Subject: [EXTERNAL] [PATCH v3] mm/thp: fix "mm: thp: kill > __transhuge_page_enabled()" > > [You don't often get email from zokeefe@xxxxxxxxxx. Learn why this is > important at https://aka.ms/LearnAboutSenderIdentification ] > > The 6.0 commits: > > commit 9fec51689ff6 ("mm: thp: kill transparent_hugepage_active()") commit > 7da4e2cb8b1f ("mm: thp: kill __transhuge_page_enabled()") > > merged "can we have THPs in this VMA?" logic that was previously done > separately by fault-path, khugepaged, and smaps "THPeligible" checks. > > During the process, the semantics of the fault path check changed in two > ways: > > 1) A VM_NO_KHUGEPAGED check was introduced (also added to smaps > path). > 2) We no longer checked if non-anonymous memory had a vm_ops- > >huge_fault > handler that could satisfy the fault. Previously, this check had been > done in create_huge_pud() and create_huge_pmd() routines, but after > the changes, we never reach those routines. > > During the review of the above commits, it was determined that in-tree users > weren't affected by the change; most notably, since the only relevant user (in > terms of THP) of VM_MIXEDMAP or ->huge_fault is DAX, which is explicitly > approved early in approval logic. However, there is at least one occurrence > where an out-of-tree driver that used VM_HUGEPAGE|VM_MIXEDMAP with a > vm_ops->huge_fault handler, was broken. > > Remove the VM_NO_KHUGEPAGED check when not in collapse path and give > any ->huge_fault handler a chance to handle the fault. Note that we don't > validate the file mode or mapping alignment, which is consistent with the > behavior before the aforementioned commits. > > Fixes: 7da4e2cb8b1f ("mm: thp: kill __transhuge_page_enabled()") > Reported-by: Saurabh Singh Sengar <ssengar@xxxxxxxxxxxxx> > Signed-off-by: Zach O'Keefe <zokeefe@xxxxxxxxxx> > Cc: Yang Shi <shy828301@xxxxxxxxx> > --- > Changed from v2[1]: > - Fixed false negative in smaps check when !dax && ->huge_fault > Changed from v1[2]: > - [Saurabhi] Allow ->huge_fault handler to handle fault, if it exists > > There are some logical holes in smaps' THPeligible checks here, but those are > best dealt with in follow-up patches. For now, just make sure the fault path is > dealt with. > > [1] > https://lore.k/ > ernel.org%2Flinux-mm%2F20230818211533.2523697-1- > zokeefe%40google.com%2F&data=05%7C01%7Cssengar%40microsoft.com%7 > Ce782558e7bce4f9d060608dba2a12b58%7C72f988bf86f141af91ab2d7cd011 > db47%7C1%7C0%7C638282585367952964%7CUnknown%7CTWFpbGZsb3d8e > yJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D > %7C3000%7C%7C%7C&sdata=t%2FwAGlOyKmKp%2FnDPGv9cl2j3h%2F3xVuV > Y%2BQqeu3A4HHk%3D&reserved=0 > [2] > https://lore.k/ > ernel.org%2Flinux- > mm%2FCAAa6QmQw%2BF%3Do6htOn%3D6ADD6mwvMO%3DOw_67f3ifBv3 > GpXx9Xg_g%40mail.gmail.com%2F&data=05%7C01%7Cssengar%40microsoft. > com%7Ce782558e7bce4f9d060608dba2a12b58%7C72f988bf86f141af91ab2d > 7cd011db47%7C1%7C0%7C638282585367952964%7CUnknown%7CTWFpbGZ > sb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6M > n0%3D%7C3000%7C%7C%7C&sdata=lT6ZqrOBoVIcPbOH%2BHto5pPTmpC6pk > QMu58gnKG7aLo%3D&reserved=0 > > --- > mm/huge_memory.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c index > eb3678360b97..901dcf8db8d2 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -96,11 +96,11 @@ bool hugepage_vma_check(struct vm_area_struct > *vma, unsigned long vm_flags, > return in_pf; > > /* > - * Special VMA and hugetlb VMA. > + * khugepaged special VMA and hugetlb VMA. > * Must be checked after dax since some dax mappings may have > * VM_MIXEDMAP set. > */ > - if (vm_flags & VM_NO_KHUGEPAGED) > + if (!in_pf && !smaps && (vm_flags & VM_NO_KHUGEPAGED)) > return false; > > /* > @@ -128,12 +128,18 @@ bool hugepage_vma_check(struct vm_area_struct > *vma, unsigned long vm_flags, > !hugepage_flags_always()))) > return false; > > - /* Only regular file is valid */ > - if (!in_pf && file_thp_enabled(vma)) > - return true; > - > - if (!vma_is_anonymous(vma)) > + if (!vma_is_anonymous(vma)) { > + /* > + * Trust that ->huge_fault() handlers know what they are doing > + * in fault path. > + */ > + if (((in_pf || smaps)) && vma->vm_ops->huge_fault) > + return true; Thanks for the patch. I have tested it, looks good to me. - Saurabh > + /* Only regular file is valid in collapse path */ > + if (((!in_pf || smaps)) && file_thp_enabled(vma)) > + return true; > return false; > + } > > if (vma_is_temporary_stack(vma)) > return false; > -- > 2.42.0.rc1.204.g551eb34607-goog
