riscv: mm: Extend mappable memory up to hint address

PATCH Applied Charlie Jenkins 5 participants 22 messages Jan 31, 2024 – Aug 26, 2024

Subsystems (5)

Memory management Documentation Kselftest RISC-V +1 more subsystems

lore

Share & tools

lore

Charlie Jenkins

author

2024-01-31 01:07 UTC #0 lore raw

v3 cover riscv: mm: Extend mappable memory up to hint address

On riscv, mmap currently returns an address from the largest address space that can fit entirely inside of the hint address. This makes it such that the hint address is almost never returned. This patch raises the mappable area up to and including the hint address. This allows mmap to often return the hint address, which allows a performance improvement over searching for a valid address as well as making the behavior more similar to other architectures.

Note that a previous patch introduced stronger semantics compared to other architectures for riscv mmap. On riscv, mmap will not use bits in the upper bits of the virtual address depending on the hint address. On other architectures, a random address is returned in the address space requested. On all architectures the hint address will be returned if it is available. This allows riscv applications to configure how many bits in the virtual address should be left empty. This has the two benefits of being able to request address spaces that are smaller than the default and doesn't require the application to know the page table layout of riscv.

Signed-off-by Charlie Jenkins <[email protected]>

---
Changes in v3:
- Add back forgotten semi-colon
- Fix test cases
- Add support for rv32
- Change cover letter name so it's not the same as patch 1
- Link to v2: https://lore.kernel.org/r/[email protected]

Changes in v2:
- Add back forgotten "mmap_end = STACK_TOP_MAX"
- Link to v1: https://lore.kernel.org/r/[email protected]

---
Charlie Jenkins (3):
riscv: mm: Use hint address in mmap if available
selftests: riscv: Generalize mm selftests
docs: riscv: Define behavior of mmap

 Documentation/arch/riscv/vm-layout.rst           |  16 ++--
 arch/riscv/include/asm/processor.h               |  27 +++---
 tools/testing/selftests/riscv/mm/mmap_bottomup.c |  23 +----
 tools/testing/selftests/riscv/mm/mmap_default.c  |  23 +----
 tools/testing/selftests/riscv/mm/mmap_test.h     | 107 ++++++++++++++---------
 5 files changed, 83 insertions(+), 113 deletions(-)

---
base-commit: 556e2d17cae620d549c5474b1ece053430cd50bc
change-id: 20240119-use_mmap_hint_address-f9f4b1b6f5f1

Signature · 1 line

- Charlie

Charlie Jenkins

author

2024-01-31 01:07 UTC #1 lore raw

PATCH v3 1/3 riscv: mm: Use hint address in mmap if available

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 <[email protected]>

---
 arch/riscv/include/asm/processor.h | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

arch/riscv/include/asm/processor.h +11 ~~-16~~

index f19f861cda54..8ece7a8f0e18 100644
@@ -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

Signature · 1 line

2.43.0

Yangyu Chen

2024-01-31 14:41 UTC #2 lore raw

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

Quoted 92 lines · Charlie Jenkins

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 <[email protected]>
---
arch/riscv/include/asm/processor.h | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)
arch/riscv/include/asm/processor.h +0 -0
index f19f861cda54..8ece7a8f0e18 100644
@@ -14,22 +14,16 @@
#include <asm/ptrace.h>
arch/riscv/include/asm/processor.h +0 -4
-#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); \
arch/riscv/include/asm/processor.h +0 -1
-	if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) &&
is_compat_task())) \
arch/riscv/include/asm/processor.h +3 -0
+	if ((_addr) == 0 ||					\
+	    (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) ||	\
+	    ((_addr + len) > BIT(VA_BITS -
1))) \
mmap_end = STACK_TOP_MAX; \
arch/riscv/include/asm/processor.h +0 -3
-	else if ((_addr) >= VA_USER_SV57)			\
-		mmap_end = STACK_TOP_MAX;			\
-	else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >=
VA_BITS_SV48)) \
arch/riscv/include/asm/processor.h +0 -1
-		mmap_end = VA_USER_SV48;			\
else \
arch/riscv/include/asm/processor.h +1 -1
-		mmap_end = VA_USER_SV39;			\
+		mmap_end = (_addr + len);			\
mmap_end; \
})
arch/riscv/include/asm/processor.h +4 -6
@@ -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;						\
  })
arch/riscv/include/asm/processor.h +3 -0
+#ifdef CONFIG_64BIT
+#define DEFAULT_MAP_WINDOW	(UL(1) << (MMAP_VA_BITS - 1))
+#define STACK_TOP_MAX		TASK_SIZE_64

Quoted 4 lines

#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.

Yangyu Chen

2024-01-31 15:59 UTC #3 lore raw

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

Quoted 133 lines · Yangyu Chen

Quoted 95 lines · Charlie Jenkins
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 <[email protected]>
---
arch/riscv/include/asm/processor.h | 27 +++++++++++--------------- -
 1 file changed, 11 insertions(+), 16 deletions(-)
arch/riscv/include/asm/processor.h +0 -0
index f19f861cda54..8ece7a8f0e18 100644
@@ -14,22 +14,16 @@
#include <asm/ptrace.h>
arch/riscv/include/asm/processor.h +0 -4
-#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); \
arch/riscv/include/asm/processor.h +0 -1
-	if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) &&
is_compat_task())) \
arch/riscv/include/asm/processor.h +3 -0
+	if ((_addr) == 0 ||					\
+	    (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) ||	\
+	    ((_addr + len) > BIT(VA_BITS -
1))) \
mmap_end = STACK_TOP_MAX; \
arch/riscv/include/asm/processor.h +0 -3
-	else if ((_addr) >= VA_USER_SV57)			\
-		mmap_end = STACK_TOP_MAX;			\
-	else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >=
VA_BITS_SV48)) \
arch/riscv/include/asm/processor.h +0 -1
-		mmap_end = VA_USER_SV48;			\
else \
arch/riscv/include/asm/processor.h +1 -1
-		mmap_end = VA_USER_SV39;			\
+		mmap_end = (_addr + len);			\
mmap_end; \
})
arch/riscv/include/asm/processor.h +4 -6
@@ -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;						\
  })
arch/riscv/include/asm/processor.h +3 -0
+#ifdef CONFIG_64BIT
+#define DEFAULT_MAP_WINDOW	(UL(1) << (MMAP_VA_BITS - 1))
+#define STACK_TOP_MAX		TASK_SIZE_64
Quoted 4 lines
#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
Quoted 1 line
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.

Quoted 9 lines

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.

Charlie Jenkins

author

2024-02-02 02:28 UTC #4 lore raw

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

Quoted 150 lines · Yangyu Chen

Quoted 133 lines · Yangyu Chen
Quoted 95 lines · Charlie Jenkins
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 <[email protected]>
---
arch/riscv/include/asm/processor.h | 27 +++++++++++--------------- -
 1 file changed, 11 insertions(+), 16 deletions(-)
arch/riscv/include/asm/processor.h +0 -0
index f19f861cda54..8ece7a8f0e18 100644
@@ -14,22 +14,16 @@
#include <asm/ptrace.h>
arch/riscv/include/asm/processor.h +0 -4
-#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); \
arch/riscv/include/asm/processor.h +0 -1
-	if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) &&
is_compat_task())) \
arch/riscv/include/asm/processor.h +3 -0
+	if ((_addr) == 0 ||					\
+	    (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) ||	\
+	    ((_addr + len) > BIT(VA_BITS -
1))) \
mmap_end = STACK_TOP_MAX; \
arch/riscv/include/asm/processor.h +0 -3
-	else if ((_addr) >= VA_USER_SV57)			\
-		mmap_end = STACK_TOP_MAX;			\
-	else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >=
VA_BITS_SV48)) \
arch/riscv/include/asm/processor.h +0 -1
-		mmap_end = VA_USER_SV48;			\
else \
arch/riscv/include/asm/processor.h +1 -1
-		mmap_end = VA_USER_SV39;			\
+		mmap_end = (_addr + len);			\
mmap_end; \
})
arch/riscv/include/asm/processor.h +4 -6
@@ -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;						\
  })
