Message-ID: <ZsQCtUP9ul2Y9WB9@ghost>
Date: Mon, 19 Aug 2024 19:43:01 -0700
From: Charlie Jenkins <charlie@...osinc.com>
To: Levi Zim <rsworktech@...look.com>
Cc: Palmer Dabbelt <palmer@...belt.com>, cyy@...self.name,
	alexghiti@...osinc.com, Paul Walmsley <paul.walmsley@...ive.com>,
	aou@...s.berkeley.edu, shuah@...nel.org, corbet@....net,
	linux-mm@...ck.org, linux-riscv@...ts.infradead.org,
	linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-api@...r.kernel.org
Subject: Re: [PATCH v3 1/3] riscv: mm: Use hint address in mmap if available

On Tue, Aug 20, 2024 at 09:48:50AM +0800, Levi Zim wrote:
> On 2024-08-20 01:00, Charlie Jenkins wrote:
> > On Mon, Aug 19, 2024 at 01:55:57PM +0800, Levi Zim wrote:
> > > On 2024-03-22 22:06, Palmer Dabbelt wrote:
> > > > On Thu, 01 Feb 2024 18:28:06 PST (-0800), Charlie Jenkins wrote:
> > > > > On Wed, Jan 31, 2024 at 11:59:43PM +0800, Yangyu Chen wrote:
> > > > > > On Wed, 2024-01-31 at 22:41 +0800, Yangyu Chen wrote:
> > > > > > > On Tue, 2024-01-30 at 17:07 -0800, Charlie Jenkins wrote:
> > > > > > > > On riscv it is guaranteed that the address returned by mmap is less
> > > > > > > > than
> > > > > > > > the hint address. Allow mmap to return an address all the way up to
> > > > > > > > addr, if provided, rather than just up to the lower address space.
> > > > > > > > > > This provides a performance benefit as well, allowing
> > > > > > mmap to exit
> > > > > > > > after
> > > > > > > > checking that the address is in range rather than searching for a
> > > > > > > > valid
> > > > > > > > address.
> > > > > > > > > > It is possible to provide an address that uses at most the same
> > > > > > > > number
> > > > > > > > of bits, however it is significantly more computationally expensive
> > > > > > > > to
> > > > > > > > provide that number rather than setting the max to be the hint
> > > > > > > > address.
> > > > > > > > There is the instruction clz/clzw in Zbb that returns the highest
> > > > > > > > set
> > > > > > > > bit
> > > > > > > > which could be used to performantly implement this, but it would
> > > > > > > > still
> > > > > > > > be slower than the current implementation. At worst case, half of
> > > > > > > > the
> > > > > > > > address would not be able to be allocated when a hint address is
> > > > > > > > provided.
> > > > > > > > > > Signed-off-by: Charlie Jenkins<charlie@...osinc.com>
> > > > > > > > ---
> > > > > > > >   arch/riscv/include/asm/processor.h | 27 +++++++++++---------------
> > > > > > > > -
> > > > > > > >   1 file changed, 11 insertions(+), 16 deletions(-)
> > > > > > > > > > diff --git a/arch/riscv/include/asm/processor.h
> > > > > > > > b/arch/riscv/include/asm/processor.h
> > > > > > > > index f19f861cda54..8ece7a8f0e18 100644
> > > > > > > > --- a/arch/riscv/include/asm/processor.h
> > > > > > > > +++ b/arch/riscv/include/asm/processor.h
> > > > > > > > @@ -14,22 +14,16 @@
> > > > > > > > 
> > > > > > > >   #include <asm/ptrace.h>
> > > > > > > > 
> > > > > > > > -#ifdef CONFIG_64BIT
> > > > > > > > -#define DEFAULT_MAP_WINDOW    (UL(1) << (MMAP_VA_BITS - 1))
> > > > > > > > -#define STACK_TOP_MAX        TASK_SIZE_64
> > > > > > > > -
> > > > > > > >   #define arch_get_mmap_end(addr, len, flags)            \
> > > > > > > >   ({                                \
> > > > > > > >       unsigned long
> > > > > > > > mmap_end;                    \
> > > > > > > >       typeof(addr) _addr = (addr);                \
> > > > > > > > -    if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) &&
> > > > > > > > is_compat_task())) \
> > > > > > > > +    if ((_addr) == 0 ||                    \
> > > > > > > > +        (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) ||    \
> > > > > > > > +        ((_addr + len) > BIT(VA_BITS -
> > > > > > > > 1)))            \
> > > > > > > >           mmap_end = STACK_TOP_MAX;            \
> > > > > > > > -    else if ((_addr) >= VA_USER_SV57) \
> > > > > > > > -        mmap_end = STACK_TOP_MAX;            \
> > > > > > > > -    else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >=
> > > > > > > > VA_BITS_SV48)) \
> > > > > > > > -        mmap_end = VA_USER_SV48;            \
> > > > > > > >       else                            \
> > > > > > > > -        mmap_end = VA_USER_SV39;            \
> > > > > > > > +        mmap_end = (_addr + len);            \
> > > > > > > >       mmap_end;                        \
> > > > > > > >   })
> > > > > > > > 
> > > > > > > > @@ -39,17 +33,18 @@
> > > > > > > >       typeof(addr) _addr = (addr);                \
> > > > > > > >       typeof(base) _base = (base);                \
> > > > > > > >       unsigned long rnd_gap = DEFAULT_MAP_WINDOW - (_base);    \
> > > > > > > > -    if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) &&
> > > > > > > > is_compat_task())) \
> > > > > > > > +    if ((_addr) == 0 ||                    \
> > > > > > > > +        (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) ||    \
> > > > > > > > +        ((_addr + len) > BIT(VA_BITS -
> > > > > > > > 1)))            \
> > > > > > > >           mmap_base = (_base);                \
> > > > > > > > -    else if (((_addr) >= VA_USER_SV57) && (VA_BITS >=
> > > > > > > > VA_BITS_SV57)) \
> > > > > > > > -        mmap_base = VA_USER_SV57 - rnd_gap; \
> > > > > > > > -    else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >=
> > > > > > > > VA_BITS_SV48)) \
> > > > > > > > -        mmap_base = VA_USER_SV48 - rnd_gap; \
> > > > > > > >       else                            \
> > > > > > > > -        mmap_base = VA_USER_SV39 - rnd_gap; \
> > > > > > > > +        mmap_base = (_addr + len) - rnd_gap; \
> > > > > > > >       mmap_base;                        \
> > > > > > > >   })
> > > > > > > > 
> > > > > > > > +#ifdef CONFIG_64BIT
> > > > > > > > +#define DEFAULT_MAP_WINDOW    (UL(1) << (MMAP_VA_BITS - 1))
> > > > > > > > +#define STACK_TOP_MAX        TASK_SIZE_64
> > > > > > > >   #else
> > > > > > > >   #define DEFAULT_MAP_WINDOW    TASK_SIZE
> > > > > > > >   #define STACK_TOP_MAX        TASK_SIZE
> > > > > > > > > > I have carefully tested your patch on qemu with sv57. A
> > > > > > bug that
> > > > > > > needs
> > > > > > > to be solved is that mmap with the same hint address without
> > > > > > > MAP_FIXED
> > > > > > > set will fail the second time.
> > > > > > > > Userspace code to reproduce the bug:
> > > > > > > > #include <sys/mman.h>
> > > > > > > #include <stdio.h>
> > > > > > > #include <stdint.h>
> > > > > > > > void test(char *addr) {
> > > > > > >      char *res = mmap(addr, 4096, PROT_READ | PROT_WRITE,
> > > > > > > MAP_ANONYMOUS
> > > > > > > > MAP_PRIVATE, -1, 0);
> > > > > > >      printf("hint %p got %p.\n", addr, res);
> > > > > > > }
> > > > > > > > int main (void) {
> > > > > > >      test(1<<30);
> > > > > > >      test(1<<30);
> > > > > > >      test(1<<30);
> > > > > > >      return 0;
> > > > > > > }
> > > > > > > > output:
> > > > > > > > hint 0x40000000 got 0x40000000.
> > > > > > > hint 0x40000000 got 0xffffffffffffffff.
> > > > > > > hint 0x40000000 got 0xffffffffffffffff.
> > > > > > > > output on x86:
> > > > > > > > hint 0x40000000 got 0x40000000.
> > > > > > > hint 0x40000000 got 0x7f9171363000.
> > > > > > > hint 0x40000000 got 0x7f9171362000.
> > > > > > > > It may need to implement a special arch_get_unmapped_area and
> > > > > > > arch_get_unmapped_area_topdown function.
> > > > > > > 
> > > > > > This is because hint address < rnd_gap. I have tried to let mmap_base =
> > > > > > min((_addr + len), (base) + TASK_SIZE - DEFAULT_MAP_WINDOW). However it
> > > > > > does not work for bottom-up while ulimit -s is unlimited. You said this
> > > > > > behavior is expected from patch v2 review. However it brings a new
> > > > > > regression even on sv39 systems.
> > > > > > 
> > > > > > I still don't know the reason why use addr+len as the upper-bound. I
> > > > > > think solution like x86/arm64/powerpc provide two address space switch
> > > > > > based on whether hint address above the default map window is enough.
> > > > > > 
> > > > > Yep this is expected. It is up to the maintainers to decide.
> > > > Sorry I forgot to reply to this, I had a buffer sitting around somewhere
> > > > but I must have lost it.
> > > > 
> > > > I think Charlie's approach is the right way to go.  Putting my userspace
> > > > hat on, I'd much rather have my allocations fail rather than silently
> > > > ignore the hint when there's memory pressure.
> > > > 
> > > > If there's some real use case that needs these low hints to be silently
> > > > ignored under VA pressure then we can try and figure something out that
> > > > makes those applications work.
> > > I could confirm that this patch has broken chromium's partition allocator on
> > > riscv64. The minimal reproduction I use is chromium-mmap.c:
> > > 
> > > #include <stdio.h>
> > > #include <sys/mman.h>
> > > 
> > > int main() {
> > >      void* expected = (void*)0x400000000;
> > >      void* addr = mmap(expected, 17179869184, PROT_NONE,
> > > MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
> > >      if (addr != expected) {
> > It is not valid to assume that the address returned by mmap will be the
> > hint address. If the hint address is not available, mmap will return a
> > different address.
> 
> Oh, sorry I didn't make it clear what is the expected behavior.
> The printf here is solely for debugging purpose and I don't mean that
> chromium expect it will get the hint address. The expected behavior is that
> both the two mmap calls will succeed.
> 
> > >          printf("Not expected address: %p != %p\n", addr, expected);
> > >      }
> > >      expected = (void*)0x3fffff000;
> > >      addr = mmap(expected, 17179873280, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS,
> > > -1, 0);
> > >      if (addr != expected) {
> > >          printf("Not expected address: %p != %p\n", addr, expected);
> > >      }
> > >      return 0;
> > > }
> > > 
> > > The second mmap fails with ENOMEM. Manually reverting this commit fixes the
> > > issue for me. So I think it's clearly a regression and breaks userspace.
> > > 
> > The issue here is that overlapping memory is being requested. This
> > second mmap will never be able to provide an address at 0x3fffff000 with
> > a size of 0x400001000 since mmap just provided an address at 0x400000000
> > with a size of 0x400000000.
> > 
> > Before this patch, this request causes mmap to return a completely
> > arbitrary value. There is no reason to use a hint address in this manner
> > because the hint can never be respected. Since an arbitrary address is
> > desired, a hint of zero should be used.
> > 
> > This patch causes the behavior to be more deterministic. Instead of
> > providing an arbitrary address, it causes the address to be less than or
> > equal to the hint address. This allows for applications to make
> > assumptions about the returned address.
> 
> About the overlap, of course the partition allocator's request for
> overlapped vma seems unreasonable.
> 
> But I still don't quite understand why mmap cannot use an address higher
> than the hint address.
> The hint address, after all, is a hint, not a requirement.

Yes that is fair. A "hint" that does not guarantee anything is
useless so architectures have abused the term quite a bit.

> 
> Quoting the man page:
> 
> >    If another mapping already exists there, the kernel picks
> >         a new address that may or may not depend on the hint.  The
> >         address of the new mapping is returned as the result of the call.
> So for casual programmers that only reads man page but not architecture
> specific kernel
> documentation, the current behavior of mmap on riscv64 failing on overlapped
> address ranges
> are quite surprising IMO.

The man pages for riscv are in desperate need of attention. I have
submitted a couple of updates to them recently, but there is a lot more
work to be done to help developers.

> 
> And quoting the man page again about the errno:
> 
> >       ENOMEM No memory is available.
> > 
> >       ENOMEM The process's maximum number of mappings would have been
> >              exceeded.  This error can also occur for munmap(), when
> >              unmapping a region in the middle of an existing mapping,
> >              since this results in two smaller mappings on either side
> >              of the region being unmapped.
> > 
> >       ENOMEM (since Linux 4.7) The process's RLIMIT_DATA limit,
> >              described in getrlimit(2), would have been exceeded.
> > 
> >       ENOMEM We don't like addr, because it exceeds the virtual address
> >              space of the CPU.
> > 
> 
> There's no matching description for the ENOMEM returned here.
> I would suggest removing "because it exceeds the virtual address
> space of the CPU." from the last item if the ENOMEM behavior here
> is expected.

This ENOMEM means something like "no memory available in the requested
region".

> 
> > This code is unfortunately relying on the previously mostly undefined
> > behavior of the hint address in mmap.
> Although I haven't read the code of chromium's partition allocator to judge
> whether it should
> be improved or fixed for riscv64, I do know that the kernel "don't break
> userspace" and
> "never EVER blame the user programs".

The hint address design of mmap is a tricky one because it is largely
implementation defined and what the man pages say is not how it is
implemented in most architectures!

> > The goal of this patch is to help
> > developers have more consistent mmap behavior, but maybe it is necessary
> > to hide this behavior behind an mmap flag.
> Thank you for helping to shape a more consistent mmap behavior.
> I think this should be fixed ASAP either by allowing the hint address to be
> ignored
> (as suggested by the Linux man page), or hide this behavior behind an mmap
> flag as you said.

Having a flag could also lead to a generic way of defining this
behavior. Other architectures do not provide a way for applications to
guarantee that some number of bits are left unused in a virtual address,
and that was one of the motivating design goals here.

- Charlie

> 
> > - Charlie
> > 
> > > See alsohttps://github.com/riscv-forks/electron/issues/4
> > > 
> > > > > - Charlie
> > > Sincerely,
> > > Levi
> > > 

Powered by blists - more mailing lists