linux-kernel - Re: [PATCH mm-unstable 17/17] mm: Respect mmap hint before THP alignment if allocation is possible

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAHbLzkqbH_AR2jy_6LZ7KSh6bcf4L5B51Mq9DwYtdBcVz1Lu6w@mail.gmail.com>
Date: Tue, 10 Dec 2024 11:39:48 -0800
From: Yang Shi <shy828301@...il.com>
To: Kalesh Singh <kaleshsingh@...gle.com>
Cc: akpm@...ux-foundation.org, vbabka@...e.cz, yang@...amperecomputing.com, 
	riel@...riel.com, david@...hat.com, linux@...linux.org.uk, 
	tsbogend@...ha.franken.de, James.Bottomley@...senpartnership.com, 
	ysato@...rs.sourceforge.jp, dalias@...c.org, glaubitz@...sik.fu-berlin.de, 
	davem@...emloft.net, andreas@...sler.com, tglx@...utronix.de, bp@...en8.de, 
	dave.hansen@...ux.intel.com, x86@...nel.org, chris@...kel.net, 
	jcmvbkbc@...il.com, bhelgaas@...gle.com, jason.andryuk@....com, 
	leitao@...ian.org, linux-alpha@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-snps-arc@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org, 
	linux-csky@...r.kernel.org, loongarch@...ts.linux.dev, 
	linux-mips@...r.kernel.org, linux-parisc@...r.kernel.org, 
	linuxppc-dev@...ts.ozlabs.org, linux-s390@...r.kernel.org, 
	linux-sh@...r.kernel.org, sparclinux@...r.kernel.org, linux-mm@...ck.org, 
	kernel-team@...roid.com, android-mm@...gle.com
Subject: Re: [PATCH mm-unstable 17/17] mm: Respect mmap hint before THP
 alignment if allocation is possible

On Tue, Dec 10, 2024 at 9:34 AM Kalesh Singh <kaleshsingh@...gle.com> wrote:
>
> On Mon, Dec 9, 2024 at 7:37 PM Yang Shi <shy828301@...il.com> wrote:
> >
> > On Mon, Dec 9, 2024 at 6:45 PM Kalesh Singh <kaleshsingh@...gle.com> wrote:
> > >
> > > Commit 249608ee4713 ("mm: respect mmap hint address when aligning for THP")
> > > fallsback to PAGE_SIZE alignment instead of THP alignment
> > > for anonymous mapping as long as a hint address is provided by the user
> > > -- even if we weren't able to allocate the unmapped area at the hint
> > > address in the end.
> > >
> > > This was done to address the immediate regression in anonymous mappings
> > > where the hint address were being ignored in some cases; due to commit
> > > efa7df3e3bb5 ("mm: align larger anonymous mappings on THP boundaries").
> > >
> > > It was later pointed out that this issue also existed for file-backed
> > > mappings from file systems that use thp_get_unmapped_area() for their
> > > .get_unmapped_area() file operation.
> > >
> > > The same fix was not applied for file-backed mappings since it would
> > > mean any mmap requests that provide a hint address would be only
> > > PAGE_SIZE-aligned regardless of whether allocation was successful at
> > > the hint address or not.
> > >
> > > Instead, use arch_mmap_hint() to first attempt allocation at the hint
> > > address and fallback to THP alignment if that fails.
> >
> > Thanks for taking time to try to fix this.
> >
> > >
> > > Signed-off-by: Kalesh Singh <kaleshsingh@...gle.com>
> > > ---
> > >  mm/huge_memory.c | 15 ++++++++-------
> > >  mm/mmap.c        |  1 -
> > >  2 files changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > > index 137abeda8602..f070c89dafc9 100644
> > > --- a/mm/huge_memory.c
> > > +++ b/mm/huge_memory.c
> > > @@ -1097,6 +1097,14 @@ static unsigned long __thp_get_unmapped_area(struct file *filp,
> > >         loff_t off_align = round_up(off, size);
> > >         unsigned long len_pad, ret, off_sub;
> > >
> > > +       /*
> > > +        * If allocation at the address hint succeeds; respect the hint and
> > > +        * don't try to align to THP boundary.
> > > +        */
> > > +       addr = arch_mmap_hint(filp, addr, len, off, flags);
> > > +       if (addr)
> > > +               return addr;
> > > +
>
> Hi Yang,
>
> Thanks for the comments.
>
> >
> > IIUC, arch_mmap_hint() will be called in arch_get_unmapped_area() and
> > arch_get_unmapped_area_topdown() again. So we will actually look up
> > maple tree twice. It sounds like the second hint address search is
> > pointless. You should be able to set addr to 0 before calling
> > mm_get_unmapped_area_vmflags() in order to skip the second hint
> > address search.
>
> You are right that it would call into arch_mmap_hint() twice but it
> only attempts the lookup once since on the second attempt addr == 0.

Aha, yeah, I missed addr is going to be reset if arch_mmap_hint()
fails to find a suitable area.

>
> Thanks,
> Kalesh
> >
> > >         if (!IS_ENABLED(CONFIG_64BIT) || in_compat_syscall())
> > >                 return 0;
> > >
> > > @@ -1117,13 +1125,6 @@ static unsigned long __thp_get_unmapped_area(struct file *filp,
> > >         if (IS_ERR_VALUE(ret))
> > >                 return 0;
> > >
> > > -       /*
> > > -        * Do not try to align to THP boundary if allocation at the address
> > > -        * hint succeeds.
> > > -        */
> > > -       if (ret == addr)
> > > -               return addr;
> > > -
> > >         off_sub = (off - ret) & (size - 1);
> > >
> > >         if (test_bit(MMF_TOPDOWN, &current->mm->flags) && !off_sub)
> > > diff --git a/mm/mmap.c b/mm/mmap.c
> > > index 59bf7d127aa1..6bfeec80152a 100644
> > > --- a/mm/mmap.c
> > > +++ b/mm/mmap.c
> > > @@ -807,7 +807,6 @@ __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) && !file
> > > -                  && !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,
> > > --
> > > 2.47.0.338.g60cca15819-goog
> > >
> > >