arch/riscv/include/asm/processor.h +3 -0
+#ifdef CONFIG_64BIT
+#define DEFAULT_MAP_WINDOW	(UL(1) << (MMAP_VA_BITS - 1))
+#define STACK_TOP_MAX		TASK_SIZE_64
Quoted 4 lines
#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
Quoted 1 line
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.
Quoted 9 lines
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.

Quoted 4 lines

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.

- Charlie

Palmer Dabbelt

maintainer

2024-03-22 14:12 UTC #5 lore raw

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

Quoted 151 lines · Charlie Jenkins

Quoted 150 lines · Yangyu Chen
Quoted 133 lines · Yangyu Chen
Quoted 95 lines · Charlie Jenkins
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 <[email protected]>
---
arch/riscv/include/asm/processor.h | 27 +++++++++++--------------- -
 1 file changed, 11 insertions(+), 16 deletions(-)
arch/riscv/include/asm/processor.h +0 -0
index f19f861cda54..8ece7a8f0e18 100644
@@ -14,22 +14,16 @@
#include <asm/ptrace.h>
arch/riscv/include/asm/processor.h +0 -4
-#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); \
arch/riscv/include/asm/processor.h +0 -1
-	if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) &&
is_compat_task())) \
arch/riscv/include/asm/processor.h +3 -0
+	if ((_addr) == 0 ||					\
+	    (IS_ENABLED(CONFIG_COMPAT) && is_compat_task()) ||	\
+	    ((_addr + len) > BIT(VA_BITS -
1))) \
mmap_end = STACK_TOP_MAX; \
arch/riscv/include/asm/processor.h +0 -3
-	else if ((_addr) >= VA_USER_SV57)			\
-		mmap_end = STACK_TOP_MAX;			\
-	else if ((((_addr) >= VA_USER_SV48)) && (VA_BITS >=
VA_BITS_SV48)) \
arch/riscv/include/asm/processor.h +0 -1
-		mmap_end = VA_USER_SV48;			\
else \
arch/riscv/include/asm/processor.h +1 -1
-		mmap_end = VA_USER_SV39;			\
+		mmap_end = (_addr + len);			\
mmap_end; \
})
arch/riscv/include/asm/processor.h +4 -6
@@ -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;						\
  })
arch/riscv/include/asm/processor.h +3 -0
+#ifdef CONFIG_64BIT
+#define DEFAULT_MAP_WINDOW	(UL(1) << (MMAP_VA_BITS - 1))
+#define STACK_TOP_MAX		TASK_SIZE_64
Quoted 4 lines
#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
Quoted 1 line
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.
Quoted 9 lines
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.

Quoted 6 lines

Quoted 4 lines
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.

Quoted 2 lines

- Charlie

Levi Zim

2024-08-18 15:35 UTC #6 lore raw

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

Quoted 143 lines · Palmer Dabbelt

Quoted 142 lines · Charlie Jenkins
Quoted 140 lines · Yangyu Chen
Quoted 6 lines · Yangyu Chen
Quoted 5 lines · Charlie Jenkins
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.
Quoted 1 line
Quoted 1 line
This provides a performance benefit as well, allowing mmap
to exit
Quoted 83 lines
Quoted 83 lines
after
checking that the address is in range rather than searching for a valid
address.
Quoted 1 line
Quoted 1 line
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.
Quoted 1 line
Quoted 1 line
Signed-off-by Charlie Jenkins <[email protected]>
---
arch/riscv/include/asm/processor.h | 27 +++++++++++--------------- -
 1 file changed, 11 insertions(+), 16 deletions(-)
Quoted 1 line
Review
Quoted 1 line
(quoted diff) +0 -0
b/arch/riscv/include/asm/processor.h
index f19f861cda54..8ece7a8f0e18 100644
arch/riscv/include/asm/processor.h +0 -4
@@ -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;                        \
})
arch/riscv/include/asm/processor.h +4 -6
@@ -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;                        \
  })
Quoted 7 lines
Quoted 7 lines
arch/riscv/include/asm/processor.h +3 -0
+#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
Quoted 1 line
Quoted 1 line
I have carefully tested your patch on qemu with sv57. A bug
that
Quoted 22 lines
needs
to be solved is that mmap with the same hint address without MAP_FIXED
set will fail the second time.
Quoted 2 lines
Userspace code to reproduce the bug:
#include <sys/mman.h>
#include <stdio.h>
#include <stdint.h>
Quoted 1 line
void test(char *addr) {
    char *res = mmap(addr, 4096, PROT_READ | PROT_WRITE, MAP_ANONYMOUS
Quoted 1 line
MAP_PRIVATE, -1, 0);
    printf("hint %p got %p.\n", addr, res);
}
Quoted 1 line
int main (void) {
    test(1<<30);
    test(1<<30);
    test(1<<30);
    return 0;
}
Quoted 2 lines
output:
hint 0x40000000 got 0x40000000.
Quoted 9 lines
hint 0x40000000 got 0xffffffffffffffff.
hint 0x40000000 got 0xffffffffffffffff.
Quoted 2 lines
output on x86:
hint 0x40000000 got 0x40000000.
hint 0x40000000 got 0x7f9171363000.
hint 0x40000000 got 0x7f9171362000.
Quoted 1 line
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.

Quoted 7 lines

Quoted 1 line
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 suspect this has broke chromium and all other software that depends on chromium on riscv64. See also https://github.com/riscv-forks/electron/issues/4

Quoted 8 lines

Quoted 3 lines
- Charlie
_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

Levi Zim

2024-08-19 05:56 UTC #7 lore raw

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

In reply to Palmer Dabbelt

Quoted 143 lines · Palmer Dabbelt

Quoted 142 lines · Charlie Jenkins
Quoted 140 lines · Yangyu Chen
Quoted 6 lines · Yangyu Chen
Quoted 5 lines · Charlie Jenkins
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.
Quoted 1 line
Quoted 1 line
This provides a performance benefit as well, allowing mmap
to exit
Quoted 83 lines
Quoted 83 lines
after
checking that the address is in range rather than searching for a valid
address.
Quoted 1 line
Quoted 1 line
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.
Quoted 1 line
Quoted 1 line
Signed-off-by Charlie Jenkins <[email protected]>
---
arch/riscv/include/asm/processor.h | 27 +++++++++++--------------- -
 1 file changed, 11 insertions(+), 16 deletions(-)
Quoted 1 line
Review
Quoted 1 line
(quoted diff) +0 -0
b/arch/riscv/include/asm/processor.h
index f19f861cda54..8ece7a8f0e18 100644
arch/riscv/include/asm/processor.h +0 -4
@@ -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;                        \
})
arch/riscv/include/asm/processor.h +4 -6
@@ -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;                        \
  })
Quoted 7 lines
Quoted 7 lines
arch/riscv/include/asm/processor.h +3 -0
+#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
Quoted 1 line
Quoted 1 line
I have carefully tested your patch on qemu with sv57. A bug
that
Quoted 22 lines
needs
to be solved is that mmap with the same hint address without MAP_FIXED
set will fail the second time.
Quoted 2 lines
Userspace code to reproduce the bug:
#include <sys/mman.h>
#include <stdio.h>
#include <stdint.h>
Quoted 1 line
void test(char *addr) {
    char *res = mmap(addr, 4096, PROT_READ | PROT_WRITE, MAP_ANONYMOUS
Quoted 1 line
MAP_PRIVATE, -1, 0);
    printf("hint %p got %p.\n", addr, res);
}
Quoted 1 line
int main (void) {
    test(1<<30);
    test(1<<30);
    test(1<<30);
    return 0;
}
Quoted 2 lines
output:
hint 0x40000000 got 0x40000000.
Quoted 9 lines
hint 0x40000000 got 0xffffffffffffffff.
hint 0x40000000 got 0xffffffffffffffff.
Quoted 2 lines
output on x86:
hint 0x40000000 got 0x40000000.
hint 0x40000000 got 0x7f9171363000.
hint 0x40000000 got 0x7f9171362000.
Quoted 1 line
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.

