| Message ID | 20240129-use_mmap_hint_address-v1-1-4c74da813ba1@rivosinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | riscv: mm: Use hint address in mmap if available | expand |
On Mon, Jan 29, 2024, at 7:37 PM, 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 | 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; \ Setting mmap_end in the no-hint case seems to have been lost? -s > - 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))) \ > 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))) \ > 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; \ > }) > > > -- > 2.43.0 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Mon, Jan 29, 2024 at 08:53:31PM -0500, Stefan O'Rear wrote: > On Mon, Jan 29, 2024, at 7:37 PM, 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 | 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; \ > > Setting mmap_end in the no-hint case seems to have been lost? > > -s Thanks for catching that, will fix in v2. - Charlie > > > - 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))) \ > > 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))) \ > > 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; \ > > }) > > > > > > -- > > 2.43.0 > > > > > > _______________________________________________ > > linux-riscv mailing list > > linux-riscv@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> On Jan 30, 2024, at 08:37, Charlie Jenkins <charlie@rivosinc.com> 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 | 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. 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@qq.com/ > -- > 2.43.0 >
On Tue, Jan 30, 2024 at 10:34:03AM +0800, Yangyu Chen wrote: > > > On Jan 30, 2024, at 08:37, Charlie Jenkins <charlie@rivosinc.com> 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 | 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@qq.com/ > > > -- > > 2.43.0 > > >
On Mon, 2024-01-29 at 18:50 -0800, Charlie Jenkins wrote: > On Tue, Jan 30, 2024 at 10:34:03AM +0800, Yangyu Chen wrote: > > > > > On Jan 30, 2024, at 08:37, Charlie Jenkins <charlie@rivosinc.com> > > > 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 | 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. I think return random address is expected and other ISAs like x86/arm/powerpc will also return random address if hint is NULL. Also add CC linux-mm to get more opinions from people who familiar with mm. > > 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@qq.com/ > > > > > -- > > > 2.43.0 > > > > >
Hi Charlie,
kernel test robot noticed the following build errors:
[auto build test ERROR on 556e2d17cae620d549c5474b1ece053430cd50bc]
url: https://github.com/intel-lab-lkp/linux/commits/Charlie-Jenkins/riscv-mm-Use-hint-address-in-mmap-if-available/20240130-084208
base: 556e2d17cae620d549c5474b1ece053430cd50bc
patch link: https://lore.kernel.org/r/20240129-use_mmap_hint_address-v1-1-4c74da813ba1%40rivosinc.com
patch subject: [PATCH 1/3] riscv: mm: Use hint address in mmap if available
config: riscv-defconfig (https://download.01.org/0day-ci/archive/20240131/202401310404.eNJvHoC9-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240131/202401310404.eNJvHoC9-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401310404.eNJvHoC9-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from arch/riscv/include/asm/irqflags.h:10,
from include/linux/irqflags.h:18,
from arch/riscv/include/asm/bitops.h:14,
from include/linux/bitops.h:68,
from include/linux/kernel.h:23,
from mm/mmap.c:12:
mm/mmap.c: In function 'generic_get_unmapped_area':
>> arch/riscv/include/asm/processor.h:28:9: error: expected expression before 'else'
28 | else \
| ^~~~
mm/mmap.c:1703:40: note: in expansion of macro 'arch_get_mmap_end'
1703 | const unsigned long mmap_end = arch_get_mmap_end(addr, len, flags);
| ^~~~~~~~~~~~~~~~~
mm/mmap.c: In function 'generic_get_unmapped_area_topdown':
>> arch/riscv/include/asm/processor.h:28:9: error: expected expression before 'else'
28 | else \
| ^~~~
mm/mmap.c:1751:40: note: in expansion of macro 'arch_get_mmap_end'
1751 | const unsigned long mmap_end = arch_get_mmap_end(addr, len, flags);
| ^~~~~~~~~~~~~~~~~
--
In file included from arch/riscv/include/asm/irqflags.h:10,
from include/linux/irqflags.h:18,
from arch/riscv/include/asm/bitops.h:14,
from include/linux/bitops.h:68,
from include/linux/thread_info.h:27,
from fs/hugetlbfs/inode.c:12:
fs/hugetlbfs/inode.c: In function 'hugetlb_get_unmapped_area_bottomup':
>> arch/riscv/include/asm/processor.h:28:9: error: expected expression before 'else'
28 | else \
| ^~~~
fs/hugetlbfs/inode.c:173:27: note: in expansion of macro 'arch_get_mmap_end'
173 | info.high_limit = arch_get_mmap_end(addr, len, flags);
| ^~~~~~~~~~~~~~~~~
fs/hugetlbfs/inode.c: In function 'hugetlb_get_unmapped_area_topdown':
>> arch/riscv/include/asm/processor.h:28:9: error: expected expression before 'else'
28 | else \
| ^~~~
fs/hugetlbfs/inode.c:204:35: note: in expansion of macro 'arch_get_mmap_end'
204 | info.high_limit = arch_get_mmap_end(addr, len, flags);
| ^~~~~~~~~~~~~~~~~
fs/hugetlbfs/inode.c: In function 'generic_hugetlb_get_unmapped_area':
>> arch/riscv/include/asm/processor.h:28:9: error: expected expression before 'else'
28 | else \
| ^~~~
fs/hugetlbfs/inode.c:219:40: note: in expansion of macro 'arch_get_mmap_end'
219 | const unsigned long mmap_end = arch_get_mmap_end(addr, len, flags);
| ^~~~~~~~~~~~~~~~~
vim +/else +28 arch/riscv/include/asm/processor.h
add2cc6b6515f7 Charlie Jenkins 2023-08-09 20
add2cc6b6515f7 Charlie Jenkins 2023-08-09 21 #define arch_get_mmap_end(addr, len, flags) \
add2cc6b6515f7 Charlie Jenkins 2023-08-09 22 ({ \
add2cc6b6515f7 Charlie Jenkins 2023-08-09 23 unsigned long mmap_end; \
add2cc6b6515f7 Charlie Jenkins 2023-08-09 24 typeof(addr) _addr = (addr); \
c5712238cfe3f5 Charlie Jenkins 2024-01-29 25 if ((_addr) == 0 || \
c5712238cfe3f5 Charlie Jenkins 2024-01-29 26 (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) || \
c5712238cfe3f5 Charlie Jenkins 2024-01-29 27 ((_addr + len) > BIT(VA_BITS - 1))) \
add2cc6b6515f7 Charlie Jenkins 2023-08-09 @28 else \
c5712238cfe3f5 Charlie Jenkins 2024-01-29 29 mmap_end = (_addr + len); \
add2cc6b6515f7 Charlie Jenkins 2023-08-09 30 mmap_end; \
add2cc6b6515f7 Charlie Jenkins 2023-08-09 31 })
add2cc6b6515f7 Charlie Jenkins 2023-08-09 32
Hi Charlie,
kernel test robot noticed the following build errors:
[auto build test ERROR on 556e2d17cae620d549c5474b1ece053430cd50bc]
url: https://github.com/intel-lab-lkp/linux/commits/Charlie-Jenkins/riscv-mm-Use-hint-address-in-mmap-if-available/20240130-084208
base: 556e2d17cae620d549c5474b1ece053430cd50bc
patch link: https://lore.kernel.org/r/20240129-use_mmap_hint_address-v1-1-4c74da813ba1%40rivosinc.com
patch subject: [PATCH 1/3] riscv: mm: Use hint address in mmap if available
config: riscv-allnoconfig (https://download.01.org/0day-ci/archive/20240131/202401310513.lub8Ilwm-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project fdac7d0b6f74f919d319b31a0680c77f66732586)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240131/202401310513.lub8Ilwm-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401310513.lub8Ilwm-lkp@intel.com/
All errors (new ones prefixed by >>):
>> mm/mmap.c:1703:33: error: expected expression
1703 | const unsigned long mmap_end = arch_get_mmap_end(addr, len, flags);
| ^
arch/riscv/include/asm/processor.h:28:2: note: expanded from macro 'arch_get_mmap_end'
28 | else \
| ^
mm/mmap.c:1751:33: error: expected expression
1751 | const unsigned long mmap_end = arch_get_mmap_end(addr, len, flags);
| ^
arch/riscv/include/asm/processor.h:28:2: note: expanded from macro 'arch_get_mmap_end'
28 | else \
| ^
2 errors generated.
vim +1703 mm/mmap.c
f6795053dac8d4d Steve Capper 2018-12-06 1683
^1da177e4c3f415 Linus Torvalds 2005-04-16 1684 /* Get an address range which is currently unmapped.
^1da177e4c3f415 Linus Torvalds 2005-04-16 1685 * For shmat() with addr=0.
^1da177e4c3f415 Linus Torvalds 2005-04-16 1686 *
^1da177e4c3f415 Linus Torvalds 2005-04-16 1687 * Ugly calling convention alert:
^1da177e4c3f415 Linus Torvalds 2005-04-16 1688 * Return value with the low bits set means error value,
^1da177e4c3f415 Linus Torvalds 2005-04-16 1689 * ie
^1da177e4c3f415 Linus Torvalds 2005-04-16 1690 * if (ret & ~PAGE_MASK)
^1da177e4c3f415 Linus Torvalds 2005-04-16 1691 * error = ret;
^1da177e4c3f415 Linus Torvalds 2005-04-16 1692 *
^1da177e4c3f415 Linus Torvalds 2005-04-16 1693 * This function "knows" that -ENOMEM has the bits set.
^1da177e4c3f415 Linus Torvalds 2005-04-16 1694 */
^1da177e4c3f415 Linus Torvalds 2005-04-16 1695 unsigned long
4b439e25e29ec33 Christophe Leroy 2022-04-09 1696 generic_get_unmapped_area(struct file *filp, unsigned long addr,
4b439e25e29ec33 Christophe Leroy 2022-04-09 1697 unsigned long len, unsigned long pgoff,
4b439e25e29ec33 Christophe Leroy 2022-04-09 1698 unsigned long flags)
^1da177e4c3f415 Linus Torvalds 2005-04-16 1699 {
^1da177e4c3f415 Linus Torvalds 2005-04-16 1700 struct mm_struct *mm = current->mm;
1be7107fbe18eed Hugh Dickins 2017-06-19 1701 struct vm_area_struct *vma, *prev;
db4fbfb9523c935 Michel Lespinasse 2012-12-11 1702 struct vm_unmapped_area_info info;
2cb4de085f383cb Christophe Leroy 2022-04-09 @1703 const unsigned long mmap_end = arch_get_mmap_end(addr, len, flags);
^1da177e4c3f415 Linus Torvalds 2005-04-16 1704
f6795053dac8d4d Steve Capper 2018-12-06 1705 if (len > mmap_end - mmap_min_addr)
^1da177e4c3f415 Linus Torvalds 2005-04-16 1706 return -ENOMEM;
^1da177e4c3f415 Linus Torvalds 2005-04-16 1707
06abdfb47ee745a Benjamin Herrenschmidt 2007-05-06 1708 if (flags & MAP_FIXED)
06abdfb47ee745a Benjamin Herrenschmidt 2007-05-06 1709 return addr;
06abdfb47ee745a Benjamin Herrenschmidt 2007-05-06 1710
^1da177e4c3f415 Linus Torvalds 2005-04-16 1711 if (addr) {
^1da177e4c3f415 Linus Torvalds 2005-04-16 1712 addr = PAGE_ALIGN(addr);
1be7107fbe18eed Hugh Dickins 2017-06-19 1713 vma = find_vma_prev(mm, addr, &prev);
f6795053dac8d4d Steve Capper 2018-12-06 1714 if (mmap_end - len >= addr && addr >= mmap_min_addr &&
1be7107fbe18eed Hugh Dickins 2017-06-19 1715 (!vma || addr + len <= vm_start_gap(vma)) &&
1be7107fbe18eed Hugh Dickins 2017-06-19 1716 (!prev || addr >= vm_end_gap(prev)))
^1da177e4c3f415 Linus Torvalds 2005-04-16 1717 return addr;
^1da177e4c3f415 Linus Torvalds 2005-04-16 1718 }
^1da177e4c3f415 Linus Torvalds 2005-04-16 1719
db4fbfb9523c935 Michel Lespinasse 2012-12-11 1720 info.flags = 0;
db4fbfb9523c935 Michel Lespinasse 2012-12-11 1721 info.length = len;
4e99b02131b280b Heiko Carstens 2013-11-12 1722 info.low_limit = mm->mmap_base;
f6795053dac8d4d Steve Capper 2018-12-06 1723 info.high_limit = mmap_end;
db4fbfb9523c935 Michel Lespinasse 2012-12-11 1724 info.align_mask = 0;
09ef5283fd96ac4 Jaewon Kim 2020-04-10 1725 info.align_offset = 0;
db4fbfb9523c935 Michel Lespinasse 2012-12-11 1726 return vm_unmapped_area(&info);
^1da177e4c3f415 Linus Torvalds 2005-04-16 1727 }
4b439e25e29ec33 Christophe Leroy 2022-04-09 1728
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))) \ 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))) \ 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 | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-)