> On Aug 23, 2024, at 13:57, Charlie Jenkins <char...@rivosinc.com> wrote:
>
> On Fri, Aug 23, 2024 at 01:28:18PM +0800, Yangyu Chen wrote:
>>
>>
>>> On Aug 23, 2024, at 12:39, Charlie Jenkins <char...@rivosinc.com> wrote:
>>>
>>> On Thu, Aug 22, 2024 at 10:51:54AM +0800, Yangyu Chen wrote:
>>>>
>>>>
>>>>> On Aug 22, 2024, at 06:17, Palmer Dabbelt <pal...@dabbelt.com> wrote:
>>>>>
>>>>> On Mon, 19 Aug 2024 18:58:18 PDT (-0700), rsworkt...@outlook.com 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<char...@rivosinc.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.
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> 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 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".
>>>>>
>>>>> Ya, sorry for breaking stuff.
>>>>>
>>>>> The goal here was to move to the mmap flag behavor similar to what arm64
>>>>> and x86 have, as that was done in a way that didn't appear to break
>>>>> userspace -- or at least any real userspace programs. IIRC that first
>>>>> test was pretty broken (it actually depended on the hint address), but
>>>>> sounds like that's not the case.
>>>>>
>>>>> I think maybe this is just luck: we didn't chunk the address space up,
>>>>> we're just hinting on every bit, so we're just more likely to hit the
>>>>> exhaustion. Doesn't really matter, though, as if it's breaking stuff so
>>>>> we've got to deal with it.
>>>>>
>>>>> Charlie and I are just talking, and best we can come up with is to move
>>>>> to the behavior where we fall back to larger allocation regions when
>>>>> there's no space in the smaller allocation region.
>>>>
>>>>
>>>> For this solution, the only difference from the mmap behavior of
>>>> x86 and aarch64 is that we will first try to allocate some memory
>>>> from an address less or equal to the request address + size. But
>>>> for most cases, I think there is no need to do that, especially for
>>>> those addresses < BIT(47), as most program works fine on x86-64,
>>>> which has 47bit available userspace address space to use. And for
>>>> that program that wants an address < BIT(32), we already have
>>>> MAP_32BIT now.
>>>>
>>>> I think we can just fix like that patch:
>>>> https://lore.kernel.org/lkml/tencent_b2d0435bc011135736262764b511994f4...@qq.com/
>>>
>>> This patch does not satisfy the requirement of having the ability to
>>> guarantee
>>> that mmap returns an address that is less than the hint address.
>>
>> Indeed. My intuition is to remove it and align it with x86 and aarch64.
>>
>>> This
>>> patch only allows an address to be less than the DEFAULT_MAP_WINDOW
>>> which is 32 bits on sv32, 39 bits on sv39, and 48 bits on sv48 or sv57.
>>>
>>> This patch also again falls into the trap of using the hint address to
>>> forcefully restrict the address space.
>>
>> Indeed. However, x86 and aarch64 also use this behavior to restrict
>> va >= BIT(47) by default unless we have the hint address larger
>> than BIT(47).
>>
>>> I agree with Levi that it is not
>>> very good behavior to have a "hint" cause mmap to fail if conforming to
>>> the hint isn't possible. Instead, I believe it to be more logical to try
>>> to allocate at the hint address, otherwise give a random address.
>>>
>>
>> I also agree with this.
>>
>>> The current behavior can then be maintained through the flag
>>> MAP_BELOW_HINT. This way the user explicitly selects that they want mmap
>>> to fail if an address could not be found within the hint address
>>> constraints.
>>>
>>
>> I think restricting the addresses with the MAP_BELOW_HINT flag
>> would be the best choice. However, it remains a problem: What should
>> the behavior be when there is no MAP_BELOW_HINT? I think we can
>> fallback to Sv48 on the Sv57 machine by default to align with x86
>> and aarch64.
>
> Although that is the behavior on other architectures, I am hesitant to
> follow it because it is a somewhat arbitrary restriction. With a generic
> flag that can force mmap to provide exactly the number of bits that an
> application needs, there is no need for this restriction on riscv. It
> may cause problems for applications running on sv57 hardware, however:
>
> 1. sv57 hardware does not exist yet
>
Note that we have QEMU, which uses Sv57 by default. If the mmap
returns an address >= BIT(47) with hint address == NULL, many QEMU
users, such as some distro package builders, may need to deal with
some problems.
> 2. A hint address would still be required if following the same behavior
> as other architectures.
> a. It would aid in the porting of an application to sv57
> hardware, but I am not sure that forcing this restriction is
> worth having this one piece of parity. Applications using the
> proposed generic flag would work as expected on all
> architectures as well.
>
>>
>>> - Charlie
>>>
>>>>
>>>>> Charlie's going to try and throw together a patch for that, hopefully
>>>>> it'll sort things out.
>>>>>
>>>>>>> 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.
>>>>>>
>>>>>>> - Charlie
>>>>>>>
>>>>>>>> See alsohttps://github.com/riscv-forks/electron/issues/4
>>>>>>>>
>>>>>>>>>> - Charlie
>>>>>>>> Sincerely,
>>>>>>>> Levi
>>>>>>>>
>>>>>>
>>>>>> I accidentally introduced some HTML into this reply so this reply is
>>>>>> resent as plain text.
>>>>>>
>>>>>> Sincerely,
>>>>>> Levi
>>
>>