Quoted 7 lines

Quoted 1 line
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) {
        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.

Quoted 2 lines

Quoted 2 lines
- Charlie

Sincerely,
Levi

Charlie Jenkins

author

2024-08-19 17:01 UTC #8 lore raw

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

Quoted 160 lines · Levi Zim

Quoted 148 lines · Palmer Dabbelt
Quoted 137 lines · Charlie Jenkins
Quoted 136 lines · Yangyu Chen
Quoted 6 lines · Yangyu Chen
Quoted 5 lines · Charlie Jenkins
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.
Quoted 1 line
Quoted 1 line
This provides a performance benefit as well, allowing
mmap to exit
Quoted 83 lines
Quoted 83 lines
after
checking that the address is in range rather than searching for a valid
address.
Quoted 1 line
Quoted 1 line
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.
Quoted 1 line
Quoted 1 line
Signed-off-by Charlie Jenkins <[email protected]>
---
arch/riscv/include/asm/processor.h | 27 +++++++++++--------------- -
 1 file changed, 11 insertions(+), 16 deletions(-)
Quoted 1 line
Review
Quoted 1 line
(quoted diff) +0 -0
b/arch/riscv/include/asm/processor.h
index f19f861cda54..8ece7a8f0e18 100644
arch/riscv/include/asm/processor.h +0 -4
@@ -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;                        \
})
arch/riscv/include/asm/processor.h +4 -6
@@ -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;                        \
  })
Quoted 7 lines
Quoted 7 lines
arch/riscv/include/asm/processor.h +3 -0
+#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
Quoted 1 line
Quoted 1 line
I have carefully tested your patch on qemu with sv57. A
bug that
Quoted 22 lines
needs
to be solved is that mmap with the same hint address without MAP_FIXED
set will fail the second time.
Quoted 2 lines
Userspace code to reproduce the bug:
#include <sys/mman.h>
#include <stdio.h>
#include <stdint.h>
Quoted 1 line
void test(char *addr) {
    char *res = mmap(addr, 4096, PROT_READ | PROT_WRITE, MAP_ANONYMOUS
Quoted 1 line
MAP_PRIVATE, -1, 0);
    printf("hint %p got %p.\n", addr, res);
}
Quoted 1 line
int main (void) {
    test(1<<30);
    test(1<<30);
    test(1<<30);
    return 0;
}
Quoted 2 lines
output:
hint 0x40000000 got 0x40000000.
Quoted 9 lines
hint 0x40000000 got 0xffffffffffffffff.
hint 0x40000000 got 0xffffffffffffffff.
Quoted 2 lines
output on x86:
hint 0x40000000 got 0x40000000.
hint 0x40000000 got 0x7f9171363000.
hint 0x40000000 got 0x7f9171362000.
Quoted 1 line
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.
Quoted 6 lines
Quoted 4 lines
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.
Quoted 7 lines
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:

Quoted 8 lines

#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.

Quoted 11 lines

        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;
}

Quoted 3 lines

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.

This code is unfortunately relying on the previously mostly undefined behavior of the hint address in mmap. 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.

- Charlie

Quoted 8 lines

See also https://github.com/riscv-forks/electron/issues/4
Quoted 2 lines
Quoted 2 lines
- Charlie
Sincerely,
Levi

Levi Zim

2024-08-20 01:49 UTC #9 lore raw

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

Quoted 161 lines · Charlie Jenkins

Quoted 160 lines · Levi Zim
Quoted 146 lines · Palmer Dabbelt
Quoted 137 lines · Charlie Jenkins
Quoted 136 lines · Yangyu Chen
Quoted 6 lines · Yangyu Chen
Quoted 5 lines · Charlie Jenkins
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.
Quoted 1 line
Quoted 1 line
This provides a performance benefit as well, allowing
mmap to exit
Quoted 83 lines
Quoted 83 lines
after
checking that the address is in range rather than searching for a valid
address.
Quoted 1 line
Quoted 1 line
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.
Quoted 1 line
Quoted 1 line
Signed-off-by Charlie Jenkins<[email protected]>
---
  arch/riscv/include/asm/processor.h | 27 +++++++++++---------------
-
  1 file changed, 11 insertions(+), 16 deletions(-)
Quoted 1 line
Review
Quoted 1 line
(quoted diff) +0 -0
b/arch/riscv/include/asm/processor.h
index f19f861cda54..8ece7a8f0e18 100644
arch/riscv/include/asm/processor.h +8 ~~-16~~
@@ -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;                        \
  })
Quoted 7 lines
Quoted 7 lines
arch/riscv/include/asm/processor.h +3 -0
+#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
Quoted 1 line
Quoted 1 line
I have carefully tested your patch on qemu with sv57. A
bug that
Quoted 22 lines
needs
to be solved is that mmap with the same hint address without MAP_FIXED
set will fail the second time.
Quoted 2 lines
Userspace code to reproduce the bug:
#include <sys/mman.h>
#include <stdio.h>
#include <stdint.h>
Quoted 1 line
void test(char *addr) {
    char *res = mmap(addr, 4096, PROT_READ | PROT_WRITE,
MAP_ANONYMOUS
Quoted 1 line
MAP_PRIVATE, -1, 0);
    printf("hint %p got %p.\n", addr, res);
}
Quoted 1 line
int main (void) {
    test(1<<30);
    test(1<<30);
    test(1<<30);
    return 0;
}
Quoted 2 lines
output:
hint 0x40000000 got 0x40000000.
Quoted 9 lines
hint 0x40000000 got 0xffffffffffffffff.
hint 0x40000000 got 0xffffffffffffffff.
Quoted 2 lines
output on x86:
hint 0x40000000 got 0x40000000.
hint 0x40000000 got 0x7f9171363000.
hint 0x40000000 got 0x7f9171362000.
Quoted 1 line
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.
Quoted 5 lines
Quoted 4 lines
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.
Quoted 7 lines
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>

Quoted 8 lines

Quoted 5 lines
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.

Quoted 24 lines

Quoted 11 lines
        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;
}
Quoted 3 lines
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.

Quoted 4 lines

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:

Quoted 3 lines

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:

Quoted 8 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

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".

Quoted 3 lines

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.

Quoted 8 lines

- Charlie
Quoted 6 lines
See alsohttps://github.com/riscv-forks/electron/issues/4
Quoted 1 line
Quoted 1 line
- Charlie
Sincerely,
Levi

Charlie Jenkins

author

2024-08-20 02:43 UTC #10 lore raw

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

Quoted 205 lines · Levi Zim

Quoted 161 lines · Charlie Jenkins
Quoted 160 lines · Levi Zim
Quoted 146 lines · Palmer Dabbelt
Quoted 137 lines · Charlie Jenkins
Quoted 136 lines · Yangyu Chen
Quoted 6 lines · Yangyu Chen
Quoted 5 lines · Charlie Jenkins
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.
Quoted 1 line
Quoted 1 line
This provides a performance benefit as well, allowing
mmap to exit
Quoted 83 lines
Quoted 83 lines
after
checking that the address is in range rather than searching for a valid
address.
Quoted 1 line
Quoted 1 line
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.
Quoted 1 line
Quoted 1 line
Signed-off-by Charlie Jenkins<[email protected]>
---
  arch/riscv/include/asm/processor.h | 27 +++++++++++---------------
