On Thu, 29 Feb 2024 04:10:03 PST (-0800), [email protected] wrote:
> This patch has not been reviewed for more than a month. There is another patch that did the same fix but in another way and still has not been reviewed like this. I'm here to do a comparison of some choices briefly to let the maintainer understand the issues and the solutions. I think it's time to make a decision before the next Linux LTS v6.9. As a number of sv48 chips will be released this year.
>
> Issues:
>
> Since commit add2cc6b6515 ("RISC-V: mm: Restrict address space for sv39,sv48,sv57") from patch [1], userspace software cannot create virtual address memory mapping on the hint address if the address larger than (1<<38) on sv48, sv57 capable CPU using mmap without MAP_FIXED set.
>
> This is because since that commit, the hint address is treated as the upper bound to create the mapping when the hint address is larger than (1<<38).
>
> Existing regression for userspace software since that commit:
> - box64 [2]
Is this the same regression as before? IIUC the real issue there is
that userspace wasn't passing MAP_FIXED and expecting a fixed address to
be mapped. That's just a bug in userspace.
Is there any software that uses mmap() in a legal way that the flags
patch caused a regression in? If that's the case then we'll need to
figure out what it's doing so we can avoid the regression.
The only thing I can think of are realloc-type schemes, where rounding
the hint address down would result in performance problems. I don't
know of anything like that specifically, but I think Charlie's patch
would fix it.
> Some choices are:
>
> 1. Do not change it
>
> Con:
>
> This behavior is not the same as x86, arm64, and powerpc when treating memory address space larger than 48-bit. On x86, arm64, and powerpc, if the hint address is larger than 48-bit, mmap will not limit the upper bound to use.
>
> Also, these ISAs limit the mmap to 48-bit by default. However, RISC-V currently uses sv39 by default, which is not the same as the document and commit message.
IIUC arm64/amd64 started with 48-bit-capable hardware and kernels, and
thus the only regression was when moving to the larger VA spaces. We
started with sv39-based VA space,
> 2. Use my patch
>
> which limits the upper bound of mmap to 47-bit by default, if the hint address is larger than (1<<47), then no limit.
>
> Pros: Let the behavior of mmap align with x86, arm64, powerpc
>
> Cons: A new regression for software that assumes mmap will not return an address larger than the hint address if the hint address is larger than (1<<38) as it has been documented on RISC-V since v6.6. However, there is no change in the widespread sv39 systems we use now.
The OpenJDK and Go people have at least talked about using the interface
as it is currently defined. I'm trying to chase down some of the folks
around here who understand that stuff, but it might take a bit...
> 3. Use Charlie's patch [3]
>
> which adjusts the upper bound to hint address + size.
IMO we can call that compatible with the docs. There's sort of a grey
area in "A hint address passed to mmap will cause the largest
address space that fits entirely into the hint to be used" as to how
that hint address is used, but I think interpreting it as the base
address is sane and we can just update the docs.
This also should fix the realloc-type cases I can think of, though those
are sort of theoretical right now.
> Pros: Still has upper-bound limit using hint address but allows userspace to create mapping on the hint address without MAP_FIXED set.
>
> Cons: That patch will introduce a new regression even for the sv39 system when creating mmap with the same hint address more than one time if the hint address is less than round-gap.
I'm not quite sure what you're trying to say there. If users are
passing a hint that's already allocated then they're not going to get
that address allocated, so as long as we give them something else we're
OK.
We might want to take more advantage of the clause in the docs that
allows larger addresses to be allocated under memory pressure to avoid
too many allocation failures, but that applies to any of these schemes.
> 4. Some new ideas currently are not on the mailing list
>
> Hope this issue can be fixed before the Linux v6.9 release.
>
> Thanks,
> Yangyu Chen
>
> [1] https://lore.kernel.org/linux-riscv/[email protected]/
> [2] https://github.com/ptitSeb/box64/commit/5b700cb6e6f397d2074c49659f7f9915f4a33c5f
> [3] https://lore.kernel.org/linux-riscv/[email protected]/
On 2024/3/1 03:21, Palmer Dabbelt wrote:
> On Thu, 29 Feb 2024 04:10:03 PST (-0800), [email protected] wrote:
>> This patch has not been reviewed for more than a month. There is
>> another patch that did the same fix but in another way and still has
>> not been reviewed like this. I'm here to do a comparison of some
>> choices briefly to let the maintainer understand the issues and the
>> solutions. I think it's time to make a decision before the next Linux
>> LTS v6.9. As a number of sv48 chips will be released this year.
>>
>> Issues:
>>
>> Since commit add2cc6b6515 ("RISC-V: mm: Restrict address space for
>> sv39,sv48,sv57") from patch [1], userspace software cannot create
>> virtual address memory mapping on the hint address if the address
>> larger than (1<<38) on sv48, sv57 capable CPU using mmap without
>> MAP_FIXED set.
>>
>> This is because since that commit, the hint address is treated as the
>> upper bound to create the mapping when the hint address is larger than
>> (1<<38).
>>
>> Existing regression for userspace software since that commit:
>> - box64 [2]
>
> Is this the same regression as before? IIUC the real issue there is
> that userspace wasn't passing MAP_FIXED and expecting a fixed address to
> be mapped. That's just a bug in userspace.
>
> Is there any software that uses mmap() in a legal way that the flags
> patch caused a regression in? If that's the case then we'll need to
> figure out what it's doing so we can avoid the regression.
>
> The only thing I can think of are realloc-type schemes, where rounding
> the hint address down would result in performance problems. I don't
> know of anything like that specifically, but I think Charlie's patch
> would fix it.
>
Yes. The regression for a legal mmap is only on performance for
userspace software, not on functionality.
>> Some choices are:
>>
>> 1. Do not change it
>>
>> Con:
>>
>> This behavior is not the same as x86, arm64, and powerpc when treating
>> memory address space larger than 48-bit. On x86, arm64, and powerpc,
>> if the hint address is larger than 48-bit, mmap will not limit the
>> upper bound to use.
>>
>> Also, these ISAs limit the mmap to 48-bit by default. However, RISC-V
>> currently uses sv39 by default, which is not the same as the document
>> and commit message.
>
> IIUC arm64/amd64 started with 48-bit-capable hardware and kernels, and
> thus the only regression was when moving to the larger VA spaces. We
> started with sv39-based VA space,
It's about the document and the commit message says it uses sv48 by
default. However, the code in the kernel uses sv39 by default. The
reasons for using sv48 by default has been talked about in that patch
review previously. [4]
Whatever, the document or the code can be simply fixed if we decide not
to change it.
Another concern is that if we can't make this decision in time to catch
up with v6.9 we don't want some bad things to happen as a large number
of sv48 machines might appear this year and they may run on the next
v6.9 LTS kernel, Shall we change the code in the kernel to use sv48 by
default right now?
[4] https://lore.kernel.org/linux-riscv/ZJzgi8RyqG3Mjt0R@ghost/
>> 2. Use my patch
>>
>> which limits the upper bound of mmap to 47-bit by default, if the hint
>> address is larger than (1<<47), then no limit.
>>
>> Pros: Let the behavior of mmap align with x86, arm64, powerpc
>>
>> Cons: A new regression for software that assumes mmap will not return
>> an address larger than the hint address if the hint address is larger
>> than (1<<38) as it has been documented on RISC-V since v6.6. However,
>> there is no change in the widespread sv39 systems we use now.
>
> The OpenJDK and Go people have at least talked about using the interface
> as it is currently defined. I'm trying to chase down some of the folks
> around here who understand that stuff, but it might take a bit...
>
Roger that.
>> 3. Use Charlie's patch [3]
>>
>> which adjusts the upper bound to hint address + size.
>
> IMO we can call that compatible with the docs. There's sort of a grey
> area in "A hint address passed to mmap will cause the largest
> address space that fits entirely into the hint to be used" as to how
> that hint address is used, but I think interpreting it as the base
> address is sane and we can just update the docs.
>
> This also should fix the realloc-type cases I can think of, though those
> are sort of theoretical right now.
>
>> Pros: Still has upper-bound limit using hint address but allows
>> userspace to create mapping on the hint address without MAP_FIXED set.
>>
>> Cons: That patch will introduce a new regression even for the sv39
>> system when creating mmap with the same hint address more than one
>> time if the hint address is less than round-gap.
>
> I'm not quite sure what you're trying to say there. If users are
> passing a hint that's already allocated then they're not going to get
> that address allocated, so as long as we give them something else we're OK.
>
In this case, mmap will return MAP_FAILED in the second time. But on
arm64, x86, it will pick an address in 48-bit space to use. However,
after reviewing the code, I think it's not easy to make Charlie's patch
search for another space to create the mapping without any changes
outside of arch/riscv.
> We might want to take more advantage of the clause in the docs that
> allows larger addresses to be allocated under memory pressure to avoid
> too many allocation failures, but that applies to any of these schemes.
>
Indeed. After thinking about it for a while, especially for the OpenJDK
and Go people have at least talked about using the interface. If it is
not used now, I have an idea is that to port Charlie's patch to Linux-mm
not only for RISC-V, and pick a flag like MAP_UPPERBOUND to use it. And
then change the mmap behavior on RISC-V to align with x86, arm64, and
powerpc. So we have all ISAs take advantage to use Charlie's idea and
all ISAs will treat mmap in the same way, which makes userspace
developers happy as they don't need to care about the ISA-specific behavior.
>> 4. Some new ideas currently are not on the mailing list
>>
>> Hope this issue can be fixed before the Linux v6.9 release.
>>
>> Thanks,
>> Yangyu Chen
>>
>> [1]
>> https://lore.kernel.org/linux-riscv/[email protected]/
>> [2]
>> https://github.com/ptitSeb/box64/commit/5b700cb6e6f397d2074c49659f7f9915f4a33c5f
>> [3]
>> https://lore.kernel.org/linux-riscv/[email protected]/
On Fri, Mar 01, 2024 at 04:54:05AM +0800, Yangyu Chen wrote:
> Another concern is that if we can't make this decision in time to catch up
> with v6.9 we don't want some bad things to happen as a large number of sv48
> machines might appear this year and they may run on the next v6.9 LTS
> kernel, Shall we change the code in the kernel to use sv48 by default right
> now?
Just pointing out that v6.9 is highly unlikely to be the next lts
kernel, depending on whether or not Linus delays some releases, it'll
most likely be either v6.11 or v6.12.