| Message ID | 20240130-use_mmap_hint_address-v2-1-f34ebfd33053@rivosinc.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | riscv: mm: Use hint address in mmap if available | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/vmtest-fixes-PR | fail | merge-conflict |
On Tue, 2024-01-30 at 11:04 -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@rivosinc.com> > --- > arch/riscv/include/asm/processor.h | 22 +++++++++------------- > 1 file changed, 9 insertions(+), 13 deletions(-) > > diff --git a/arch/riscv/include/asm/processor.h > b/arch/riscv/include/asm/processor.h > index f19f861cda54..5d966ae81a58 100644 > --- a/arch/riscv/include/asm/processor.h > +++ b/arch/riscv/include/asm/processor.h > @@ -22,14 +22,12 @@ > ({ \ > unsigned long > mmap_end; \ > typeof(addr) _addr = (addr); \ > - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && > is_compat_task())) \ > - 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; \ > + if ((_addr) == 0 || \ > + (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) || \ > + ((_addr + len) > BIT(VA_BITS - > 1))) \ > + mmap_end = STACK_TOP_MAX \ > else \ > - mmap_end = VA_USER_SV39; \ > + mmap_end = (_addr + len); \ > mmap_end; \ > }) > > @@ -39,14 +37,12 @@ > 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; \ Please mind that rnd_gap can be non-zero, in this case, the map will fail. It will be better to let mmap_base = min((_addr + len), (base) + TASK_SIZE - DEFAULT_MAP_WINDOW) . > mmap_base; \ > }) > >
On Wed, Jan 31, 2024 at 03:15:16AM +0800, Yangyu Chen wrote: > On Tue, 2024-01-30 at 11:04 -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@rivosinc.com> > > --- > > arch/riscv/include/asm/processor.h | 22 +++++++++------------- > > 1 file changed, 9 insertions(+), 13 deletions(-) > > > > diff --git a/arch/riscv/include/asm/processor.h > > b/arch/riscv/include/asm/processor.h > > index f19f861cda54..5d966ae81a58 100644 > > --- a/arch/riscv/include/asm/processor.h > > +++ b/arch/riscv/include/asm/processor.h > > @@ -22,14 +22,12 @@ > > ({ \ > > unsigned long > > mmap_end; \ > > typeof(addr) _addr = (addr); \ > > - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && > > is_compat_task())) \ > > - 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; \ > > + if ((_addr) == 0 || \ > > + (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) || \ > > + ((_addr + len) > BIT(VA_BITS - > > 1))) \ > > + mmap_end = STACK_TOP_MAX \ > > else \ > > - mmap_end = VA_USER_SV39; \ > > + mmap_end = (_addr + len); \ > > mmap_end; \ > > }) > > > > @@ -39,14 +37,12 @@ > > 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; \ > > Please mind that rnd_gap can be non-zero, in this case, the map will > fail. It will be better to let mmap_base = min((_addr + len), (base) + > TASK_SIZE - DEFAULT_MAP_WINDOW) . Why would an rnd_gap that is non-zero cause mmap to fail? mmap will fail if rnd_gap is greater than (_addr + len). That is expected and should be interpreted as no address was available in the requested address space. mmap_base is used if the hint address is not available. I do see that I fumbled the test cases in this patch so I will fix that in a v3. - Charlie > > > mmap_base; \ > > }) > > > > >
diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h index f19f861cda54..5d966ae81a58 100644 --- a/arch/riscv/include/asm/processor.h +++ b/arch/riscv/include/asm/processor.h @@ -22,14 +22,12 @@ ({ \ unsigned long mmap_end; \ typeof(addr) _addr = (addr); \ - if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \ - 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; \ + if ((_addr) == 0 || \ + (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) || \ + ((_addr + len) > BIT(VA_BITS - 1))) \ + mmap_end = STACK_TOP_MAX \ else \ - mmap_end = VA_USER_SV39; \ + mmap_end = (_addr + len); \ mmap_end; \ }) @@ -39,14 +37,12 @@ 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; \ })
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@rivosinc.com> --- arch/riscv/include/asm/processor.h | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-)