-
  1 file changed, 11 insertions(+), 16 deletions(-)
Quoted 1 line
Review
Quoted 1 line
(quoted diff) +0 -0
b/arch/riscv/include/asm/processor.h
index f19f861cda54..8ece7a8f0e18 100644
arch/riscv/include/asm/processor.h +8 ~~-16~~
@@ -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;                        \
  })
Quoted 7 lines
Quoted 7 lines
arch/riscv/include/asm/processor.h +3 -0
+#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
Quoted 1 line
Quoted 1 line
I have carefully tested your patch on qemu with sv57. A
bug that
Quoted 22 lines
needs
to be solved is that mmap with the same hint address without MAP_FIXED
set will fail the second time.
Quoted 2 lines
Userspace code to reproduce the bug:
#include <sys/mman.h>
#include <stdio.h>
#include <stdint.h>
Quoted 1 line
void test(char *addr) {
    char *res = mmap(addr, 4096, PROT_READ | PROT_WRITE,
MAP_ANONYMOUS
Quoted 1 line
MAP_PRIVATE, -1, 0);
    printf("hint %p got %p.\n", addr, res);
}
Quoted 1 line
int main (void) {
    test(1<<30);
    test(1<<30);
    test(1<<30);
    return 0;
}
Quoted 2 lines
output:
hint 0x40000000 got 0x40000000.
Quoted 9 lines
hint 0x40000000 got 0xffffffffffffffff.
hint 0x40000000 got 0xffffffffffffffff.
Quoted 2 lines
output on x86:
hint 0x40000000 got 0x40000000.
hint 0x40000000 got 0x7f9171363000.
hint 0x40000000 got 0x7f9171362000.
Quoted 1 line
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.
Quoted 5 lines
Quoted 4 lines
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.
Quoted 7 lines
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>
Quoted 8 lines
Quoted 5 lines
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.
Quoted 24 lines
Quoted 11 lines
        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;
}
Quoted 3 lines
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.
Quoted 4 lines
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.

Quoted 6 lines

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.

Yes that is fair. A "hint" that does not guarantee anything is useless so architectures have abused the term quite a bit.

Quoted 11 lines

Quoting the man page:
Quoted 3 lines
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.

The man pages for riscv are in desperate need of attention. I have submitted a couple of updates to them recently, but there is a lot more work to be done to help developers.

Quoted 14 lines

And quoting the man page again about the errno:
Quoted 11 lines
      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.

Quoted 8 lines

Quoted 3 lines
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 ENOMEM means something like "no memory available in the requested region".

Quoted 8 lines

Quoted 3 lines
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".

The hint address design of mmap is a tricky one because it is largely implementation defined and what the man pages say is not how it is implemented in most architectures!

Quoted 8 lines

Quoted 3 lines
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.

Having a flag could also lead to a generic way of defining this behavior. Other architectures do not provide a way for applications to guarantee that some number of bits are left unused in a virtual address, and that was one of the motivating design goals here.

- Charlie

Quoted 9 lines

Quoted 9 lines
- Charlie
Quoted 6 lines
See alsohttps://github.com/riscv-forks/electron/issues/4
Quoted 1 line
Quoted 1 line
- Charlie
Sincerely,
Levi

Levi Zim

2024-08-20 01:58 UTC #11 lore raw

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

In reply to Charlie Jenkins

Quoted 161 lines · Charlie Jenkins

Quoted 160 lines · Levi Zim
Quoted 146 lines · Palmer Dabbelt
Quoted 137 lines · Charlie Jenkins
Quoted 136 lines · Yangyu Chen
Quoted 6 lines · Yangyu Chen
Quoted 5 lines · Charlie Jenkins
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.
Quoted 1 line
Quoted 1 line
This provides a performance benefit as well, allowing
mmap to exit
Quoted 83 lines
Quoted 83 lines
after
checking that the address is in range rather than searching for a valid
address.
Quoted 1 line
Quoted 1 line
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.
Quoted 1 line
Quoted 1 line
Signed-off-by Charlie Jenkins<[email protected]>
---
  arch/riscv/include/asm/processor.h | 27 +++++++++++---------------
-
  1 file changed, 11 insertions(+), 16 deletions(-)
Quoted 1 line
Review
Quoted 1 line
(quoted diff) +0 -0
b/arch/riscv/include/asm/processor.h
index f19f861cda54..8ece7a8f0e18 100644
arch/riscv/include/asm/processor.h +8 ~~-16~~
@@ -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;                        \
  })
Quoted 7 lines
Quoted 7 lines
arch/riscv/include/asm/processor.h +3 -0
+#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
Quoted 1 line
Quoted 1 line
I have carefully tested your patch on qemu with sv57. A
bug that
Quoted 22 lines
needs
to be solved is that mmap with the same hint address without MAP_FIXED
set will fail the second time.
Quoted 2 lines
Userspace code to reproduce the bug:
#include <sys/mman.h>
#include <stdio.h>
#include <stdint.h>
Quoted 1 line
void test(char *addr) {
    char *res = mmap(addr, 4096, PROT_READ | PROT_WRITE,
MAP_ANONYMOUS
Quoted 1 line
MAP_PRIVATE, -1, 0);
    printf("hint %p got %p.\n", addr, res);
}
Quoted 1 line
int main (void) {
    test(1<<30);
    test(1<<30);
    test(1<<30);
    return 0;
}
Quoted 2 lines
output:
hint 0x40000000 got 0x40000000.
Quoted 9 lines
hint 0x40000000 got 0xffffffffffffffff.
hint 0x40000000 got 0xffffffffffffffff.
Quoted 2 lines
output on x86:
hint 0x40000000 got 0x40000000.
hint 0x40000000 got 0x7f9171363000.
hint 0x40000000 got 0x7f9171362000.
Quoted 1 line
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.
Quoted 5 lines
Quoted 4 lines
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.
Quoted 7 lines
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>

Quoted 8 lines

Quoted 5 lines
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.

Quoted 24 lines

Quoted 11 lines
        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;
}
Quoted 3 lines
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.

Quoted 4 lines

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:

Quoted 3 lines

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.

And quoting the man page again about the errno:

Quoted 8 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

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".

Quoted 3 lines

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.

Quoted 8 lines

- Charlie
Quoted 6 lines
See alsohttps://github.com/riscv-forks/electron/issues/4
Quoted 1 line
Quoted 1 line
- Charlie
Sincerely,
Levi

I accidentally introduced some HTML into this reply so this reply is resent as plain text.

Sincerely,
Levi

Palmer Dabbelt

maintainer

2024-08-21 22:17 UTC #12 lore raw

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

Quoted 244 lines · On Mon, 19 Aug 2024 18:58:18 PDT (-0700), [email protected] wrote:

Quoted 161 lines · Charlie Jenkins
Quoted 160 lines · Levi Zim
Quoted 146 lines · Palmer Dabbelt
Quoted 137 lines · Charlie Jenkins
Quoted 136 lines · Yangyu Chen
Quoted 6 lines · Yangyu Chen
Quoted 5 lines · Charlie Jenkins
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.
Quoted 1 line
Quoted 1 line
This provides a performance benefit as well, allowing
mmap to exit
Quoted 83 lines
Quoted 83 lines
after
checking that the address is in range rather than searching for a valid
address.
Quoted 1 line
Quoted 1 line
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.
Quoted 1 line
Quoted 1 line
Signed-off-by Charlie Jenkins<[email protected]>
---
  arch/riscv/include/asm/processor.h | 27 +++++++++++---------------
-
  1 file changed, 11 insertions(+), 16 deletions(-)
Quoted 1 line
Review
Quoted 1 line
(quoted diff) +0 -0
b/arch/riscv/include/asm/processor.h
index f19f861cda54..8ece7a8f0e18 100644
arch/riscv/include/asm/processor.h +8 ~~-16~~
@@ -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;                        \
  })
