Re: [PATCH 1/3] riscv: mm: Use hint address in mmap if available

[Charlie Jenkins]

On Tue, Jan 30, 2024 at 10:34:03AM +0800, Yangyu Chen wrote:
> 
> > On Jan 30, 2024, at 08:37, Charlie Jenkins <charlie@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> > arch/riscv/include/asm/processor.h | 21 ++++++++-------------
> > 1 file changed, 8 insertions(+), 13 deletions(-)
> > 
> > diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
> > index f19f861cda54..f3ea5166e3b2 100644
> > --- a/arch/riscv/include/asm/processor.h
> > +++ b/arch/riscv/include/asm/processor.h
> > @@ -22,14 +22,11 @@
> > ({ \
> > 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))) \
> 
> How about replacing BIT(VA_BITS-1) to DEFAULT_MAP_WINDOW to make the code
> more general.
> 
> > else \
> > - mmap_end = VA_USER_SV39; \
> > + mmap_end = (_addr + len); \
> > mmap_end; \
> > })
> > 
> > @@ -39,14 +36,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))) \
> 
> Same here.
> 
> > 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; \
> > })
> > 
> > 
> 
> What about not setting the upper bound as x86/arm/powerpc as [1] did?
> In this case, user space can directly pass a constant hint address >
> BIT(47) to get a mapping in sv57. If you want this, this code also allows
> user-space to pass any address larger than TASK_SIZE. You should also
> limit the mmap_base to (base) + TASK_SIZE - DEFAULT_MAP_WINDOW.

No. This suggestion causes a random address to be used if the hint
address is not available. That is why I didn't simply go with your
patch.

This patch both gives your application the benefit of being able to use
a hint address in the hopes that the address is available, as well as
continuing to support the guarantee that an address using more bits than
the hint address is not returned.

- Charlie

> 
> I’m also aware of the rnd_gap if it is not 0, then we will not get
> address mapped to VA_USER_SV39 - rnd_gap.
> 
> [1]. https://lore.kernel.org/linux-riscv/tencent_2683632BEE438C6D4854E30BDF9CA0843606@xxxxxx/
> 
> > -- 
> > 2.43.0
> > 
>