| Message ID | 20241206070345.2526501-1-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [-next] mm: don't try THP align for FS without get_unmapped_area | expand |
On 12/6/24 08:03, Kefeng Wang wrote: > Commit ed48e87c7df3 ("thp: add thp_get_unmapped_area_vmflags()") > changes thp_get_unmapped_area() to thp_get_unmapped_area_vmflags() > in __get_unmapped_area(), which won't setup get_area for anonymous > mappings, but it leads to always try THP align when file ops without > '.get_unmapped_area' callback too as the get_area is NULL. > > Since commit efa7df3e3bb5 ("mm: align larger anonymous mappings on > THP boundaries") only want to enable THP align for anonymous, adding > !file check to fix it. > > Fixes: ed48e87c7df3 ("thp: add thp_get_unmapped_area_vmflags()") > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> Yeah, well spotted. Cc stable as we did for the others? Reviewed-by: Vlastimil Babka <vbabka@suse.cz> > --- > mm/mmap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/mmap.c b/mm/mmap.c > index 1c6bdffa13dd..b373486bd1c6 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -782,7 +782,7 @@ __get_unmapped_area(struct file *file, unsigned long addr, unsigned long len, > > if (get_area) { > addr = get_area(file, addr, len, pgoff, flags); > - } else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) > + } else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) && !file > && !addr /* no hint */ > && IS_ALIGNED(len, PMD_SIZE)) { > /* Ensures that larger anonymous mappings are THP aligned. */
On Thu, Dec 5, 2024 at 11:04 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > > Commit ed48e87c7df3 ("thp: add thp_get_unmapped_area_vmflags()") > changes thp_get_unmapped_area() to thp_get_unmapped_area_vmflags() > in __get_unmapped_area(), which won't setup get_area for anonymous > mappings, but it leads to always try THP align when file ops without > '.get_unmapped_area' callback too as the get_area is NULL. > > Since commit efa7df3e3bb5 ("mm: align larger anonymous mappings on > THP boundaries") only want to enable THP align for anonymous, adding > !file check to fix it. Good catch. Reviewed-by: Yang Shi <shy828301@gmail.com> > > Fixes: ed48e87c7df3 ("thp: add thp_get_unmapped_area_vmflags()") > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > --- > mm/mmap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/mmap.c b/mm/mmap.c > index 1c6bdffa13dd..b373486bd1c6 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -782,7 +782,7 @@ __get_unmapped_area(struct file *file, unsigned long addr, unsigned long len, > > if (get_area) { > addr = get_area(file, addr, len, pgoff, flags); > - } else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) > + } else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) && !file > && !addr /* no hint */ > && IS_ALIGNED(len, PMD_SIZE)) { > /* Ensures that larger anonymous mappings are THP aligned. */ > -- > 2.27.0 >
On Fri, 6 Dec 2024 15:03:45 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > Commit ed48e87c7df3 ("thp: add thp_get_unmapped_area_vmflags()") > changes thp_get_unmapped_area() to thp_get_unmapped_area_vmflags() > in __get_unmapped_area(), which won't setup get_area for anonymous > mappings, but it leads to always try THP align when file ops without > '.get_unmapped_area' callback too as the get_area is NULL. > > Since commit efa7df3e3bb5 ("mm: align larger anonymous mappings on > THP boundaries") only want to enable THP align for anonymous, adding > !file check to fix it. The above is tough. I attempted a rewrite, please review for accuracy and completeness: : Commit ed48e87c7df3 ("thp: add thp_get_unmapped_area_vmflags()") changes : thp_get_unmapped_area() to thp_get_unmapped_area_vmflags() in : __get_unmapped_area(), which doesn't initialize local get_area for : anonymous mappings. This leads to us always trying THP alignment even for : file_operations which have a NULL ->get_unmapped_area() callback. : : Since commit efa7df3e3bb5 ("mm: align larger anonymous mappings on THP : boundaries") we only want to enable THP alignment for anonymous mappings, : so add a !file check to avoid attempting THP alignment for file mappings. Also, the changelog failed to describe the userspace-visible effects of the flaw, which is basically essential when fixing bugs. The bug has been there since 6.10 so it would be interesting to learn why it took this long to be noticed.
On 2024/12/7 14:34, Andrew Morton wrote: > On Fri, 6 Dec 2024 15:03:45 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > >> Commit ed48e87c7df3 ("thp: add thp_get_unmapped_area_vmflags()") >> changes thp_get_unmapped_area() to thp_get_unmapped_area_vmflags() >> in __get_unmapped_area(), which won't setup get_area for anonymous >> mappings, but it leads to always try THP align when file ops without >> '.get_unmapped_area' callback too as the get_area is NULL. >> >> Since commit efa7df3e3bb5 ("mm: align larger anonymous mappings on >> THP boundaries") only want to enable THP align for anonymous, adding >> !file check to fix it. > > The above is tough. I attempted a rewrite, please review for accuracy > and completeness: Forgive my English, thanks for rewriting the better changelog. > > : Commit ed48e87c7df3 ("thp: add thp_get_unmapped_area_vmflags()") changes > : thp_get_unmapped_area() to thp_get_unmapped_area_vmflags() in > : __get_unmapped_area(), which doesn't initialize local get_area for > : anonymous mappings. This leads to us always trying THP alignment even for > : file_operations which have a NULL ->get_unmapped_area() callback. > : > : Since commit efa7df3e3bb5 ("mm: align larger anonymous mappings on THP > : boundaries") we only want to enable THP alignment for anonymous mappings, > : so add a !file check to avoid attempting THP alignment for file mappings. > > Also, the changelog failed to describe the userspace-visible effects of > the flaw, which is basically essential when fixing bugs. > > The bug has been there since 6.10 so it would be interesting to learn > why it took this long to be noticed. Found issue by code inspection. THP alignment is used for easy or more pmd mappings, from vma side, I don't think it will introduce usespace- visible effects, only different vma address, but I don't know if there's any other effect.
On 12/9/24 06:00, Kefeng Wang wrote: > > > On 2024/12/7 14:34, Andrew Morton wrote: >> On Fri, 6 Dec 2024 15:03:45 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: >> >>> Commit ed48e87c7df3 ("thp: add thp_get_unmapped_area_vmflags()") >>> changes thp_get_unmapped_area() to thp_get_unmapped_area_vmflags() >>> in __get_unmapped_area(), which won't setup get_area for anonymous >>> mappings, but it leads to always try THP align when file ops without >>> '.get_unmapped_area' callback too as the get_area is NULL. >>> >>> Since commit efa7df3e3bb5 ("mm: align larger anonymous mappings on >>> THP boundaries") only want to enable THP align for anonymous, adding >>> !file check to fix it. >> >> The above is tough. I attempted a rewrite, please review for accuracy >> and completeness: > > Forgive my English, thanks for rewriting the better changelog. >> >> : Commit ed48e87c7df3 ("thp: add thp_get_unmapped_area_vmflags()") changes >> : thp_get_unmapped_area() to thp_get_unmapped_area_vmflags() in >> : __get_unmapped_area(), which doesn't initialize local get_area for >> : anonymous mappings. This leads to us always trying THP alignment even for >> : file_operations which have a NULL ->get_unmapped_area() callback. >> : >> : Since commit efa7df3e3bb5 ("mm: align larger anonymous mappings on THP >> : boundaries") we only want to enable THP alignment for anonymous mappings, >> : so add a !file check to avoid attempting THP alignment for file mappings. >> >> Also, the changelog failed to describe the userspace-visible effects of >> the flaw, which is basically essential when fixing bugs. >> >> The bug has been there since 6.10 so it would be interesting to learn >> why it took this long to be noticed. > > Found issue by code inspection. THP alignment is used for easy or more > pmd mappings, from vma side, I don't think it will introduce usespace- > visible effects, only different vma address, but I don't know if there's > any other effect. How about: This may cause unnecessary VMA fragmentation and potentially worse performance on filesystems that do not actually support THPs and thus cannot benefit from the alignment.
On 2024/12/9 16:36, Vlastimil Babka wrote: > On 12/9/24 06:00, Kefeng Wang wrote: >> >> >> On 2024/12/7 14:34, Andrew Morton wrote: >>> On Fri, 6 Dec 2024 15:03:45 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: >>> >>>> Commit ed48e87c7df3 ("thp: add thp_get_unmapped_area_vmflags()") >>>> changes thp_get_unmapped_area() to thp_get_unmapped_area_vmflags() >>>> in __get_unmapped_area(), which won't setup get_area for anonymous >>>> mappings, but it leads to always try THP align when file ops without >>>> '.get_unmapped_area' callback too as the get_area is NULL. >>>> >>>> Since commit efa7df3e3bb5 ("mm: align larger anonymous mappings on >>>> THP boundaries") only want to enable THP align for anonymous, adding >>>> !file check to fix it. >>> >>> The above is tough. I attempted a rewrite, please review for accuracy >>> and completeness: >> >> Forgive my English, thanks for rewriting the better changelog. >>> >>> : Commit ed48e87c7df3 ("thp: add thp_get_unmapped_area_vmflags()") changes >>> : thp_get_unmapped_area() to thp_get_unmapped_area_vmflags() in >>> : __get_unmapped_area(), which doesn't initialize local get_area for >>> : anonymous mappings. This leads to us always trying THP alignment even for >>> : file_operations which have a NULL ->get_unmapped_area() callback. >>> : >>> : Since commit efa7df3e3bb5 ("mm: align larger anonymous mappings on THP >>> : boundaries") we only want to enable THP alignment for anonymous mappings, >>> : so add a !file check to avoid attempting THP alignment for file mappings. >>> >>> Also, the changelog failed to describe the userspace-visible effects of >>> the flaw, which is basically essential when fixing bugs. >>> >>> The bug has been there since 6.10 so it would be interesting to learn >>> why it took this long to be noticed. >> >> Found issue by code inspection. THP alignment is used for easy or more >> pmd mappings, from vma side, I don't think it will introduce usespace- >> visible effects, only different vma address, but I don't know if there's >> any other effect. > > How about: > > This may cause unnecessary VMA fragmentation and potentially worse > performance on filesystems that do not actually support THPs and thus cannot > benefit from the alignment. Thanks for your update, yes, like efa7df3e3bb5, there is performance regression for align anonymous mapping before. Hi Andrew, please help to squash above part into the changelog, thanks.
diff --git a/mm/mmap.c b/mm/mmap.c index 1c6bdffa13dd..b373486bd1c6 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -782,7 +782,7 @@ __get_unmapped_area(struct file *file, unsigned long addr, unsigned long len, if (get_area) { addr = get_area(file, addr, len, pgoff, flags); - } else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) + } else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) && !file && !addr /* no hint */ && IS_ALIGNED(len, PMD_SIZE)) { /* Ensures that larger anonymous mappings are THP aligned. */
Commit ed48e87c7df3 ("thp: add thp_get_unmapped_area_vmflags()") changes thp_get_unmapped_area() to thp_get_unmapped_area_vmflags() in __get_unmapped_area(), which won't setup get_area for anonymous mappings, but it leads to always try THP align when file ops without '.get_unmapped_area' callback too as the get_area is NULL. Since commit efa7df3e3bb5 ("mm: align larger anonymous mappings on THP boundaries") only want to enable THP align for anonymous, adding !file check to fix it. Fixes: ed48e87c7df3 ("thp: add thp_get_unmapped_area_vmflags()") Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- mm/mmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)