Quoted 7 lines
Quoted 7 lines
arch/riscv/include/asm/processor.h +3 -0
+#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
Quoted 1 line
Quoted 1 line
I have carefully tested your patch on qemu with sv57. A
bug that
Quoted 22 lines
needs
to be solved is that mmap with the same hint address without MAP_FIXED
set will fail the second time.
Quoted 2 lines
Userspace code to reproduce the bug:
#include <sys/mman.h>
#include <stdio.h>
#include <stdint.h>
Quoted 1 line
void test(char *addr) {
    char *res = mmap(addr, 4096, PROT_READ | PROT_WRITE,
MAP_ANONYMOUS
Quoted 1 line
MAP_PRIVATE, -1, 0);
    printf("hint %p got %p.\n", addr, res);
}
Quoted 1 line
int main (void) {
    test(1<<30);
    test(1<<30);
    test(1<<30);
    return 0;
}
Quoted 2 lines
output:
hint 0x40000000 got 0x40000000.
Quoted 9 lines
hint 0x40000000 got 0xffffffffffffffff.
hint 0x40000000 got 0xffffffffffffffff.
Quoted 2 lines
output on x86:
hint 0x40000000 got 0x40000000.
hint 0x40000000 got 0x7f9171363000.
hint 0x40000000 got 0x7f9171362000.
Quoted 1 line
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.
Quoted 5 lines
Quoted 4 lines
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.
Quoted 7 lines
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>
Quoted 8 lines
Quoted 5 lines
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.
Quoted 24 lines
Quoted 11 lines
        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;
}
Quoted 3 lines
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.
Quoted 4 lines
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:
Quoted 3 lines
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:
Quoted 8 lines
      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.
Quoted 6 lines
      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.

Quoted 7 lines

Quoted 2 lines
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. Charlie's going to try and throw together a patch for that, hopefully it'll sort things out.

Quoted 24 lines

Quoted 3 lines
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.
Quoted 8 lines
- Charlie
Quoted 6 lines
See alsohttps://github.com/riscv-forks/electron/issues/4
Quoted 1 line
Quoted 1 line
- Charlie
Sincerely,
Levi
I accidentally introduced some HTML into this reply so this reply is resent as plain text.
Sincerely,
Levi

Yangyu Chen

2024-08-22 02:52 UTC #13 lore raw

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

Quoted 257 lines · Palmer Dabbelt

Quoted 244 lines · On Mon, 19 Aug 2024 18:58:18 PDT (-0700), [email protected] wrote:
Quoted 161 lines · Charlie Jenkins
Quoted 160 lines · Levi Zim
Quoted 146 lines · Palmer Dabbelt
Quoted 137 lines · Charlie Jenkins
Quoted 136 lines · Yangyu Chen
Quoted 6 lines · Yangyu Chen
Quoted 5 lines · Charlie Jenkins
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.
Quoted 1 line
Quoted 1 line
This provides a performance benefit as well, allowing
mmap to exit
Quoted 83 lines
Quoted 83 lines
after
checking that the address is in range rather than searching for a valid
address.
Quoted 1 line
Quoted 1 line
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.
Quoted 1 line
Quoted 1 line
Signed-off-by Charlie Jenkins<[email protected]>
---
 arch/riscv/include/asm/processor.h | 27 +++++++++++---------------
-
 1 file changed, 11 insertions(+), 16 deletions(-)
Quoted 1 line
Review
Quoted 1 line
(quoted diff) +0 -0
b/arch/riscv/include/asm/processor.h
index f19f861cda54..8ece7a8f0e18 100644
arch/riscv/include/asm/processor.h +8 ~~-16~~
@@ -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;                        \
 })
Quoted 7 lines
Quoted 7 lines
arch/riscv/include/asm/processor.h +3 -0
+#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
Quoted 1 line
Quoted 1 line
I have carefully tested your patch on qemu with sv57. A
bug that
Quoted 22 lines
needs
to be solved is that mmap with the same hint address without MAP_FIXED
set will fail the second time.
Quoted 2 lines
Userspace code to reproduce the bug:
#include <sys/mman.h>
#include <stdio.h>
#include <stdint.h>
Quoted 1 line
void test(char *addr) {
char *res = mmap(addr, 4096, PROT_READ | PROT_WRITE,
MAP_ANONYMOUS
Quoted 1 line
MAP_PRIVATE, -1, 0);
    printf("hint %p got %p.\n", addr, res);
}
Quoted 1 line
int main (void) {
    test(1<<30);
    test(1<<30);
    test(1<<30);
    return 0;
}
Quoted 2 lines
output:
hint 0x40000000 got 0x40000000.
Quoted 9 lines
hint 0x40000000 got 0xffffffffffffffff.
hint 0x40000000 got 0xffffffffffffffff.
Quoted 2 lines
output on x86:
hint 0x40000000 got 0x40000000.
hint 0x40000000 got 0x7f9171363000.
hint 0x40000000 got 0x7f9171362000.
Quoted 1 line
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.
Quoted 5 lines
Quoted 4 lines
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.
Quoted 7 lines
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>
Quoted 8 lines
Quoted 5 lines
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.
Quoted 24 lines
Quoted 11 lines
        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;
}
Quoted 3 lines
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.
Quoted 4 lines
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:
Quoted 3 lines
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:
Quoted 8 lines
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.
Quoted 6 lines
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.
Quoted 7 lines
Quoted 2 lines
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".

Quoted 7 lines

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/[email protected]/

Quoted 26 lines

Charlie's going to try and throw together a patch for that, hopefully it'll sort things out.
Quoted 24 lines
Quoted 3 lines
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.
Quoted 8 lines
- Charlie
Quoted 6 lines
See alsohttps://github.com/riscv-forks/electron/issues/4
Quoted 1 line
Quoted 1 line
- Charlie
Sincerely,
Levi
I accidentally introduced some HTML into this reply so this reply is resent as plain text.
Sincerely,
Levi

Charlie Jenkins

author

2024-08-23 04:39 UTC #14 lore raw

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

Quoted 275 lines · Yangyu Chen

Quoted 257 lines · Palmer Dabbelt
Quoted 244 lines · On Mon, 19 Aug 2024 18:58:18 PDT (-0700), [email protected] wrote:
Quoted 161 lines · Charlie Jenkins
Quoted 160 lines · Levi Zim
Quoted 146 lines · Palmer Dabbelt
Quoted 137 lines · Charlie Jenkins
Quoted 136 lines · Yangyu Chen
Quoted 6 lines · Yangyu Chen
Quoted 5 lines · Charlie Jenkins
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.
Quoted 1 line
Quoted 1 line
This provides a performance benefit as well, allowing
mmap to exit
Quoted 83 lines
Quoted 83 lines
after
checking that the address is in range rather than searching for a valid
address.
Quoted 1 line
Quoted 1 line
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.
Quoted 1 line
Quoted 1 line
Signed-off-by Charlie Jenkins<[email protected]>
---
 arch/riscv/include/asm/processor.h | 27 +++++++++++---------------
-
 1 file changed, 11 insertions(+), 16 deletions(-)
Quoted 1 line
Review
Quoted 1 line
(quoted diff) +0 -0
b/arch/riscv/include/asm/processor.h
index f19f861cda54..8ece7a8f0e18 100644
arch/riscv/include/asm/processor.h +8 ~~-16~~
@@ -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;                        \
 })
