I just saw the opportunity of optimizing the helper is_compat_task() by
introducing a compile-time test, and it made possible to remove some
#ifdef's without any loss of performance.
I also saw the possibility of removing the direct check of task flags from
general code, and concentrated it in asm/compat.h by creating a few more
helpers, which in the end helped optimize code.
arch_get_mmap_end() just got a simple improvement and some extra docs.
Changes since RFC:
- Fused with other patchset: Improve arch_get_mmap_end() macro
- Renamed from "Introduce & Optimize compat-mode helpers"
Leonardo Bras (5):
riscv: Improve arch_get_mmap_end() macro
riscv: Replace direct thread flag check with is_compat_task()
riscv: add compile-time test into is_compat_task()
riscv: Introduce is_compat_thread() into compat.h
riscv: Introduce set_compat_task() in asm/compat.h
arch/riscv/include/asm/compat.h | 19 +++++++++++++++++++
arch/riscv/include/asm/elf.h | 11 ++---------
arch/riscv/include/asm/pgtable.h | 8 +-------
arch/riscv/include/asm/processor.h | 16 +++++++++++-----
arch/riscv/kernel/ptrace.c | 6 +++---
5 files changed, 36 insertions(+), 24 deletions(-)
base-commit: 610a9b8f49fbcf1100716370d3b5f6f884a2835a
--
2.43.0
There is some code that detects compat mode into a task by checking the
flag directly, and other code that check using the helper is_compat_task().
Since the helper already exists, use it instead of checking the flags
directly.
Signed-off-by: Leonardo Bras <[email protected]>
---
arch/riscv/include/asm/elf.h | 2 +-
arch/riscv/include/asm/pgtable.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/riscv/include/asm/elf.h b/arch/riscv/include/asm/elf.h
index 06c236bfab53b..59a08367fddd7 100644
--- a/arch/riscv/include/asm/elf.h
+++ b/arch/riscv/include/asm/elf.h
@@ -54,7 +54,7 @@ extern bool compat_elf_check_arch(Elf32_Ehdr *hdr);
#ifdef CONFIG_64BIT
#ifdef CONFIG_COMPAT
-#define STACK_RND_MASK (test_thread_flag(TIF_32BIT) ? \
+#define STACK_RND_MASK (is_compat_task() ? \
0x7ff >> (PAGE_SHIFT - 12) : \
0x3ffff >> (PAGE_SHIFT - 12))
#else
diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
index ab00235b018f8..1d472b31e0cfe 100644
--- a/arch/riscv/include/asm/pgtable.h
+++ b/arch/riscv/include/asm/pgtable.h
@@ -882,7 +882,7 @@ static inline pte_t pte_swp_clear_exclusive(pte_t pte)
#ifdef CONFIG_COMPAT
#define TASK_SIZE_32 (_AC(0x80000000, UL) - PAGE_SIZE)
-#define TASK_SIZE (test_thread_flag(TIF_32BIT) ? \
+#define TASK_SIZE (is_compat_task() ? \
TASK_SIZE_32 : TASK_SIZE_64)
#else
#define TASK_SIZE TASK_SIZE_64
--
2.43.0
This macro caused me some confusion, which took some reviewer's time to
make it clear, so I propose adding a short comment in code to avoid
confusion in the future.
Also, added some improvements to the macro, such as removing the
assumption of VA_USER_SV57 being the largest address space.
Signed-off-by: Leonardo Bras <[email protected]>
Reviewed-by: Guo Ren <[email protected]>
---
arch/riscv/include/asm/processor.h | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
index f19f861cda549..2278e2a8362af 100644
--- a/arch/riscv/include/asm/processor.h
+++ b/arch/riscv/include/asm/processor.h
@@ -18,15 +18,21 @@
#define DEFAULT_MAP_WINDOW (UL(1) << (MMAP_VA_BITS - 1))
#define STACK_TOP_MAX TASK_SIZE_64
+/*
+ * addr is a hint to the maximum userspace address that mmap should provide, so
+ * this macro needs to return the largest address space available so that
+ * mmap_end < addr, being mmap_end the top of that address space.
+ * See Documentation/arch/riscv/vm-layout.rst for more details.
+ */
#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())) \
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)) \
+ else if (((_addr) >= VA_USER_SV57) && (VA_BITS >= VA_BITS_SV57)) \
+ mmap_end = VA_USER_SV57; \
+ else if (((_addr) >= VA_USER_SV48) && (VA_BITS >= VA_BITS_SV48)) \
mmap_end = VA_USER_SV48; \
else \
mmap_end = VA_USER_SV39; \
--
2.43.0
Currently several places will test for CONFIG_COMPAT before testing
is_compat_task(), probably in order to avoid a run-time test into the task
structure.
Since is_compat_task() is an inlined function, it would be helpful to add a
compile-time test of CONFIG_COMPAT, making sure it always returns zero when
the option is not enabled during the kernel build.
With this, the compiler is able to understand in build-time that
is_compat_task() will always return 0, and optimize-out some of the extra
code introduced by the option.
This will also allow removing a lot #ifdefs that were introduced, and make
the code more clean.
Signed-off-by: Leonardo Bras <[email protected]>
Reviewed-by: Guo Ren <[email protected]>
Reviewed-by: Andy Chiu <[email protected]>
---
arch/riscv/include/asm/compat.h | 3 +++
arch/riscv/include/asm/elf.h | 4 ----
arch/riscv/include/asm/pgtable.h | 6 ------
arch/riscv/include/asm/processor.h | 4 ++--
4 files changed, 5 insertions(+), 12 deletions(-)
diff --git a/arch/riscv/include/asm/compat.h b/arch/riscv/include/asm/compat.h
index 2ac955b51148f..91517b51b8e27 100644
--- a/arch/riscv/include/asm/compat.h
+++ b/arch/riscv/include/asm/compat.h
@@ -14,6 +14,9 @@
static inline int is_compat_task(void)
{
+ if (!IS_ENABLED(CONFIG_COMPAT))
+ return 0;
+
return test_thread_flag(TIF_32BIT);
}
diff --git a/arch/riscv/include/asm/elf.h b/arch/riscv/include/asm/elf.h
index 59a08367fddd7..2e88257cafaea 100644
--- a/arch/riscv/include/asm/elf.h
+++ b/arch/riscv/include/asm/elf.h
@@ -53,13 +53,9 @@ extern bool compat_elf_check_arch(Elf32_Ehdr *hdr);
#define ELF_ET_DYN_BASE ((DEFAULT_MAP_WINDOW / 3) * 2)
#ifdef CONFIG_64BIT
-#ifdef CONFIG_COMPAT
#define STACK_RND_MASK (is_compat_task() ? \
0x7ff >> (PAGE_SHIFT - 12) : \
0x3ffff >> (PAGE_SHIFT - 12))
-#else
-#define STACK_RND_MASK (0x3ffff >> (PAGE_SHIFT - 12))
-#endif
#endif
/*
diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
index 1d472b31e0cfe..ea5b269be223a 100644
--- a/arch/riscv/include/asm/pgtable.h
+++ b/arch/riscv/include/asm/pgtable.h
@@ -127,16 +127,10 @@
#define VA_USER_SV48 (UL(1) << (VA_BITS_SV48 - 1))
#define VA_USER_SV57 (UL(1) << (VA_BITS_SV57 - 1))
-#ifdef CONFIG_COMPAT
#define MMAP_VA_BITS_64 ((VA_BITS >= VA_BITS_SV48) ? VA_BITS_SV48 : VA_BITS)
#define MMAP_MIN_VA_BITS_64 (VA_BITS_SV39)
#define MMAP_VA_BITS (is_compat_task() ? VA_BITS_SV32 : MMAP_VA_BITS_64)
#define MMAP_MIN_VA_BITS (is_compat_task() ? VA_BITS_SV32 : MMAP_MIN_VA_BITS_64)
-#else
-#define MMAP_VA_BITS ((VA_BITS >= VA_BITS_SV48) ? VA_BITS_SV48 : VA_BITS)
-#define MMAP_MIN_VA_BITS (VA_BITS_SV39)
-#endif /* CONFIG_COMPAT */
-
#else
#include <asm/pgtable-32.h>
#endif /* CONFIG_64BIT */
diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
index 2278e2a8362af..d2d7ce30baf3e 100644
--- a/arch/riscv/include/asm/processor.h
+++ b/arch/riscv/include/asm/processor.h
@@ -28,7 +28,7 @@
({ \
unsigned long mmap_end; \
typeof(addr) _addr = (addr); \
- if ((_addr) == 0 || (IS_ENABLED(CONFIG_COMPAT) && is_compat_task())) \
+ if ((_addr) == 0 || is_compat_task()) \
mmap_end = STACK_TOP_MAX; \
else if (((_addr) >= VA_USER_SV57) && (VA_BITS >= VA_BITS_SV57)) \
mmap_end = VA_USER_SV57; \
@@ -45,7 +45,7 @@
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_compat_task()) \
mmap_base = (_base); \
else if (((_addr) >= VA_USER_SV57) && (VA_BITS >= VA_BITS_SV57)) \
mmap_base = VA_USER_SV57 - rnd_gap; \
--
2.43.0
task_user_regset_view() makes use of a function very similar to
is_compat_task(), but pointing to a any thread.
In arm64 asm/compat.h there is a function very similar to that:
is_compat_thread(struct thread_info *thread)
Copy this function to riscv asm/compat.h and make use of it into
task_user_regset_view().
Also, introduce a compile-time test for CONFIG_COMPAT and simplify the
function code by removing the #ifdef.
Signed-off-by: Leonardo Bras <[email protected]>
Reviewed-by: Guo Ren <[email protected]>
Reviewed-by: Andy Chiu <[email protected]>
---
arch/riscv/include/asm/compat.h | 8 ++++++++
arch/riscv/kernel/ptrace.c | 6 +++---
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/arch/riscv/include/asm/compat.h b/arch/riscv/include/asm/compat.h
index 91517b51b8e27..da4b28cd01a95 100644
--- a/arch/riscv/include/asm/compat.h
+++ b/arch/riscv/include/asm/compat.h
@@ -20,6 +20,14 @@ static inline int is_compat_task(void)
return test_thread_flag(TIF_32BIT);
}
+static inline int is_compat_thread(struct thread_info *thread)
+{
+ if (!IS_ENABLED(CONFIG_COMPAT))
+ return 0;
+
+ return test_ti_thread_flag(thread, TIF_32BIT);
+}
+
struct compat_user_regs_struct {
compat_ulong_t pc;
compat_ulong_t ra;
diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
index 2afe460de16a6..f362832123616 100644
--- a/arch/riscv/kernel/ptrace.c
+++ b/arch/riscv/kernel/ptrace.c
@@ -374,14 +374,14 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
return ret;
}
+#else
+static const struct user_regset_view compat_riscv_user_native_view = {};
#endif /* CONFIG_COMPAT */
const struct user_regset_view *task_user_regset_view(struct task_struct *task)
{
-#ifdef CONFIG_COMPAT
- if (test_tsk_thread_flag(task, TIF_32BIT))
+ if (is_compat_thread(&task->thread_info))
return &compat_riscv_user_native_view;
else
-#endif
return &riscv_user_native_view;
}
--
2.43.0
In order to have all task compat bit access directly in compat.h, introduce
set_compat_task() to set/reset those when needed.
Also, since it's only used on an if/else scenario, simplify the macro using
it.
Signed-off-by: Leonardo Bras <[email protected]>
Reviewed-by: Guo Ren <[email protected]>
---
arch/riscv/include/asm/compat.h | 8 ++++++++
arch/riscv/include/asm/elf.h | 5 +----
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/arch/riscv/include/asm/compat.h b/arch/riscv/include/asm/compat.h
index da4b28cd01a95..aa103530a5c83 100644
--- a/arch/riscv/include/asm/compat.h
+++ b/arch/riscv/include/asm/compat.h
@@ -28,6 +28,14 @@ static inline int is_compat_thread(struct thread_info *thread)
return test_ti_thread_flag(thread, TIF_32BIT);
}
+static inline void set_compat_task(bool is_compat)
+{
+ if (is_compat)
+ set_thread_flag(TIF_32BIT);
+ else
+ clear_thread_flag(TIF_32BIT);
+}
+
struct compat_user_regs_struct {
compat_ulong_t pc;
compat_ulong_t ra;
diff --git a/arch/riscv/include/asm/elf.h b/arch/riscv/include/asm/elf.h
index 2e88257cafaea..c7aea7886d22a 100644
--- a/arch/riscv/include/asm/elf.h
+++ b/arch/riscv/include/asm/elf.h
@@ -135,10 +135,7 @@ do { \
#ifdef CONFIG_COMPAT
#define SET_PERSONALITY(ex) \
-do { if ((ex).e_ident[EI_CLASS] == ELFCLASS32) \
- set_thread_flag(TIF_32BIT); \
- else \
- clear_thread_flag(TIF_32BIT); \
+do { set_compat_task((ex).e_ident[EI_CLASS] == ELFCLASS32); \
if (personality(current->personality) != PER_LINUX32) \
set_personality(PER_LINUX | \
(current->personality & (~PER_MASK))); \
--
2.43.0
Hello:
This series was applied to riscv/linux.git (for-next)
by Palmer Dabbelt <[email protected]>:
On Wed, 3 Jan 2024 13:00:18 -0300 you wrote:
> I just saw the opportunity of optimizing the helper is_compat_task() by
> introducing a compile-time test, and it made possible to remove some
> #ifdef's without any loss of performance.
>
> I also saw the possibility of removing the direct check of task flags from
> general code, and concentrated it in asm/compat.h by creating a few more
> helpers, which in the end helped optimize code.
>
> [...]
Here is the summary with links:
- [v1,1/5] riscv: Improve arch_get_mmap_end() macro
https://git.kernel.org/riscv/c/6be7ee4bebd1
- [v1,2/5] riscv: Replace direct thread flag check with is_compat_task()
https://git.kernel.org/riscv/c/9dc30419248f
- [v1,3/5] riscv: add compile-time test into is_compat_task()
https://git.kernel.org/riscv/c/4c0b5a451675
- [v1,4/5] riscv: Introduce is_compat_thread() into compat.h
https://git.kernel.org/riscv/c/5917ea17ad07
- [v1,5/5] riscv: Introduce set_compat_task() in asm/compat.h
https://git.kernel.org/riscv/c/2a8986fc5e1c
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html