On Fri, Nov 15, 2024 at 2:15 PM Yang Shi <shy828301@xxxxxxxxx> wrote:
>
> On Fri, Nov 15, 2024 at 1:52 PM Kalesh Singh <kaleshsingh@xxxxxxxxxx> wrote:
> >
> > Commit efa7df3e3bb5 ("mm: align larger anonymous mappings on THP
> > boundaries") updated __get_unmapped_area() to align the start address
> > for the VMA to a PMD boundary if CONFIG_TRANSPARENT_HUGEPAGE=y.
> >
> > It does this by effectively looking up a region that is of size,
> > request_size + PMD_SIZE, and aligning up the start to a PMD boundary.
> >
> > Commit 4ef9ad19e176 ("mm: huge_memory: don't force huge page alignment
> > on 32 bit") opted out of this for 32bit due to regressions in mmap base
> > randomization.
> >
> > Commit d4148aeab412 ("mm, mmap: limit THP alignment of anonymous
> > mappings to PMD-aligned sizes") restricted this to only mmap sizes that
> > are multiples of the PMD_SIZE due to reported regressions in some
> > performance benchmarks -- which seemed mostly due to the reduced spatial
> > locality of related mappings due to the forced PMD-alignment.
> >
> > Another unintended side effect has emerged: When a user specifies an mmap
> > hint address, the THP alignment logic modifies the behavior, potentially
> > ignoring the hint even if a sufficiently large gap exists at the requested
> > hint location.
> >
> > Example Scenario:
> >
> > Consider the following simplified virtual address (VA) space:
> >
> > ...
> >
> > 0x200000-0x400000 --- VMA A
> > 0x400000-0x600000 --- Hole
> > 0x600000-0x800000 --- VMA B
> >
> > ...
> >
> > A call to mmap() with hint=0x400000 and len=0x200000 behaves differently:
> >
> > - Before THP alignment: The requested region (size 0x200000) fits into
> > the gap at 0x400000, so the hint is respected.
> >
> > - After alignment: The logic searches for a region of size
> > 0x400000 (len + PMD_SIZE) starting at 0x400000.
> > This search fails due to the mapping at 0x600000 (VMA B), and the hint
> > is ignored, falling back to arch_get_unmapped_area[_topdown]().
> >
> > In general the hint is effectively ignored, if there is any
> > existing mapping in the below range:
> >
> > [mmap_hint + mmap_size, mmap_hint + mmap_size + PMD_SIZE)
> >
> > This changes the semantics of mmap hint; from ""Respect the hint if a
> > sufficiently large gap exists at the requested location" to "Respect the
> > hint only if an additional PMD-sized gap exists beyond the requested size".
> >
> > This has performance implications for allocators that allocate their heap
> > using mmap but try to keep it "as contiguous as possible" by using the
> > end of the exisiting heap as the address hint. With the new behavior
> > it's more likely to get a much less contiguous heap, adding extra
> > fragmentation and performance overhead.
> >
> > To restore the expected behavior; don't use thp_get_unmapped_area_vmflags()
> > when the user provided a hint address.
>
> Thanks for fixing it. I agree we should respect the hint address. But
> this patch actually just fixed anonymous mapping and the file mappings
> which don't support thp_get_unmapped_area(). So I think you should
> move the hint check to __thp_get_unmapped_area().
>
> And Vlastimil's fix d4148aeab412 ("mm, mmap: limit THP alignment of
> anonymous mappings to PMD-aligned sizes") should be moved to there too
> IMHO.
Thanks Yang, you are right, to cover the file systems that are using
this for their .get_unmapped_area(). I'll move this to where the 64
bit checks are done when posting v2.
Thanks,
Kalesh
>
> > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Vlastimil Babka <vbabka@xxxxxxx>
> > Cc: Yang Shi <yang@xxxxxxxxxxxxxxxxxxxxxx>
> > Cc: Rik van Riel <riel@xxxxxxxxxxx>
> > Cc: Ryan Roberts <ryan.roberts@xxxxxxx>
> > Cc: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> > Cc: Minchan Kim <minchan@xxxxxxxxxx>
> > Cc: Hans Boehm <hboehm@xxxxxxxxxx>
> > Cc: Lokesh Gidra <lokeshgidra@xxxxxxxxxx>
> > Cc: <stable@xxxxxxxxxxxxxxx>
> > Fixes: efa7df3e3bb5 ("mm: align larger anonymous mappings on THP boundaries")
> > Signed-off-by: Kalesh Singh <kaleshsingh@xxxxxxxxxx>
> > ---
> > mm/mmap.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 79d541f1502b..2f01f1a8e304 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -901,6 +901,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)
> > + && !addr /* no hint */
> > && IS_ALIGNED(len, PMD_SIZE)) {
> > /* Ensures that larger anonymous mappings are THP aligned. */
> > addr = thp_get_unmapped_area_vmflags(file, addr, len,
> >
> > base-commit: 2d5404caa8c7bb5c4e0435f94b28834ae5456623
> > --
> > 2.47.0.338.g60cca15819-goog
> >