Quoted 7 lines
Quoted 7 lines
arch/riscv/include/asm/processor.h +3 -0
+#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
Quoted 1 line
Quoted 1 line
I have carefully tested your patch on qemu with sv57. A
bug that
Quoted 22 lines
needs
to be solved is that mmap with the same hint address without MAP_FIXED
set will fail the second time.
Quoted 2 lines
Userspace code to reproduce the bug:
#include <sys/mman.h>
#include <stdio.h>
#include <stdint.h>
Quoted 1 line
void test(char *addr) {
char *res = mmap(addr, 4096, PROT_READ | PROT_WRITE,
MAP_ANONYMOUS
Quoted 1 line
MAP_PRIVATE, -1, 0);
    printf("hint %p got %p.\n", addr, res);
}
Quoted 1 line
int main (void) {
    test(1<<30);
    test(1<<30);
    test(1<<30);
    return 0;
}
Quoted 2 lines
output:
hint 0x40000000 got 0x40000000.
Quoted 9 lines
hint 0x40000000 got 0xffffffffffffffff.
hint 0x40000000 got 0xffffffffffffffff.
Quoted 2 lines
output on x86:
hint 0x40000000 got 0x40000000.
hint 0x40000000 got 0x7f9171363000.
hint 0x40000000 got 0x7f9171362000.
Quoted 1 line
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.
Quoted 5 lines
Quoted 4 lines
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.
Quoted 7 lines
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>
Quoted 8 lines
Quoted 5 lines
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.
Quoted 24 lines
Quoted 11 lines
        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;
}
Quoted 3 lines
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.
Quoted 4 lines
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:
Quoted 3 lines
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:
Quoted 8 lines
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.
Quoted 6 lines
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.
Quoted 7 lines
Quoted 2 lines
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".
Quoted 7 lines
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.

Quoted 2 lines

I think we can just fix like that patch:
https://lore.kernel.org/lkml/[email protected]/

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. 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. 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.

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.

- Charlie

Quoted 29 lines

Quoted 27 lines
Charlie's going to try and throw together a patch for that, hopefully it'll sort things out.
Quoted 24 lines
Quoted 3 lines
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.
Quoted 8 lines
- Charlie
Quoted 6 lines
See alsohttps://github.com/riscv-forks/electron/issues/4
Quoted 1 line
Quoted 1 line
- Charlie
Sincerely,
Levi
I accidentally introduced some HTML into this reply so this reply is resent as plain text.
Sincerely,
Levi

Yangyu Chen

2024-08-23 05:28 UTC #15 lore raw

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

Quoted 280 lines · Charlie Jenkins

Quoted 275 lines · Yangyu Chen
Quoted 257 lines · Palmer Dabbelt
Quoted 244 lines · On Mon, 19 Aug 2024 18:58:18 PDT (-0700), [email protected] wrote:
Quoted 161 lines · Charlie Jenkins
Quoted 160 lines · Levi Zim
Quoted 146 lines · Palmer Dabbelt
Quoted 137 lines · Charlie Jenkins
Quoted 136 lines · Yangyu Chen
Quoted 6 lines · Yangyu Chen
Quoted 5 lines · Charlie Jenkins
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.
Quoted 1 line
Quoted 1 line
This provides a performance benefit as well, allowing
mmap to exit
Quoted 83 lines
Quoted 83 lines
after
checking that the address is in range rather than searching for a valid
address.
Quoted 1 line
Quoted 1 line
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.
Quoted 1 line
Quoted 1 line
Signed-off-by: Charlie Jenkins<[email protected]>
---
arch/riscv/include/asm/processor.h | 27 +++++++++++--------------- -
1 file changed, 11 insertions(+), 16 deletions(-)
Quoted 1 line
Quoted 1 line
diff --git a/arch/riscv/include/asm/processor.h
b/arch/riscv/include/asm/processor.h
index f19f861cda54..8ece7a8f0e18 100644
arch/riscv/include/asm/processor.h +4 ~~-10~~
@@ -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;                        \
})
arch/riscv/include/asm/processor.h +4 -6
@@ -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;                        \
 })
Quoted 7 lines
Quoted 7 lines
arch/riscv/include/asm/processor.h +3 -0
+#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
Quoted 1 line
Quoted 1 line
I have carefully tested your patch on qemu with sv57. A
bug that
Quoted 22 lines
needs
to be solved is that mmap with the same hint address without MAP_FIXED
set will fail the second time.
Quoted 2 lines
Userspace code to reproduce the bug:
#include <sys/mman.h>
#include <stdio.h>
#include <stdint.h>
Quoted 1 line
void test(char *addr) {
char *res = mmap(addr, 4096, PROT_READ | PROT_WRITE,
MAP_ANONYMOUS
Quoted 1 line
MAP_PRIVATE, -1, 0);
printf("hint %p got %p.\n", addr, res);
}
Quoted 1 line
int main (void) {
   test(1<<30);
   test(1<<30);
   test(1<<30);
   return 0;
}
Quoted 2 lines
output:
hint 0x40000000 got 0x40000000.
Quoted 9 lines
hint 0x40000000 got 0xffffffffffffffff.
hint 0x40000000 got 0xffffffffffffffff.
Quoted 2 lines
output on x86:
hint 0x40000000 got 0x40000000.
hint 0x40000000 got 0x7f9171363000.
hint 0x40000000 got 0x7f9171362000.
Quoted 1 line
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.
Quoted 5 lines
Quoted 4 lines
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.
Quoted 7 lines
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>
Quoted 8 lines
Quoted 5 lines
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.
Quoted 24 lines
Quoted 11 lines
       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;
}
Quoted 3 lines
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.
Quoted 4 lines
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:
Quoted 3 lines
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:
Quoted 8 lines
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.
Quoted 6 lines
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.
Quoted 7 lines
Quoted 2 lines
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".
Quoted 7 lines
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.

Quoted 5 lines

Quoted 2 lines
I think we can just fix like that patch:
https://lore.kernel.org/lkml/[email protected]/
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.

Quoted 6 lines

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).

Quoted 5 lines

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.

Quoted 5 lines

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.

Quoted 29 lines

- Charlie
Quoted 27 lines
Quoted 27 lines
Charlie's going to try and throw together a patch for that, hopefully it'll sort things out.
Quoted 24 lines
Quoted 3 lines
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.
Quoted 8 lines
- Charlie
Quoted 6 lines
See alsohttps://github.com/riscv-forks/electron/issues/4
Quoted 1 line
Quoted 1 line
- Charlie
Sincerely,
Levi
I accidentally introduced some HTML into this reply so this reply is resent as plain text.
Sincerely,
Levi

Charlie Jenkins

author

2024-08-23 05:57 UTC #16 lore raw

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

Quoted 313 lines · Yangyu Chen

Quoted 280 lines · Charlie Jenkins
Quoted 275 lines · Yangyu Chen
Quoted 257 lines · Palmer Dabbelt
Quoted 244 lines · On Mon, 19 Aug 2024 18:58:18 PDT (-0700), [email protected] wrote:
Quoted 161 lines · Charlie Jenkins
Quoted 160 lines · Levi Zim
Quoted 146 lines · Palmer Dabbelt
Quoted 137 lines · Charlie Jenkins
Quoted 136 lines · Yangyu Chen
Quoted 6 lines · Yangyu Chen
Quoted 5 lines · Charlie Jenkins
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.
Quoted 1 line
This provides a performance benefit as well, allowing
mmap to exit
Quoted 83 lines
Quoted 83 lines
after
checking that the address is in range rather than searching for a valid
address.
Quoted 1 line
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.
Quoted 1 line
Signed-off-by: Charlie Jenkins<[email protected]>
---
arch/riscv/include/asm/processor.h | 27 +++++++++++--------------- -
1 file changed, 11 insertions(+), 16 deletions(-)
Quoted 1 line
diff --git a/arch/riscv/include/asm/processor.h
b/arch/riscv/include/asm/processor.h
index f19f861cda54..8ece7a8f0e18 100644
arch/riscv/include/asm/processor.h +4 ~~-10~~
@@ -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;                        \
})
arch/riscv/include/asm/processor.h +4 -6
@@ -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;                        \
 })
Quoted 7 lines
Quoted 7 lines
arch/riscv/include/asm/processor.h +3 -0
+#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
Quoted 1 line
I have carefully tested your patch on qemu with sv57. A
bug that
Quoted 22 lines
needs
to be solved is that mmap with the same hint address without MAP_FIXED
set will fail the second time.
Quoted 2 lines
Userspace code to reproduce the bug:
#include <sys/mman.h>
#include <stdio.h>
#include <stdint.h>
Quoted 1 line
void test(char *addr) {
char *res = mmap(addr, 4096, PROT_READ | PROT_WRITE,
MAP_ANONYMOUS
Quoted 1 line
MAP_PRIVATE, -1, 0);
printf("hint %p got %p.\n", addr, res);
}
Quoted 1 line
int main (void) {
   test(1<<30);
   test(1<<30);
   test(1<<30);
   return 0;
}
Quoted 2 lines
output:
hint 0x40000000 got 0x40000000.
Quoted 9 lines
hint 0x40000000 got 0xffffffffffffffff.
hint 0x40000000 got 0xffffffffffffffff.
Quoted 2 lines
output on x86:
hint 0x40000000 got 0x40000000.
hint 0x40000000 got 0x7f9171363000.
hint 0x40000000 got 0x7f9171362000.
Quoted 1 line
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.
Quoted 5 lines
Quoted 4 lines
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.
Quoted 7 lines
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>
Quoted 8 lines
Quoted 5 lines
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.
Quoted 24 lines
Quoted 11 lines
       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;
}
Quoted 3 lines
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.
Quoted 4 lines
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:
Quoted 3 lines
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:
Quoted 8 lines
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.
Quoted 6 lines
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.
Quoted 7 lines
Quoted 2 lines
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".
Quoted 7 lines
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.
Quoted 5 lines
Quoted 2 lines
I think we can just fix like that patch:
https://lore.kernel.org/lkml/[email protected]/
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.
Quoted 6 lines
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).
Quoted 5 lines
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.
Quoted 5 lines
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.

Quoted 5 lines

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

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.

Quoted 32 lines

Quoted 30 lines
- Charlie
Quoted 27 lines
Quoted 27 lines
Charlie's going to try and throw together a patch for that, hopefully it'll sort things out.
Quoted 24 lines
Quoted 3 lines
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.
Quoted 8 lines
- Charlie
Quoted 6 lines
See alsohttps://github.com/riscv-forks/electron/issues/4
Quoted 1 line
Quoted 1 line
- Charlie
Sincerely,
Levi
I accidentally introduced some HTML into this reply so this reply is resent as plain text.
Sincerely,
Levi

Yangyu Chen

2024-08-23 06:55 UTC #17 lore raw

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

Quoted 324 lines · Charlie Jenkins

Quoted 313 lines · Yangyu Chen
Quoted 280 lines · Charlie Jenkins
Quoted 275 lines · Yangyu Chen
Quoted 257 lines · Palmer Dabbelt
Quoted 244 lines · On Mon, 19 Aug 2024 18:58:18 PDT (-0700), [email protected] wrote:
Quoted 161 lines · Charlie Jenkins
Quoted 160 lines · Levi Zim
Quoted 146 lines · Palmer Dabbelt
Quoted 137 lines · Charlie Jenkins
Quoted 136 lines · Yangyu Chen
Quoted 6 lines · Yangyu Chen
Quoted 5 lines · Charlie Jenkins
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
Quoted 83 lines
Quoted 83 lines
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<[email protected]>
---
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; \
})
Quoted 7 lines
Quoted 7 lines
+#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
Quoted 22 lines
needs
to be solved is that mmap with the same hint address without MAP_FIXED
set will fail the second time.
Quoted 2 lines
Userspace code to reproduce the bug:
#include <sys/mman.h>
#include <stdio.h>
#include <stdint.h>
Quoted 1 line
void test(char *addr) {
char *res = mmap(addr, 4096, PROT_READ | PROT_WRITE,
MAP_ANONYMOUS
Quoted 1 line
MAP_PRIVATE, -1, 0);
printf("hint %p got %p.\n", addr, res);
}
Quoted 1 line
int main (void) {
test(1<<30);
test(1<<30);
test(1<<30);
return 0;
}
Quoted 2 lines
output:
hint 0x40000000 got 0x40000000.
Quoted 9 lines
hint 0x40000000 got 0xffffffffffffffff.
hint 0x40000000 got 0xffffffffffffffff.
Quoted 2 lines
output on x86:
hint 0x40000000 got 0x40000000.
hint 0x40000000 got 0x7f9171363000.
hint 0x40000000 got 0x7f9171362000.
Quoted 1 line
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.
Quoted 5 lines
Quoted 4 lines
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.
Quoted 7 lines
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>
Quoted 8 lines
Quoted 5 lines
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.
Quoted 24 lines
Quoted 5 lines
      printf("Not expected address: %p != %p\n", addr, expected);
}
expected = (void*)0x3fffff000;
addr = mmap(expected, 17179873280, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS,
-1, 0);
Quoted 9 lines
  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.
Quoted 4 lines
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:
Quoted 3 lines
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:
Quoted 8 lines
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.
Quoted 6 lines
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.
Quoted 7 lines
Quoted 2 lines
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".
Quoted 7 lines
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.
Quoted 5 lines
Quoted 2 lines
I think we can just fix like that patch:
https://lore.kernel.org/lkml/[email protected]/
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.
Quoted 6 lines
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).
Quoted 5 lines
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.
Quoted 5 lines
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.
Quoted 5 lines
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.

Quoted 8 lines

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.

Quoted 40 lines

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.
Quoted 32 lines
Quoted 30 lines
- Charlie
Quoted 27 lines
Quoted 27 lines
Charlie's going to try and throw together a patch for that, hopefully it'll sort things out.
Quoted 24 lines
Quoted 3 lines
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.
Quoted 8 lines
- Charlie
Quoted 6 lines
See alsohttps://github.com/riscv-forks/electron/issues/4
Quoted 1 line
Quoted 1 line
- Charlie
Sincerely,
Levi
I accidentally introduced some HTML into this reply so this reply is resent as plain text.
Sincerely,
Levi

Charlie Jenkins

author

2024-08-26 16:30 UTC #18 lore raw

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

Quoted 330 lines · Yangyu Chen

Quoted 330 lines · Charlie Jenkins
Quoted 313 lines · Yangyu Chen
Quoted 280 lines · Charlie Jenkins
Quoted 275 lines · Yangyu Chen
Quoted 257 lines · Palmer Dabbelt
Quoted 244 lines · On Mon, 19 Aug 2024 18:58:18 PDT (-0700), [email protected] wrote:
Quoted 161 lines · Charlie Jenkins
Quoted 160 lines · Levi Zim
Quoted 146 lines · Palmer Dabbelt
Quoted 137 lines · Charlie Jenkins
Quoted 136 lines · Yangyu Chen
Quoted 6 lines · Yangyu Chen
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
Quoted 90 lines
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<[email protected]>
---
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
Quoted 31 lines
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.
Quoted 5 lines
Quoted 4 lines
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.
Quoted 7 lines
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>
Quoted 8 lines
Quoted 5 lines
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.
Quoted 24 lines
Quoted 5 lines
      printf("Not expected address: %p != %p\n", addr, expected);
}
expected = (void*)0x3fffff000;
addr = mmap(expected, 17179873280, PROT_NONE, MAP_PRIVATE|MAP_ANONYMOUS,
-1, 0);
Quoted 9 lines
  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.
Quoted 4 lines
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:
Quoted 3 lines
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:
Quoted 8 lines
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.
Quoted 6 lines
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.
Quoted 7 lines
Quoted 2 lines
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".
Quoted 7 lines
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.
Quoted 5 lines
Quoted 2 lines
I think we can just fix like that patch:
https://lore.kernel.org/lkml/[email protected]/
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.
Quoted 6 lines
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).
Quoted 5 lines
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.
Quoted 5 lines
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.
Quoted 5 lines
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:

Quoted 7 lines

Quoted 2 lines
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.

Yes that is true. However that was an existing problem before any of these patches went in. I think the best solution is to have an explicit flag as you have suggested with similar behavor to MAP_32BIT to solve this.

- Charlie

Quoted 42 lines

Quoted 41 lines
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.
Quoted 32 lines
Quoted 30 lines
- Charlie
Quoted 27 lines
Quoted 27 lines
Charlie's going to try and throw together a patch for that, hopefully it'll sort things out.
Quoted 24 lines
Quoted 3 lines
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.
Quoted 8 lines
- Charlie
Quoted 6 lines
See alsohttps://github.com/riscv-forks/electron/issues/4
Quoted 1 line
Quoted 1 line
- Charlie
Sincerely,
Levi
I accidentally introduced some HTML into this reply so this reply is resent as plain text.
Sincerely,
Levi

Charlie Jenkins

author

2024-01-31 01:07 UTC #19 lore raw

PATCH v3 2/3 selftests: riscv: Generalize mm selftests

In reply to Charlie Jenkins

The behavior of mmap on riscv is defined to not provide an address that uses more bits than the hint address, if provided. Make the tests reflect that.

Signed-off-by Charlie Jenkins <[email protected]>

---
 tools/testing/selftests/riscv/mm/mmap_bottomup.c |  23 +----
 tools/testing/selftests/riscv/mm/mmap_default.c  |  23 +----
 tools/testing/selftests/riscv/mm/mmap_test.h     | 107 ++++++++++++++---------
 3 files changed, 67 insertions(+), 86 deletions(-)

tools/testing/selftests/riscv/mm/mmap_bottomup.c +1 ~~-22~~

index 1757d19ca89b..7f7d3eb8b9c9 100644
@@ -6,30 +6,9 @@
 
 TEST(infinite_rlimit)
 {
-// Only works on 64 bit
-#if __riscv_xlen == 64
-	struct addresses mmap_addresses;
-
 	EXPECT_EQ(BOTTOM_UP, memory_layout());
 
-	do_mmaps(&mmap_addresses);
-
-	EXPECT_NE(MAP_FAILED, mmap_addresses.no_hint);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_37_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_38_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_46_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_47_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_55_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_56_addr);
-
-	EXPECT_GT(1UL << 47, (unsigned long)mmap_addresses.no_hint);
-	EXPECT_GT(1UL << 38, (unsigned long)mmap_addresses.on_37_addr);
-	EXPECT_GT(1UL << 38, (unsigned long)mmap_addresses.on_38_addr);
-	EXPECT_GT(1UL << 38, (unsigned long)mmap_addresses.on_46_addr);
-	EXPECT_GT(1UL << 47, (unsigned long)mmap_addresses.on_47_addr);
-	EXPECT_GT(1UL << 47, (unsigned long)mmap_addresses.on_55_addr);
-	EXPECT_GT(1UL << 56, (unsigned long)mmap_addresses.on_56_addr);
-#endif
+	TEST_MMAPS;
 }
 
 TEST_HARNESS_MAIN

tools/testing/selftests/riscv/mm/mmap_default.c +1 ~~-22~~

index c63c60b9397e..2ba3ec990006 100644
@@ -6,30 +6,9 @@
 
 TEST(default_rlimit)
 {
-// Only works on 64 bit
-#if __riscv_xlen == 64
-	struct addresses mmap_addresses;
-
 	EXPECT_EQ(TOP_DOWN, memory_layout());
 
-	do_mmaps(&mmap_addresses);
-
-	EXPECT_NE(MAP_FAILED, mmap_addresses.no_hint);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_37_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_38_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_46_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_47_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_55_addr);
-	EXPECT_NE(MAP_FAILED, mmap_addresses.on_56_addr);
-
-	EXPECT_GT(1UL << 47, (unsigned long)mmap_addresses.no_hint);
-	EXPECT_GT(1UL << 38, (unsigned long)mmap_addresses.on_37_addr);
-	EXPECT_GT(1UL << 38, (unsigned long)mmap_addresses.on_38_addr);
-	EXPECT_GT(1UL << 38, (unsigned long)mmap_addresses.on_46_addr);
-	EXPECT_GT(1UL << 47, (unsigned long)mmap_addresses.on_47_addr);
-	EXPECT_GT(1UL << 47, (unsigned long)mmap_addresses.on_55_addr);
-	EXPECT_GT(1UL << 56, (unsigned long)mmap_addresses.on_56_addr);
-#endif
+	TEST_MMAPS;
 }
 
 TEST_HARNESS_MAIN

tools/testing/selftests/riscv/mm/mmap_test.h +65 ~~-42~~

125 lines not on this page · raw message

Signature · 1 line

2.43.0

Charlie Jenkins

author

2024-01-31 01:07 UTC #20 lore raw

PATCH v3 3/3 docs: riscv: Define behavior of mmap

In reply to Charlie Jenkins

Define mmap on riscv to not provide an address that uses more bits than the hint address, if provided.

Signed-off-by Charlie Jenkins <[email protected]>

---
 Documentation/arch/riscv/vm-layout.rst | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

Documentation/arch/riscv/vm-layout.rst +5 ~~-11~~

index 69ff6da1dbf8..e476b4386bd9 100644
@@ -144,14 +144,8 @@ passing 0 into the hint address parameter of mmap. On CPUs with an address space
 smaller than sv48, the CPU maximum supported address space will be the default.
 
 Software can "opt-in" to receiving VAs from another VA space by providing
-a hint address to mmap. A hint address passed to mmap will cause the largest
-address space that fits entirely into the hint to be used, unless there is no
-space left in the address space. If there is no space available in the requested
-address space, an address in the next smallest available address space will be
-returned.
-
-For example, in order to obtain 48-bit VA space, a hint address greater than
-:code:`1 << 47` must be provided. Note that this is 47 due to sv48 userspace
-ending at :code:`1 << 47` and the addresses beyond this are reserved for the
-kernel. Similarly, to obtain 57-bit VA space addresses, a hint address greater
-than or equal to :code:`1 << 56` must be provided.
+a hint address to mmap. When a hint address is passed to mmap, the returned
+address will never use more bits than the hint address. For example, if a hint
+address of `1 << 40` is passed to mmap, a valid returned address will never use
+bits 41 through 63. If no mappable addresses are available in that range, mmap
+will return `MAP_FAILED`.

Signature · 1 line

2.43.0

[email protected]

CI Applied

2024-03-20 20:50 UTC #21 lore raw

In reply to Charlie Jenkins

This series was applied to riscv/linux.git (for-next) by Palmer Dabbelt <[email protected]>:

Hello:

This series was applied to riscv/linux.git (for-next) by Palmer Dabbelt <[email protected]>:

Quoted 9 lines · On Tue, 30 Jan 2024 17:06:59 -0800 you wrote:

On riscv, mmap currently returns an address from the largest address space that can fit entirely inside of the hint address. This makes it such that the hint address is almost never returned. This patch raises the mappable area up to and including the hint address. This allows mmap to often return the hint address, which allows a performance improvement over searching for a valid address as well as making the behavior more similar to other architectures.
[...]

Here is the summary with links:
- [v3,1/3] riscv: mm: Use hint address in mmap if available

    https://git.kernel.org/riscv/c/b5b4287accd7
  - [v3,2/3] selftests: riscv: Generalize mm selftests
    https://git.kernel.org/riscv/c/73d05262a2ca
  - [v3,3/3] docs: riscv: Define behavior of mmap
    https://git.kernel.org/riscv/c/371a3c2055db

You are awesome, thank you!

Signature · 2 lines

Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html