This patch enables support for DYNAMIC_FTRACE_WITH_CALL_OPS on RISC-V.
This allows each ftrace callsite to provide an ftrace_ops to the common
ftrace trampoline, allowing each callsite to invoke distinct tracer
functions without the need to fall back to list processing or to
allocate custom trampolines for each callsite. This significantly speeds
up cases where multiple distinct trace functions are used and callsites
are mostly traced by a single tracer.
The idea and most of the implementation is taken from the ARM64's
implementation of the same feature. The idea is to place a pointer to
the ftrace_ops as a literal at a fixed offset from the function entry
point, which can be recovered by the common ftrace trampoline.
We use -fpatchable-function-entry to reserve 8 bytes above the function
entry by emitting 2 4 byte or 4 2 byte nops depending on the presence of
CONFIG_RISCV_ISA_C. These 8 bytes are patched at runtime with a pointer
to the associated ftrace_ops for that callsite. Functions are aligned to
8 bytes to make sure that the accesses to this literal are atomic.
This approach allows for directly invoking ftrace_ops::func even for
ftrace_ops which are dynamically-allocated (or part of a module),
without going via ftrace_ops_list_func.
I've benchamrked this with the ftrace_ops sample module on Qemu, with
the next version, I will provide benchmarks on real hardware:
Without this patch:
+-----------------------+-----------------+----------------------------+
| Number of tracers | Total time (ns) | Per-call average time |
|-----------------------+-----------------+----------------------------|
| Relevant | Irrelevant | 100000 calls | Total (ns) | Overhead (ns) |
|----------+------------+-----------------+------------+---------------|
| 0 | 0 | 15615700 | 156 | - |
| 0 | 1 | 15917600 | 159 | - |
| 0 | 2 | 15668000 | 156 | - |
| 0 | 10 | 14971500 | 149 | - |
| 0 | 100 | 15417600 | 154 | - |
| 0 | 200 | 15387000 | 153 | - |
|----------+------------+-----------------+------------+---------------|
| 1 | 0 | 119906800 | 1199 | 1043 |
| 1 | 1 | 137428600 | 1374 | 1218 |
| 1 | 2 | 159562400 | 1374 | 1218 |
| 1 | 10 | 302099900 | 3020 | 2864 |
| 1 | 100 | 2008785500 | 20087 | 19931 |
| 1 | 200 | 3965221900 | 39652 | 39496 |
|----------+------------+-----------------+------------+---------------|
| 1 | 0 | 119166700 | 1191 | 1035 |
| 2 | 0 | 157999900 | 1579 | 1423 |
| 10 | 0 | 425370100 | 4253 | 4097 |
| 100 | 0 | 3595252100 | 35952 | 35796 |
| 200 | 0 | 7023485700 | 70234 | 70078 |
+----------+------------+-----------------+------------+---------------+
Note: per-call overhead is estimated relative to the baseline case with
0 relevant tracers and 0 irrelevant tracers.
With this patch:
+-----------------------+-----------------+----------------------------+
| Number of tracers | Total time (ns) | Per-call average time |
|-----------------------+-----------------+----------------------------|
| Relevant | Irrelevant | 100000 calls | Total (ns) | Overhead (ns) |
|----------+------------+-----------------+------------+---------------|
| 0 | 0 | 15254600 | 152 | - |
| 0 | 1 | 16136700 | 161 | - |
| 0 | 2 | 15329500 | 153 | - |
| 0 | 10 | 15148800 | 151 | - |
| 0 | 100 | 15746900 | 157 | - |
| 0 | 200 | 15737400 | 157 | - |
|----------+------------+-----------------+------------+---------------|
| 1 | 0 | 47909000 | 479 | 327 |
| 1 | 1 | 48297400 | 482 | 330 |
| 1 | 2 | 47314100 | 473 | 321 |
| 1 | 10 | 47844900 | 478 | 326 |
| 1 | 100 | 46591900 | 465 | 313 |
| 1 | 200 | 47178900 | 471 | 319 |
|----------+------------+-----------------+------------+---------------|
| 1 | 0 | 46715800 | 467 | 315 |
| 2 | 0 | 155134500 | 1551 | 1399 |
| 10 | 0 | 442672800 | 4426 | 4274 |
| 100 | 0 | 4092353900 | 40923 | 40771 |
| 200 | 0 | 7135796400 | 71357 | 71205 |
+----------+------------+-----------------+------------+---------------+
Note: per-call overhead is estimated relative to the baseline case with
0 relevant tracers and 0 irrelevant tracers.
As can be seen from the above:
a) Whenever there is a single relevant tracer function associated with a
tracee, the overhead of invoking the tracer is constant, and does not
scale with the number of tracers which are *not* associated with that
tracee.
b) The overhead for a single relevant tracer has dropped to ~1/3 of the
overhead prior to this series (from 1035ns to 315ns). This is largely
due to permitting calls to dynamically-allocated ftrace_ops without
going through ftrace_ops_list_func.
Why is this patch a RFC patch:
1. I saw some rcu stalls on Qemu and need to debug them and see if they
were introduced by this patch.
2. This needs to be tested thoroughly on real hardware.
3. Seeking reviews to fix any fundamental problems with this patch that I
might have missed due to my lack of RISC-V architecture knowledge.
4. I would like to benchmark this on real hardware and put the results in
the commit message.
Signed-off-by: Puranjay Mohan <[email protected]>
---
arch/riscv/Kconfig | 2 ++
arch/riscv/Makefile | 8 +++++
arch/riscv/include/asm/ftrace.h | 3 ++
arch/riscv/kernel/asm-offsets.c | 3 ++
arch/riscv/kernel/ftrace.c | 59 +++++++++++++++++++++++++++++++++
arch/riscv/kernel/mcount-dyn.S | 42 ++++++++++++++++++++---
6 files changed, 112 insertions(+), 5 deletions(-)
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 0bfcfec67ed5..e474742e23b2 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -78,6 +78,7 @@ config RISCV
select EDAC_SUPPORT
select FRAME_POINTER if PERF_EVENTS || (FUNCTION_TRACER && !DYNAMIC_FTRACE)
select FTRACE_MCOUNT_USE_PATCHABLE_FUNCTION_ENTRY if DYNAMIC_FTRACE
+ select FUNCTION_ALIGNMENT_8B if DYNAMIC_FTRACE_WITH_CALL_OPS
select GENERIC_ARCH_TOPOLOGY
select GENERIC_ATOMIC64 if !64BIT
select GENERIC_CLOCKEVENTS_BROADCAST if SMP
@@ -127,6 +128,7 @@ config RISCV
select HAVE_DYNAMIC_FTRACE if !XIP_KERNEL && MMU && (CLANG_SUPPORTS_DYNAMIC_FTRACE || GCC_SUPPORTS_DYNAMIC_FTRACE)
select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE
+ select HAVE_DYNAMIC_FTRACE_WITH_CALL_OPS if (DYNAMIC_FTRACE_WITH_REGS && !CFI_CLANG)
select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
select HAVE_FUNCTION_GRAPH_TRACER
select HAVE_FUNCTION_GRAPH_RETVAL if HAVE_FUNCTION_GRAPH_TRACER
diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
index 252d63942f34..875ad5dc3d32 100644
--- a/arch/riscv/Makefile
+++ b/arch/riscv/Makefile
@@ -14,12 +14,20 @@ endif
ifeq ($(CONFIG_DYNAMIC_FTRACE),y)
LDFLAGS_vmlinux += --no-relax
KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY
+ifeq ($(CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS), y)
+ifeq ($(CONFIG_RISCV_ISA_C),y)
+ CC_FLAGS_FTRACE := -fpatchable-function-entry=8,4
+else
+ CC_FLAGS_FTRACE := -fpatchable-function-entry=4,2
+endif
+else
ifeq ($(CONFIG_RISCV_ISA_C),y)
CC_FLAGS_FTRACE := -fpatchable-function-entry=4
else
CC_FLAGS_FTRACE := -fpatchable-function-entry=2
endif
endif
+endif
ifeq ($(CONFIG_CMODEL_MEDLOW),y)
KBUILD_CFLAGS_MODULE += -mcmodel=medany
diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
index 329172122952..c9a84222c9ea 100644
--- a/arch/riscv/include/asm/ftrace.h
+++ b/arch/riscv/include/asm/ftrace.h
@@ -28,6 +28,9 @@
void MCOUNT_NAME(void);
static inline unsigned long ftrace_call_adjust(unsigned long addr)
{
+ if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS))
+ return addr + 8;
+
return addr;
}
diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c
index a03129f40c46..7d7c4b486852 100644
--- a/arch/riscv/kernel/asm-offsets.c
+++ b/arch/riscv/kernel/asm-offsets.c
@@ -488,4 +488,7 @@ void asm_offsets(void)
DEFINE(STACKFRAME_SIZE_ON_STACK, ALIGN(sizeof(struct stackframe), STACK_ALIGN));
OFFSET(STACKFRAME_FP, stackframe, fp);
OFFSET(STACKFRAME_RA, stackframe, ra);
+#ifdef CONFIG_FUNCTION_TRACER
+ DEFINE(FTRACE_OPS_FUNC, offsetof(struct ftrace_ops, func));
+#endif
}
diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
index f5aa24d9e1c1..e2e75e15d32e 100644
--- a/arch/riscv/kernel/ftrace.c
+++ b/arch/riscv/kernel/ftrace.c
@@ -82,9 +82,52 @@ static int __ftrace_modify_call(unsigned long hook_pos, unsigned long target,
return 0;
}
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
+static const struct ftrace_ops *riscv64_rec_get_ops(struct dyn_ftrace *rec)
+{
+ const struct ftrace_ops *ops = NULL;
+
+ if (rec->flags & FTRACE_FL_CALL_OPS_EN) {
+ ops = ftrace_find_unique_ops(rec);
+ WARN_ON_ONCE(!ops);
+ }
+
+ if (!ops)
+ ops = &ftrace_list_ops;
+
+ return ops;
+}
+
+static int ftrace_rec_set_ops(const struct dyn_ftrace *rec,
+ const struct ftrace_ops *ops)
+{
+ unsigned long literal = rec->ip - 8;
+
+ return patch_text_nosync((void *)literal, &ops, sizeof(ops));
+}
+
+static int ftrace_rec_set_nop_ops(struct dyn_ftrace *rec)
+{
+ return ftrace_rec_set_ops(rec, &ftrace_nop_ops);
+}
+
+static int ftrace_rec_update_ops(struct dyn_ftrace *rec)
+{
+ return ftrace_rec_set_ops(rec, riscv64_rec_get_ops(rec));
+}
+#else
+static int ftrace_rec_set_nop_ops(struct dyn_ftrace *rec) { return 0; }
+static int ftrace_rec_update_ops(struct dyn_ftrace *rec) { return 0; }
+#endif
+
int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
{
unsigned int call[2];
+ int ret;
+
+ ret = ftrace_rec_update_ops(rec);
+ if (ret)
+ return ret;
make_call_t0(rec->ip, addr, call);
@@ -98,6 +141,11 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
unsigned long addr)
{
unsigned int nops[2] = {NOP4, NOP4};
+ int ret;
+
+ ret = ftrace_rec_set_nop_ops(rec);
+ if (ret)
+ return ret;
if (patch_text_nosync((void *)rec->ip, nops, MCOUNT_INSN_SIZE))
return -EPERM;
@@ -125,6 +173,13 @@ int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
int ftrace_update_ftrace_func(ftrace_func_t func)
{
+ /*
+ * When using CALL_OPS, the function to call is associated with the
+ * call site, and we don't have a global function pointer to update.
+ */
+ if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS))
+ return 0;
+
int ret = __ftrace_modify_call((unsigned long)&ftrace_call,
(unsigned long)func, true, true);
if (!ret) {
@@ -147,6 +202,10 @@ int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
make_call_t0(caller, old_addr, call);
ret = ftrace_check_current_call(caller, call);
+ if (ret)
+ return ret;
+
+ ret = ftrace_rec_update_ops(rec);
if (ret)
return ret;
diff --git a/arch/riscv/kernel/mcount-dyn.S b/arch/riscv/kernel/mcount-dyn.S
index b7561288e8da..cb241e36e514 100644
--- a/arch/riscv/kernel/mcount-dyn.S
+++ b/arch/riscv/kernel/mcount-dyn.S
@@ -191,11 +191,35 @@
.endm
.macro PREPARE_ARGS
- addi a0, t0, -FENTRY_RA_OFFSET
+ addi a0, t0, -FENTRY_RA_OFFSET // ip (callsite's auipc insn)
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
+ /*
+ * When CALL_OPS is enabled (2 or 4) nops [8B] are placed before the
+ * function entry, these are later overwritten with the pointer to the
+ * associated struct ftrace_ops.
+ *
+ * -8: &ftrace_ops of the associated tracer function.
+ *<ftrace enable>:
+ * 0: auipc t0/ra, 0x?
+ * 4: jalr t0/ra, ?(t0/ra)
+ *
+ * -8: &ftrace_nop_ops
+ *<ftrace disable>:
+ * 0: nop
+ * 4: nop
+ *
+ * t0 is set to ip+8 after the jalr is executed at the callsite,
+ * so we find the associated op at t0-16.
+ */
+ mv a1, ra // parent_ip
+ REG_L a2, -16(t0) // op
+ REG_L ra, FTRACE_OPS_FUNC(a2) // op->func
+#else
la a1, function_trace_op
- REG_L a2, 0(a1)
- mv a1, ra
- mv a3, sp
+ REG_L a2, 0(a1) // op
+ mv a1, ra // parent_ip
+#endif
+ mv a3, sp // regs
.endm
#endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
@@ -233,8 +257,12 @@ SYM_FUNC_START(ftrace_regs_caller)
SAVE_ABI_REGS 1
PREPARE_ARGS
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
+ jalr ra
+#else
SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
call ftrace_stub
+#endif
RESTORE_ABI_REGS 1
bnez t1, .Ldirect
@@ -247,9 +275,13 @@ SYM_FUNC_START(ftrace_caller)
SAVE_ABI_REGS 0
PREPARE_ARGS
-SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
+ jalr ra
+#else
+SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
call ftrace_stub
+#endif
RESTORE_ABI_REGS 0
jr t0
SYM_FUNC_END(ftrace_caller)
--
2.40.1
Hi Puranjay,
On 06/03/2024 17:59, Puranjay Mohan wrote:
> This patch enables support for DYNAMIC_FTRACE_WITH_CALL_OPS on RISC-V.
> This allows each ftrace callsite to provide an ftrace_ops to the common
> ftrace trampoline, allowing each callsite to invoke distinct tracer
> functions without the need to fall back to list processing or to
> allocate custom trampolines for each callsite. This significantly speeds
> up cases where multiple distinct trace functions are used and callsites
> are mostly traced by a single tracer.
>
> The idea and most of the implementation is taken from the ARM64's
> implementation of the same feature. The idea is to place a pointer to
> the ftrace_ops as a literal at a fixed offset from the function entry
> point, which can be recovered by the common ftrace trampoline.
>
> We use -fpatchable-function-entry to reserve 8 bytes above the function
> entry by emitting 2 4 byte or 4 2 byte nops depending on the presence of
> CONFIG_RISCV_ISA_C. These 8 bytes are patched at runtime with a pointer
> to the associated ftrace_ops for that callsite. Functions are aligned to
> 8 bytes to make sure that the accesses to this literal are atomic.
>
> This approach allows for directly invoking ftrace_ops::func even for
> ftrace_ops which are dynamically-allocated (or part of a module),
> without going via ftrace_ops_list_func.
>
> I've benchamrked this with the ftrace_ops sample module on Qemu, with
> the next version, I will provide benchmarks on real hardware:
>
> Without this patch:
>
> +-----------------------+-----------------+----------------------------+
> | Number of tracers | Total time (ns) | Per-call average time |
> |-----------------------+-----------------+----------------------------|
> | Relevant | Irrelevant | 100000 calls | Total (ns) | Overhead (ns) |
> |----------+------------+-----------------+------------+---------------|
> | 0 | 0 | 15615700 | 156 | - |
> | 0 | 1 | 15917600 | 159 | - |
> | 0 | 2 | 15668000 | 156 | - |
> | 0 | 10 | 14971500 | 149 | - |
> | 0 | 100 | 15417600 | 154 | - |
> | 0 | 200 | 15387000 | 153 | - |
> |----------+------------+-----------------+------------+---------------|
> | 1 | 0 | 119906800 | 1199 | 1043 |
> | 1 | 1 | 137428600 | 1374 | 1218 |
> | 1 | 2 | 159562400 | 1374 | 1218 |
> | 1 | 10 | 302099900 | 3020 | 2864 |
> | 1 | 100 | 2008785500 | 20087 | 19931 |
> | 1 | 200 | 3965221900 | 39652 | 39496 |
> |----------+------------+-----------------+------------+---------------|
> | 1 | 0 | 119166700 | 1191 | 1035 |
> | 2 | 0 | 157999900 | 1579 | 1423 |
> | 10 | 0 | 425370100 | 4253 | 4097 |
> | 100 | 0 | 3595252100 | 35952 | 35796 |
> | 200 | 0 | 7023485700 | 70234 | 70078 |
> +----------+------------+-----------------+------------+---------------+
>
> Note: per-call overhead is estimated relative to the baseline case with
> 0 relevant tracers and 0 irrelevant tracers.
>
> With this patch:
>
> +-----------------------+-----------------+----------------------------+
> | Number of tracers | Total time (ns) | Per-call average time |
> |-----------------------+-----------------+----------------------------|
> | Relevant | Irrelevant | 100000 calls | Total (ns) | Overhead (ns) |
> |----------+------------+-----------------+------------+---------------|
> | 0 | 0 | 15254600 | 152 | - |
> | 0 | 1 | 16136700 | 161 | - |
> | 0 | 2 | 15329500 | 153 | - |
> | 0 | 10 | 15148800 | 151 | - |
> | 0 | 100 | 15746900 | 157 | - |
> | 0 | 200 | 15737400 | 157 | - |
> |----------+------------+-----------------+------------+---------------|
> | 1 | 0 | 47909000 | 479 | 327 |
> | 1 | 1 | 48297400 | 482 | 330 |
> | 1 | 2 | 47314100 | 473 | 321 |
> | 1 | 10 | 47844900 | 478 | 326 |
> | 1 | 100 | 46591900 | 465 | 313 |
> | 1 | 200 | 47178900 | 471 | 319 |
> |----------+------------+-----------------+------------+---------------|
> | 1 | 0 | 46715800 | 467 | 315 |
> | 2 | 0 | 155134500 | 1551 | 1399 |
> | 10 | 0 | 442672800 | 4426 | 4274 |
> | 100 | 0 | 4092353900 | 40923 | 40771 |
> | 200 | 0 | 7135796400 | 71357 | 71205 |
> +----------+------------+-----------------+------------+---------------+
>
> Note: per-call overhead is estimated relative to the baseline case with
> 0 relevant tracers and 0 irrelevant tracers.
>
> As can be seen from the above:
>
> a) Whenever there is a single relevant tracer function associated with a
> tracee, the overhead of invoking the tracer is constant, and does not
> scale with the number of tracers which are *not* associated with that
> tracee.
>
> b) The overhead for a single relevant tracer has dropped to ~1/3 of the
> overhead prior to this series (from 1035ns to 315ns). This is largely
> due to permitting calls to dynamically-allocated ftrace_ops without
> going through ftrace_ops_list_func.
>
> Why is this patch a RFC patch:
> 1. I saw some rcu stalls on Qemu and need to debug them and see if they
> were introduced by this patch.
FYI, I'm currently working on debugging such issues (and other) with the
*current* ftrace implementation, so probably not caused by your
patchset. But keep debugging too, maybe this introduces other issues or
even better, you'll find the root cause :)
> 2. This needs to be tested thoroughly on real hardware.
> 3. Seeking reviews to fix any fundamental problems with this patch that I
> might have missed due to my lack of RISC-V architecture knowledge.
> 4. I would like to benchmark this on real hardware and put the results in
> the commit message.
>
> Signed-off-by: Puranjay Mohan <[email protected]>
> ---
> arch/riscv/Kconfig | 2 ++
> arch/riscv/Makefile | 8 +++++
> arch/riscv/include/asm/ftrace.h | 3 ++
> arch/riscv/kernel/asm-offsets.c | 3 ++
> arch/riscv/kernel/ftrace.c | 59 +++++++++++++++++++++++++++++++++
> arch/riscv/kernel/mcount-dyn.S | 42 ++++++++++++++++++++---
> 6 files changed, 112 insertions(+), 5 deletions(-)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 0bfcfec67ed5..e474742e23b2 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -78,6 +78,7 @@ config RISCV
> select EDAC_SUPPORT
> select FRAME_POINTER if PERF_EVENTS || (FUNCTION_TRACER && !DYNAMIC_FTRACE)
> select FTRACE_MCOUNT_USE_PATCHABLE_FUNCTION_ENTRY if DYNAMIC_FTRACE
> + select FUNCTION_ALIGNMENT_8B if DYNAMIC_FTRACE_WITH_CALL_OPS
A recent discussion [1] states that -falign-functions cannot guarantee
this alignment for all code and that gcc developers came up with a new
option [2]: WDYT? I have added Andy and Evgenii in +cc to help on that.
[1]
https://lore.kernel.org/linux-riscv/[email protected]/
[2]
https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=0f5a9a00e3ab1fe96142f304cfbcf3f63b15f326
> select GENERIC_ARCH_TOPOLOGY
> select GENERIC_ATOMIC64 if !64BIT
> select GENERIC_CLOCKEVENTS_BROADCAST if SMP
> @@ -127,6 +128,7 @@ config RISCV
> select HAVE_DYNAMIC_FTRACE if !XIP_KERNEL && MMU && (CLANG_SUPPORTS_DYNAMIC_FTRACE || GCC_SUPPORTS_DYNAMIC_FTRACE)
> select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE
> + select HAVE_DYNAMIC_FTRACE_WITH_CALL_OPS if (DYNAMIC_FTRACE_WITH_REGS && !CFI_CLANG)
> select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
> select HAVE_FUNCTION_GRAPH_TRACER
> select HAVE_FUNCTION_GRAPH_RETVAL if HAVE_FUNCTION_GRAPH_TRACER
> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> index 252d63942f34..875ad5dc3d32 100644
> --- a/arch/riscv/Makefile
> +++ b/arch/riscv/Makefile
> @@ -14,12 +14,20 @@ endif
> ifeq ($(CONFIG_DYNAMIC_FTRACE),y)
> LDFLAGS_vmlinux += --no-relax
> KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY
> +ifeq ($(CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS), y)
> +ifeq ($(CONFIG_RISCV_ISA_C),y)
> + CC_FLAGS_FTRACE := -fpatchable-function-entry=8,4
> +else
> + CC_FLAGS_FTRACE := -fpatchable-function-entry=4,2
> +endif
> +else
> ifeq ($(CONFIG_RISCV_ISA_C),y)
> CC_FLAGS_FTRACE := -fpatchable-function-entry=4
> else
> CC_FLAGS_FTRACE := -fpatchable-function-entry=2
> endif
> endif
> +endif
>
> ifeq ($(CONFIG_CMODEL_MEDLOW),y)
> KBUILD_CFLAGS_MODULE += -mcmodel=medany
> diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
> index 329172122952..c9a84222c9ea 100644
> --- a/arch/riscv/include/asm/ftrace.h
> +++ b/arch/riscv/include/asm/ftrace.h
> @@ -28,6 +28,9 @@
> void MCOUNT_NAME(void);
> static inline unsigned long ftrace_call_adjust(unsigned long addr)
> {
> + if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS))
> + return addr + 8;
> +
> return addr;
> }
>
> diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c
> index a03129f40c46..7d7c4b486852 100644
> --- a/arch/riscv/kernel/asm-offsets.c
> +++ b/arch/riscv/kernel/asm-offsets.c
> @@ -488,4 +488,7 @@ void asm_offsets(void)
> DEFINE(STACKFRAME_SIZE_ON_STACK, ALIGN(sizeof(struct stackframe), STACK_ALIGN));
> OFFSET(STACKFRAME_FP, stackframe, fp);
> OFFSET(STACKFRAME_RA, stackframe, ra);
> +#ifdef CONFIG_FUNCTION_TRACER
> + DEFINE(FTRACE_OPS_FUNC, offsetof(struct ftrace_ops, func));
> +#endif
> }
> diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
> index f5aa24d9e1c1..e2e75e15d32e 100644
> --- a/arch/riscv/kernel/ftrace.c
> +++ b/arch/riscv/kernel/ftrace.c
> @@ -82,9 +82,52 @@ static int __ftrace_modify_call(unsigned long hook_pos, unsigned long target,
> return 0;
> }
>
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
> +static const struct ftrace_ops *riscv64_rec_get_ops(struct dyn_ftrace *rec)
> +{
> + const struct ftrace_ops *ops = NULL;
> +
> + if (rec->flags & FTRACE_FL_CALL_OPS_EN) {
> + ops = ftrace_find_unique_ops(rec);
> + WARN_ON_ONCE(!ops);
> + }
> +
> + if (!ops)
> + ops = &ftrace_list_ops;
> +
> + return ops;
> +}
> +
> +static int ftrace_rec_set_ops(const struct dyn_ftrace *rec,
> + const struct ftrace_ops *ops)
> +{
> + unsigned long literal = rec->ip - 8;
> +
> + return patch_text_nosync((void *)literal, &ops, sizeof(ops));
> +}
> +
> +static int ftrace_rec_set_nop_ops(struct dyn_ftrace *rec)
> +{
> + return ftrace_rec_set_ops(rec, &ftrace_nop_ops);
> +}
> +
> +static int ftrace_rec_update_ops(struct dyn_ftrace *rec)
> +{
> + return ftrace_rec_set_ops(rec, riscv64_rec_get_ops(rec));
> +}
> +#else
> +static int ftrace_rec_set_nop_ops(struct dyn_ftrace *rec) { return 0; }
> +static int ftrace_rec_update_ops(struct dyn_ftrace *rec) { return 0; }
> +#endif
> +
> int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
> {
> unsigned int call[2];
> + int ret;
> +
> + ret = ftrace_rec_update_ops(rec);
> + if (ret)
> + return ret;
>
> make_call_t0(rec->ip, addr, call);
>
> @@ -98,6 +141,11 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
> unsigned long addr)
> {
> unsigned int nops[2] = {NOP4, NOP4};
> + int ret;
> +
> + ret = ftrace_rec_set_nop_ops(rec);
> + if (ret)
> + return ret;
>
> if (patch_text_nosync((void *)rec->ip, nops, MCOUNT_INSN_SIZE))
> return -EPERM;
> @@ -125,6 +173,13 @@ int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
>
> int ftrace_update_ftrace_func(ftrace_func_t func)
> {
> + /*
> + * When using CALL_OPS, the function to call is associated with the
> + * call site, and we don't have a global function pointer to update.
> + */
> + if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS))
> + return 0;
> +
> int ret = __ftrace_modify_call((unsigned long)&ftrace_call,
> (unsigned long)func, true, true);
> if (!ret) {
> @@ -147,6 +202,10 @@ int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
> make_call_t0(caller, old_addr, call);
> ret = ftrace_check_current_call(caller, call);
>
> + if (ret)
> + return ret;
> +
> + ret = ftrace_rec_update_ops(rec);
> if (ret)
> return ret;
>
> diff --git a/arch/riscv/kernel/mcount-dyn.S b/arch/riscv/kernel/mcount-dyn.S
> index b7561288e8da..cb241e36e514 100644
> --- a/arch/riscv/kernel/mcount-dyn.S
> +++ b/arch/riscv/kernel/mcount-dyn.S
> @@ -191,11 +191,35 @@
> .endm
>
> .macro PREPARE_ARGS
> - addi a0, t0, -FENTRY_RA_OFFSET
> + addi a0, t0, -FENTRY_RA_OFFSET // ip (callsite's auipc insn)
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
> + /*
> + * When CALL_OPS is enabled (2 or 4) nops [8B] are placed before the
> + * function entry, these are later overwritten with the pointer to the
> + * associated struct ftrace_ops.
> + *
> + * -8: &ftrace_ops of the associated tracer function.
> + *<ftrace enable>:
> + * 0: auipc t0/ra, 0x?
> + * 4: jalr t0/ra, ?(t0/ra)
> + *
> + * -8: &ftrace_nop_ops
> + *<ftrace disable>:
> + * 0: nop
> + * 4: nop
> + *
> + * t0 is set to ip+8 after the jalr is executed at the callsite,
> + * so we find the associated op at t0-16.
> + */
> + mv a1, ra // parent_ip
> + REG_L a2, -16(t0) // op
> + REG_L ra, FTRACE_OPS_FUNC(a2) // op->func
> +#else
> la a1, function_trace_op
> - REG_L a2, 0(a1)
> - mv a1, ra
> - mv a3, sp
> + REG_L a2, 0(a1) // op
> + mv a1, ra // parent_ip
> +#endif
> + mv a3, sp // regs
> .endm
>
> #endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
> @@ -233,8 +257,12 @@ SYM_FUNC_START(ftrace_regs_caller)
> SAVE_ABI_REGS 1
> PREPARE_ARGS
>
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
> + jalr ra
> +#else
> SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
> call ftrace_stub
> +#endif
>
> RESTORE_ABI_REGS 1
> bnez t1, .Ldirect
> @@ -247,9 +275,13 @@ SYM_FUNC_START(ftrace_caller)
> SAVE_ABI_REGS 0
> PREPARE_ARGS
>
> -SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
> + jalr ra
> +#else
> +SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
> call ftrace_stub
>
> +#endif
> RESTORE_ABI_REGS 0
> jr t0
> SYM_FUNC_END(ftrace_caller)
As I'm diving into ftrace right now, I'll give a proper review soon. But
as a note, I noticed that the function_graph tracer, when enabled, makes
the whole system unresponsive (but still up, just very slow). A fix I
sent recently seems to really improve that if you're interested in
testing it (I am :)). You can find it here:
https://lore.kernel.org/linux-riscv/[email protected]/
Thanks,
Alex
+cc Andy and Evgenii
On 06/03/2024 21:35, Alexandre Ghiti wrote:
> Hi Puranjay,
>
> On 06/03/2024 17:59, Puranjay Mohan wrote:
>> This patch enables support for DYNAMIC_FTRACE_WITH_CALL_OPS on RISC-V.
>> This allows each ftrace callsite to provide an ftrace_ops to the common
>> ftrace trampoline, allowing each callsite to invoke distinct tracer
>> functions without the need to fall back to list processing or to
>> allocate custom trampolines for each callsite. This significantly speeds
>> up cases where multiple distinct trace functions are used and callsites
>> are mostly traced by a single tracer.
>>
>> The idea and most of the implementation is taken from the ARM64's
>> implementation of the same feature. The idea is to place a pointer to
>> the ftrace_ops as a literal at a fixed offset from the function entry
>> point, which can be recovered by the common ftrace trampoline.
>>
>> We use -fpatchable-function-entry to reserve 8 bytes above the function
>> entry by emitting 2 4 byte or 4 2 byte nops depending on the
>> presence of
>> CONFIG_RISCV_ISA_C. These 8 bytes are patched at runtime with a pointer
>> to the associated ftrace_ops for that callsite. Functions are aligned to
>> 8 bytes to make sure that the accesses to this literal are atomic.
>>
>> This approach allows for directly invoking ftrace_ops::func even for
>> ftrace_ops which are dynamically-allocated (or part of a module),
>> without going via ftrace_ops_list_func.
>>
>> I've benchamrked this with the ftrace_ops sample module on Qemu, with
>> the next version, I will provide benchmarks on real hardware:
>>
>> Without this patch:
>>
>> +-----------------------+-----------------+----------------------------+
>> | Number of tracers | Total time (ns) | Per-call average time |
>> |-----------------------+-----------------+----------------------------|
>> | Relevant | Irrelevant | 100000 calls | Total (ns) | Overhead (ns) |
>> |----------+------------+-----------------+------------+---------------|
>> | 0 | 0 | 15615700 | 156 | - |
>> | 0 | 1 | 15917600 | 159 | - |
>> | 0 | 2 | 15668000 | 156 | - |
>> | 0 | 10 | 14971500 | 149 | - |
>> | 0 | 100 | 15417600 | 154 | - |
>> | 0 | 200 | 15387000 | 153 | - |
>> |----------+------------+-----------------+------------+---------------|
>> | 1 | 0 | 119906800 | 1199 | 1043 |
>> | 1 | 1 | 137428600 | 1374 | 1218 |
>> | 1 | 2 | 159562400 | 1374 | 1218 |
>> | 1 | 10 | 302099900 | 3020 | 2864 |
>> | 1 | 100 | 2008785500 | 20087 | 19931 |
>> | 1 | 200 | 3965221900 | 39652 | 39496 |
>> |----------+------------+-----------------+------------+---------------|
>> | 1 | 0 | 119166700 | 1191 | 1035 |
>> | 2 | 0 | 157999900 | 1579 | 1423 |
>> | 10 | 0 | 425370100 | 4253 | 4097 |
>> | 100 | 0 | 3595252100 | 35952 | 35796 |
>> | 200 | 0 | 7023485700 | 70234 | 70078 |
>> +----------+------------+-----------------+------------+---------------+
>>
>> Note: per-call overhead is estimated relative to the baseline case with
>> 0 relevant tracers and 0 irrelevant tracers.
>>
>> With this patch:
>>
>> +-----------------------+-----------------+----------------------------+
>> | Number of tracers | Total time (ns) | Per-call average time |
>> |-----------------------+-----------------+----------------------------|
>> | Relevant | Irrelevant | 100000 calls | Total (ns) | Overhead (ns) |
>> |----------+------------+-----------------+------------+---------------|
>> | 0 | 0 | 15254600 | 152 | - |
>> | 0 | 1 | 16136700 | 161 | - |
>> | 0 | 2 | 15329500 | 153 | - |
>> | 0 | 10 | 15148800 | 151 | - |
>> | 0 | 100 | 15746900 | 157 | - |
>> | 0 | 200 | 15737400 | 157 | - |
>> |----------+------------+-----------------+------------+---------------|
>> | 1 | 0 | 47909000 | 479 | 327 |
>> | 1 | 1 | 48297400 | 482 | 330 |
>> | 1 | 2 | 47314100 | 473 | 321 |
>> | 1 | 10 | 47844900 | 478 | 326 |
>> | 1 | 100 | 46591900 | 465 | 313 |
>> | 1 | 200 | 47178900 | 471 | 319 |
>> |----------+------------+-----------------+------------+---------------|
>> | 1 | 0 | 46715800 | 467 | 315 |
>> | 2 | 0 | 155134500 | 1551 | 1399 |
>> | 10 | 0 | 442672800 | 4426 | 4274 |
>> | 100 | 0 | 4092353900 | 40923 | 40771 |
>> | 200 | 0 | 7135796400 | 71357 | 71205 |
>> +----------+------------+-----------------+------------+---------------+
>>
>> Note: per-call overhead is estimated relative to the baseline case with
>> 0 relevant tracers and 0 irrelevant tracers.
>>
>> As can be seen from the above:
>>
>> a) Whenever there is a single relevant tracer function associated
>> with a
>> tracee, the overhead of invoking the tracer is constant, and
>> does not
>> scale with the number of tracers which are *not* associated with
>> that
>> tracee.
>>
>> b) The overhead for a single relevant tracer has dropped to ~1/3 of
>> the
>> overhead prior to this series (from 1035ns to 315ns). This is
>> largely
>> due to permitting calls to dynamically-allocated ftrace_ops without
>> going through ftrace_ops_list_func.
>>
>> Why is this patch a RFC patch:
>> 1. I saw some rcu stalls on Qemu and need to debug them and see if
>> they
>> were introduced by this patch.
>
>
> FYI, I'm currently working on debugging such issues (and other) with
> the *current* ftrace implementation, so probably not caused by your
> patchset. But keep debugging too, maybe this introduces other issues
> or even better, you'll find the root cause :)
>
>
>> 2. This needs to be tested thoroughly on real hardware.
>> 3. Seeking reviews to fix any fundamental problems with this patch
>> that I
>> might have missed due to my lack of RISC-V architecture knowledge.
>> 4. I would like to benchmark this on real hardware and put the
>> results in
>> the commit message.
>>
>> Signed-off-by: Puranjay Mohan <[email protected]>
>> ---
>> arch/riscv/Kconfig | 2 ++
>> arch/riscv/Makefile | 8 +++++
>> arch/riscv/include/asm/ftrace.h | 3 ++
>> arch/riscv/kernel/asm-offsets.c | 3 ++
>> arch/riscv/kernel/ftrace.c | 59 +++++++++++++++++++++++++++++++++
>> arch/riscv/kernel/mcount-dyn.S | 42 ++++++++++++++++++++---
>> 6 files changed, 112 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> index 0bfcfec67ed5..e474742e23b2 100644
>> --- a/arch/riscv/Kconfig
>> +++ b/arch/riscv/Kconfig
>> @@ -78,6 +78,7 @@ config RISCV
>> select EDAC_SUPPORT
>> select FRAME_POINTER if PERF_EVENTS || (FUNCTION_TRACER &&
>> !DYNAMIC_FTRACE)
>> select FTRACE_MCOUNT_USE_PATCHABLE_FUNCTION_ENTRY if
>> DYNAMIC_FTRACE
>> + select FUNCTION_ALIGNMENT_8B if DYNAMIC_FTRACE_WITH_CALL_OPS
>
>
> A recent discussion [1] states that -falign-functions cannot guarantee
> this alignment for all code and that gcc developers came up with a new
> option [2]: WDYT? I have added Andy and Evgenii in +cc to help on that.
>
> [1]
> https://lore.kernel.org/linux-riscv/[email protected]/
> [2]
> https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=0f5a9a00e3ab1fe96142f304cfbcf3f63b15f326
>
>
>> select GENERIC_ARCH_TOPOLOGY
>> select GENERIC_ATOMIC64 if !64BIT
>> select GENERIC_CLOCKEVENTS_BROADCAST if SMP
>> @@ -127,6 +128,7 @@ config RISCV
>> select HAVE_DYNAMIC_FTRACE if !XIP_KERNEL && MMU &&
>> (CLANG_SUPPORTS_DYNAMIC_FTRACE || GCC_SUPPORTS_DYNAMIC_FTRACE)
>> select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
>> select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE
>> + select HAVE_DYNAMIC_FTRACE_WITH_CALL_OPS if
>> (DYNAMIC_FTRACE_WITH_REGS && !CFI_CLANG)
>> select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
>> select HAVE_FUNCTION_GRAPH_TRACER
>> select HAVE_FUNCTION_GRAPH_RETVAL if HAVE_FUNCTION_GRAPH_TRACER
>> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
>> index 252d63942f34..875ad5dc3d32 100644
>> --- a/arch/riscv/Makefile
>> +++ b/arch/riscv/Makefile
>> @@ -14,12 +14,20 @@ endif
>> ifeq ($(CONFIG_DYNAMIC_FTRACE),y)
>> LDFLAGS_vmlinux += --no-relax
>> KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY
>> +ifeq ($(CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS), y)
>> +ifeq ($(CONFIG_RISCV_ISA_C),y)
>> + CC_FLAGS_FTRACE := -fpatchable-function-entry=8,4
>> +else
>> + CC_FLAGS_FTRACE := -fpatchable-function-entry=4,2
>> +endif
>> +else
>> ifeq ($(CONFIG_RISCV_ISA_C),y)
>> CC_FLAGS_FTRACE := -fpatchable-function-entry=4
>> else
>> CC_FLAGS_FTRACE := -fpatchable-function-entry=2
>> endif
>> endif
>> +endif
>> ifeq ($(CONFIG_CMODEL_MEDLOW),y)
>> KBUILD_CFLAGS_MODULE += -mcmodel=medany
>> diff --git a/arch/riscv/include/asm/ftrace.h
>> b/arch/riscv/include/asm/ftrace.h
>> index 329172122952..c9a84222c9ea 100644
>> --- a/arch/riscv/include/asm/ftrace.h
>> +++ b/arch/riscv/include/asm/ftrace.h
>> @@ -28,6 +28,9 @@
>> void MCOUNT_NAME(void);
>> static inline unsigned long ftrace_call_adjust(unsigned long addr)
>> {
>> + if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS))
>> + return addr + 8;
>> +
>> return addr;
>> }
>> diff --git a/arch/riscv/kernel/asm-offsets.c
>> b/arch/riscv/kernel/asm-offsets.c
>> index a03129f40c46..7d7c4b486852 100644
>> --- a/arch/riscv/kernel/asm-offsets.c
>> +++ b/arch/riscv/kernel/asm-offsets.c
>> @@ -488,4 +488,7 @@ void asm_offsets(void)
>> DEFINE(STACKFRAME_SIZE_ON_STACK, ALIGN(sizeof(struct
>> stackframe), STACK_ALIGN));
>> OFFSET(STACKFRAME_FP, stackframe, fp);
>> OFFSET(STACKFRAME_RA, stackframe, ra);
>> +#ifdef CONFIG_FUNCTION_TRACER
>> + DEFINE(FTRACE_OPS_FUNC, offsetof(struct ftrace_ops, func));
>> +#endif
>> }
>> diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
>> index f5aa24d9e1c1..e2e75e15d32e 100644
>> --- a/arch/riscv/kernel/ftrace.c
>> +++ b/arch/riscv/kernel/ftrace.c
>> @@ -82,9 +82,52 @@ static int __ftrace_modify_call(unsigned long
>> hook_pos, unsigned long target,
>> return 0;
>> }
>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
>> +static const struct ftrace_ops *riscv64_rec_get_ops(struct
>> dyn_ftrace *rec)
>> +{
>> + const struct ftrace_ops *ops = NULL;
>> +
>> + if (rec->flags & FTRACE_FL_CALL_OPS_EN) {
>> + ops = ftrace_find_unique_ops(rec);
>> + WARN_ON_ONCE(!ops);
>> + }
>> +
>> + if (!ops)
>> + ops = &ftrace_list_ops;
>> +
>> + return ops;
>> +}
>> +
>> +static int ftrace_rec_set_ops(const struct dyn_ftrace *rec,
>> + const struct ftrace_ops *ops)
>> +{
>> + unsigned long literal = rec->ip - 8;
>> +
>> + return patch_text_nosync((void *)literal, &ops, sizeof(ops));
>> +}
>> +
>> +static int ftrace_rec_set_nop_ops(struct dyn_ftrace *rec)
>> +{
>> + return ftrace_rec_set_ops(rec, &ftrace_nop_ops);
>> +}
>> +
>> +static int ftrace_rec_update_ops(struct dyn_ftrace *rec)
>> +{
>> + return ftrace_rec_set_ops(rec, riscv64_rec_get_ops(rec));
>> +}
>> +#else
>> +static int ftrace_rec_set_nop_ops(struct dyn_ftrace *rec) { return 0; }
>> +static int ftrace_rec_update_ops(struct dyn_ftrace *rec) { return 0; }
>> +#endif
>> +
>> int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
>> {
>> unsigned int call[2];
>> + int ret;
>> +
>> + ret = ftrace_rec_update_ops(rec);
>> + if (ret)
>> + return ret;
>> make_call_t0(rec->ip, addr, call);
>> @@ -98,6 +141,11 @@ int ftrace_make_nop(struct module *mod, struct
>> dyn_ftrace *rec,
>> unsigned long addr)
>> {
>> unsigned int nops[2] = {NOP4, NOP4};
>> + int ret;
>> +
>> + ret = ftrace_rec_set_nop_ops(rec);
>> + if (ret)
>> + return ret;
>> if (patch_text_nosync((void *)rec->ip, nops, MCOUNT_INSN_SIZE))
>> return -EPERM;
>> @@ -125,6 +173,13 @@ int ftrace_init_nop(struct module *mod, struct
>> dyn_ftrace *rec)
>> int ftrace_update_ftrace_func(ftrace_func_t func)
>> {
>> + /*
>> + * When using CALL_OPS, the function to call is associated with the
>> + * call site, and we don't have a global function pointer to
>> update.
>> + */
>> + if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS))
>> + return 0;
>> +
>> int ret = __ftrace_modify_call((unsigned long)&ftrace_call,
>> (unsigned long)func, true, true);
>> if (!ret) {
>> @@ -147,6 +202,10 @@ int ftrace_modify_call(struct dyn_ftrace *rec,
>> unsigned long old_addr,
>> make_call_t0(caller, old_addr, call);
>> ret = ftrace_check_current_call(caller, call);
>> + if (ret)
>> + return ret;
>> +
>> + ret = ftrace_rec_update_ops(rec);
>> if (ret)
>> return ret;
>> diff --git a/arch/riscv/kernel/mcount-dyn.S
>> b/arch/riscv/kernel/mcount-dyn.S
>> index b7561288e8da..cb241e36e514 100644
>> --- a/arch/riscv/kernel/mcount-dyn.S
>> +++ b/arch/riscv/kernel/mcount-dyn.S
>> @@ -191,11 +191,35 @@
>> .endm
>> .macro PREPARE_ARGS
>> - addi a0, t0, -FENTRY_RA_OFFSET
>> + addi a0, t0, -FENTRY_RA_OFFSET // ip (callsite's auipc insn)
>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
>> + /*
>> + * When CALL_OPS is enabled (2 or 4) nops [8B] are placed before
>> the
>> + * function entry, these are later overwritten with the pointer
>> to the
>> + * associated struct ftrace_ops.
>> + *
>> + * -8: &ftrace_ops of the associated tracer function.
>> + *<ftrace enable>:
>> + * 0: auipc t0/ra, 0x?
>> + * 4: jalr t0/ra, ?(t0/ra)
>> + *
>> + * -8: &ftrace_nop_ops
>> + *<ftrace disable>:
>> + * 0: nop
>> + * 4: nop
>> + *
>> + * t0 is set to ip+8 after the jalr is executed at the callsite,
>> + * so we find the associated op at t0-16.
>> + */
>> + mv a1, ra // parent_ip
>> + REG_L a2, -16(t0) // op
>> + REG_L ra, FTRACE_OPS_FUNC(a2) // op->func
>> +#else
>> la a1, function_trace_op
>> - REG_L a2, 0(a1)
>> - mv a1, ra
>> - mv a3, sp
>> + REG_L a2, 0(a1) // op
>> + mv a1, ra // parent_ip
>> +#endif
>> + mv a3, sp // regs
>> .endm
>> #endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
>> @@ -233,8 +257,12 @@ SYM_FUNC_START(ftrace_regs_caller)
>> SAVE_ABI_REGS 1
>> PREPARE_ARGS
>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
>> + jalr ra
>> +#else
>> SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
>> call ftrace_stub
>> +#endif
>> RESTORE_ABI_REGS 1
>> bnez t1, .Ldirect
>> @@ -247,9 +275,13 @@ SYM_FUNC_START(ftrace_caller)
>> SAVE_ABI_REGS 0
>> PREPARE_ARGS
>> -SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
>> + jalr ra
>> +#else
>> +SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
>> call ftrace_stub
>> +#endif
>> RESTORE_ABI_REGS 0
>> jr t0
>> SYM_FUNC_END(ftrace_caller)
>
>
> As I'm diving into ftrace right now, I'll give a proper review soon.
> But as a note, I noticed that the function_graph tracer, when enabled,
> makes the whole system unresponsive (but still up, just very slow). A
> fix I sent recently seems to really improve that if you're interested
> in testing it (I am :)). You can find it here:
> https://lore.kernel.org/linux-riscv/[email protected]/
>
> Thanks,
>
> Alex
>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Hi Alex,
On Wed, Mar 6, 2024 at 9:35 PM Alexandre Ghiti <[email protected]> wrote:
>
> Hi Puranjay,
>
> On 06/03/2024 17:59, Puranjay Mohan wrote:
> > This patch enables support for DYNAMIC_FTRACE_WITH_CALL_OPS on RISC-V.
> > This allows each ftrace callsite to provide an ftrace_ops to the common
> > ftrace trampoline, allowing each callsite to invoke distinct tracer
> > functions without the need to fall back to list processing or to
> > allocate custom trampolines for each callsite. This significantly speeds
> > up cases where multiple distinct trace functions are used and callsites
> > are mostly traced by a single tracer.
> >
> > The idea and most of the implementation is taken from the ARM64's
> > implementation of the same feature. The idea is to place a pointer to
> > the ftrace_ops as a literal at a fixed offset from the function entry
> > point, which can be recovered by the common ftrace trampoline.
> >
> > We use -fpatchable-function-entry to reserve 8 bytes above the function
> > entry by emitting 2 4 byte or 4 2 byte nops depending on the presence of
> > CONFIG_RISCV_ISA_C. These 8 bytes are patched at runtime with a pointer
> > to the associated ftrace_ops for that callsite. Functions are aligned to
> > 8 bytes to make sure that the accesses to this literal are atomic.
> >
> > This approach allows for directly invoking ftrace_ops::func even for
> > ftrace_ops which are dynamically-allocated (or part of a module),
> > without going via ftrace_ops_list_func.
> >
> > I've benchamrked this with the ftrace_ops sample module on Qemu, with
> > the next version, I will provide benchmarks on real hardware:
> >
> > Without this patch:
> >
> > +-----------------------+-----------------+----------------------------+
> > | Number of tracers | Total time (ns) | Per-call average time |
> > |-----------------------+-----------------+----------------------------|
> > | Relevant | Irrelevant | 100000 calls | Total (ns) | Overhead (ns) |
> > |----------+------------+-----------------+------------+---------------|
> > | 0 | 0 | 15615700 | 156 | - |
> > | 0 | 1 | 15917600 | 159 | - |
> > | 0 | 2 | 15668000 | 156 | - |
> > | 0 | 10 | 14971500 | 149 | - |
> > | 0 | 100 | 15417600 | 154 | - |
> > | 0 | 200 | 15387000 | 153 | - |
> > |----------+------------+-----------------+------------+---------------|
> > | 1 | 0 | 119906800 | 1199 | 1043 |
> > | 1 | 1 | 137428600 | 1374 | 1218 |
> > | 1 | 2 | 159562400 | 1374 | 1218 |
> > | 1 | 10 | 302099900 | 3020 | 2864 |
> > | 1 | 100 | 2008785500 | 20087 | 19931 |
> > | 1 | 200 | 3965221900 | 39652 | 39496 |
> > |----------+------------+-----------------+------------+---------------|
> > | 1 | 0 | 119166700 | 1191 | 1035 |
> > | 2 | 0 | 157999900 | 1579 | 1423 |
> > | 10 | 0 | 425370100 | 4253 | 4097 |
> > | 100 | 0 | 3595252100 | 35952 | 35796 |
> > | 200 | 0 | 7023485700 | 70234 | 70078 |
> > +----------+------------+-----------------+------------+---------------+
> >
> > Note: per-call overhead is estimated relative to the baseline case with
> > 0 relevant tracers and 0 irrelevant tracers.
> >
> > With this patch:
> >
> > +-----------------------+-----------------+----------------------------+
> > | Number of tracers | Total time (ns) | Per-call average time |
> > |-----------------------+-----------------+----------------------------|
> > | Relevant | Irrelevant | 100000 calls | Total (ns) | Overhead (ns) |
> > |----------+------------+-----------------+------------+---------------|
> > | 0 | 0 | 15254600 | 152 | - |
> > | 0 | 1 | 16136700 | 161 | - |
> > | 0 | 2 | 15329500 | 153 | - |
> > | 0 | 10 | 15148800 | 151 | - |
> > | 0 | 100 | 15746900 | 157 | - |
> > | 0 | 200 | 15737400 | 157 | - |
> > |----------+------------+-----------------+------------+---------------|
> > | 1 | 0 | 47909000 | 479 | 327 |
> > | 1 | 1 | 48297400 | 482 | 330 |
> > | 1 | 2 | 47314100 | 473 | 321 |
> > | 1 | 10 | 47844900 | 478 | 326 |
> > | 1 | 100 | 46591900 | 465 | 313 |
> > | 1 | 200 | 47178900 | 471 | 319 |
> > |----------+------------+-----------------+------------+---------------|
> > | 1 | 0 | 46715800 | 467 | 315 |
> > | 2 | 0 | 155134500 | 1551 | 1399 |
> > | 10 | 0 | 442672800 | 4426 | 4274 |
> > | 100 | 0 | 4092353900 | 40923 | 40771 |
> > | 200 | 0 | 7135796400 | 71357 | 71205 |
> > +----------+------------+-----------------+------------+---------------+
> >
> > Note: per-call overhead is estimated relative to the baseline case with
> > 0 relevant tracers and 0 irrelevant tracers.
> >
> > As can be seen from the above:
> >
> > a) Whenever there is a single relevant tracer function associated with a
> > tracee, the overhead of invoking the tracer is constant, and does not
> > scale with the number of tracers which are *not* associated with that
> > tracee.
> >
> > b) The overhead for a single relevant tracer has dropped to ~1/3 of the
> > overhead prior to this series (from 1035ns to 315ns). This is largely
> > due to permitting calls to dynamically-allocated ftrace_ops without
> > going through ftrace_ops_list_func.
> >
> > Why is this patch a RFC patch:
> > 1. I saw some rcu stalls on Qemu and need to debug them and see if they
> > were introduced by this patch.
>
>
> FYI, I'm currently working on debugging such issues (and other) with the
> *current* ftrace implementation, so probably not caused by your
> patchset. But keep debugging too, maybe this introduces other issues or
> even better, you'll find the root cause :)
>
>
> > 2. This needs to be tested thoroughly on real hardware.
> > 3. Seeking reviews to fix any fundamental problems with this patch that I
> > might have missed due to my lack of RISC-V architecture knowledge.
> > 4. I would like to benchmark this on real hardware and put the results in
> > the commit message.
> >
> > Signed-off-by: Puranjay Mohan <[email protected]>
> > ---
> > arch/riscv/Kconfig | 2 ++
> > arch/riscv/Makefile | 8 +++++
> > arch/riscv/include/asm/ftrace.h | 3 ++
> > arch/riscv/kernel/asm-offsets.c | 3 ++
> > arch/riscv/kernel/ftrace.c | 59 +++++++++++++++++++++++++++++++++
> > arch/riscv/kernel/mcount-dyn.S | 42 ++++++++++++++++++++---
> > 6 files changed, 112 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index 0bfcfec67ed5..e474742e23b2 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -78,6 +78,7 @@ config RISCV
> > select EDAC_SUPPORT
> > select FRAME_POINTER if PERF_EVENTS || (FUNCTION_TRACER && !DYNAMIC_FTRACE)
> > select FTRACE_MCOUNT_USE_PATCHABLE_FUNCTION_ENTRY if DYNAMIC_FTRACE
> > + select FUNCTION_ALIGNMENT_8B if DYNAMIC_FTRACE_WITH_CALL_OPS
>
>
> A recent discussion [1] states that -falign-functions cannot guarantee
> this alignment for all code and that gcc developers came up with a new
> option [2]: WDYT? I have added Andy and Evgenii in +cc to help on that.
I saw arm64 uses the same and assumes this guarantee, maybe it is broken too?
Will look into the discussion and try to use the other option. I am
currently using Clang to build the kernel.
>
> [1]
> https://lore.kernel.org/linux-riscv/[email protected]/
> [2]
> https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=0f5a9a00e3ab1fe96142f304cfbcf3f63b15f326
>
>
> > select GENERIC_ARCH_TOPOLOGY
> > select GENERIC_ATOMIC64 if !64BIT
> > select GENERIC_CLOCKEVENTS_BROADCAST if SMP
> > @@ -127,6 +128,7 @@ config RISCV
> > select HAVE_DYNAMIC_FTRACE if !XIP_KERNEL && MMU && (CLANG_SUPPORTS_DYNAMIC_FTRACE || GCC_SUPPORTS_DYNAMIC_FTRACE)
> > select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> > select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE
> > + select HAVE_DYNAMIC_FTRACE_WITH_CALL_OPS if (DYNAMIC_FTRACE_WITH_REGS && !CFI_CLANG)
> > select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
> > select HAVE_FUNCTION_GRAPH_TRACER
> > select HAVE_FUNCTION_GRAPH_RETVAL if HAVE_FUNCTION_GRAPH_TRACER
> > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> > index 252d63942f34..875ad5dc3d32 100644
> > --- a/arch/riscv/Makefile
> > +++ b/arch/riscv/Makefile
> > @@ -14,12 +14,20 @@ endif
> > ifeq ($(CONFIG_DYNAMIC_FTRACE),y)
> > LDFLAGS_vmlinux += --no-relax
> > KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY
> > +ifeq ($(CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS), y)
> > +ifeq ($(CONFIG_RISCV_ISA_C),y)
> > + CC_FLAGS_FTRACE := -fpatchable-function-entry=8,4
> > +else
> > + CC_FLAGS_FTRACE := -fpatchable-function-entry=4,2
> > +endif
> > +else
> > ifeq ($(CONFIG_RISCV_ISA_C),y)
> > CC_FLAGS_FTRACE := -fpatchable-function-entry=4
> > else
> > CC_FLAGS_FTRACE := -fpatchable-function-entry=2
> > endif
> > endif
> > +endif
> >
> > ifeq ($(CONFIG_CMODEL_MEDLOW),y)
> > KBUILD_CFLAGS_MODULE += -mcmodel=medany
> > diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
> > index 329172122952..c9a84222c9ea 100644
> > --- a/arch/riscv/include/asm/ftrace.h
> > +++ b/arch/riscv/include/asm/ftrace.h
> > @@ -28,6 +28,9 @@
> > void MCOUNT_NAME(void);
> > static inline unsigned long ftrace_call_adjust(unsigned long addr)
> > {
> > + if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS))
> > + return addr + 8;
> > +
> > return addr;
> > }
> >
> > diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c
> > index a03129f40c46..7d7c4b486852 100644
> > --- a/arch/riscv/kernel/asm-offsets.c
> > +++ b/arch/riscv/kernel/asm-offsets.c
> > @@ -488,4 +488,7 @@ void asm_offsets(void)
> > DEFINE(STACKFRAME_SIZE_ON_STACK, ALIGN(sizeof(struct stackframe), STACK_ALIGN));
> > OFFSET(STACKFRAME_FP, stackframe, fp);
> > OFFSET(STACKFRAME_RA, stackframe, ra);
> > +#ifdef CONFIG_FUNCTION_TRACER
> > + DEFINE(FTRACE_OPS_FUNC, offsetof(struct ftrace_ops, func));
> > +#endif
> > }
> > diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
> > index f5aa24d9e1c1..e2e75e15d32e 100644
> > --- a/arch/riscv/kernel/ftrace.c
> > +++ b/arch/riscv/kernel/ftrace.c
> > @@ -82,9 +82,52 @@ static int __ftrace_modify_call(unsigned long hook_pos, unsigned long target,
> > return 0;
> > }
> >
> > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
> > +static const struct ftrace_ops *riscv64_rec_get_ops(struct dyn_ftrace *rec)
> > +{
> > + const struct ftrace_ops *ops = NULL;
> > +
> > + if (rec->flags & FTRACE_FL_CALL_OPS_EN) {
> > + ops = ftrace_find_unique_ops(rec);
> > + WARN_ON_ONCE(!ops);
> > + }
> > +
> > + if (!ops)
> > + ops = &ftrace_list_ops;
> > +
> > + return ops;
> > +}
> > +
> > +static int ftrace_rec_set_ops(const struct dyn_ftrace *rec,
> > + const struct ftrace_ops *ops)
> > +{
> > + unsigned long literal = rec->ip - 8;
> > +
> > + return patch_text_nosync((void *)literal, &ops, sizeof(ops));
^^
I will change this to use a new function that does a 64 bit write and
doesn't do flush_icache_range() as naturally aligned writes are
atomic and therefore synchronization is not required here.
> > +}
> > +
> > +static int ftrace_rec_set_nop_ops(struct dyn_ftrace *rec)
> > +{
> > + return ftrace_rec_set_ops(rec, &ftrace_nop_ops);
> > +}
> > +
> > +static int ftrace_rec_update_ops(struct dyn_ftrace *rec)
> > +{
> > + return ftrace_rec_set_ops(rec, riscv64_rec_get_ops(rec));
> > +}
> > +#else
> > +static int ftrace_rec_set_nop_ops(struct dyn_ftrace *rec) { return 0; }
> > +static int ftrace_rec_update_ops(struct dyn_ftrace *rec) { return 0; }
> > +#endif
> > +
> > int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
> > {
> > unsigned int call[2];
> > + int ret;
> > +
> > + ret = ftrace_rec_update_ops(rec);
> > + if (ret)
> > + return ret;
> >
> > make_call_t0(rec->ip, addr, call);
> >
> > @@ -98,6 +141,11 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
> > unsigned long addr)
> > {
> > unsigned int nops[2] = {NOP4, NOP4};
> > + int ret;
> > +
> > + ret = ftrace_rec_set_nop_ops(rec);
> > + if (ret)
> > + return ret;
> >
> > if (patch_text_nosync((void *)rec->ip, nops, MCOUNT_INSN_SIZE))
> > return -EPERM;
> > @@ -125,6 +173,13 @@ int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> >
> > int ftrace_update_ftrace_func(ftrace_func_t func)
> > {
> > + /*
> > + * When using CALL_OPS, the function to call is associated with the
> > + * call site, and we don't have a global function pointer to update.
> > + */
> > + if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS))
> > + return 0;
> > +
> > int ret = __ftrace_modify_call((unsigned long)&ftrace_call,
> > (unsigned long)func, true, true);
> > if (!ret) {
> > @@ -147,6 +202,10 @@ int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
> > make_call_t0(caller, old_addr, call);
> > ret = ftrace_check_current_call(caller, call);
> >
> > + if (ret)
> > + return ret;
> > +
> > + ret = ftrace_rec_update_ops(rec);
> > if (ret)
> > return ret;
> >
> > diff --git a/arch/riscv/kernel/mcount-dyn.S b/arch/riscv/kernel/mcount-dyn.S
> > index b7561288e8da..cb241e36e514 100644
> > --- a/arch/riscv/kernel/mcount-dyn.S
> > +++ b/arch/riscv/kernel/mcount-dyn.S
> > @@ -191,11 +191,35 @@
> > .endm
> >
> > .macro PREPARE_ARGS
> > - addi a0, t0, -FENTRY_RA_OFFSET
> > + addi a0, t0, -FENTRY_RA_OFFSET // ip (callsite's auipc insn)
> > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
> > + /*
> > + * When CALL_OPS is enabled (2 or 4) nops [8B] are placed before the
> > + * function entry, these are later overwritten with the pointer to the
> > + * associated struct ftrace_ops.
> > + *
> > + * -8: &ftrace_ops of the associated tracer function.
> > + *<ftrace enable>:
> > + * 0: auipc t0/ra, 0x?
> > + * 4: jalr t0/ra, ?(t0/ra)
> > + *
> > + * -8: &ftrace_nop_ops
> > + *<ftrace disable>:
> > + * 0: nop
> > + * 4: nop
> > + *
> > + * t0 is set to ip+8 after the jalr is executed at the callsite,
> > + * so we find the associated op at t0-16.
> > + */
> > + mv a1, ra // parent_ip
> > + REG_L a2, -16(t0) // op
> > + REG_L ra, FTRACE_OPS_FUNC(a2) // op->func
> > +#else
> > la a1, function_trace_op
> > - REG_L a2, 0(a1)
> > - mv a1, ra
> > - mv a3, sp
> > + REG_L a2, 0(a1) // op
> > + mv a1, ra // parent_ip
> > +#endif
> > + mv a3, sp // regs
> > .endm
> >
> > #endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
> > @@ -233,8 +257,12 @@ SYM_FUNC_START(ftrace_regs_caller)
> > SAVE_ABI_REGS 1
> > PREPARE_ARGS
> >
> > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
> > + jalr ra
> > +#else
> > SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
> > call ftrace_stub
> > +#endif
> >
> > RESTORE_ABI_REGS 1
> > bnez t1, .Ldirect
> > @@ -247,9 +275,13 @@ SYM_FUNC_START(ftrace_caller)
> > SAVE_ABI_REGS 0
> > PREPARE_ARGS
> >
> > -SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
^^ this hunk is a mistake, I will fix it in the next version.
> > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
> > + jalr ra
> > +#else
> > +SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
> > call ftrace_stub
> >
> > +#endif
> > RESTORE_ABI_REGS 0
> > jr t0
> > SYM_FUNC_END(ftrace_caller)
>
>
> As I'm diving into ftrace right now, I'll give a proper review soon. But
> as a note, I noticed that the function_graph tracer, when enabled, makes
> the whole system unresponsive (but still up, just very slow). A fix I
> sent recently seems to really improve that if you're interested in
> testing it (I am :)). You can find it here:
> https://lore.kernel.org/linux-riscv/[email protected]/
I saw the same issue where function_graph was making the system slow on Qemu.
What hardware do you use for testing? or are you testing on Qemu as well?
I tested your patch, it speeds up the process of patching the
instructions so the following
command completes ~2.5 seconds faster compared to without your patch.
$ time echo function_graph > current_tracer
But at runtime the system is still slow and laggy with function_graph,
I guess because my
Qemu setup is not powerful enough to run function_graph.
Thanks,
Puranjay
Puranjay!
Puranjay Mohan <[email protected]> writes:
> This patch enables support for DYNAMIC_FTRACE_WITH_CALL_OPS on RISC-V.
> This allows each ftrace callsite to provide an ftrace_ops to the common
> ftrace trampoline, allowing each callsite to invoke distinct tracer
> functions without the need to fall back to list processing or to
> allocate custom trampolines for each callsite. This significantly speeds
> up cases where multiple distinct trace functions are used and callsites
> are mostly traced by a single tracer.
>
> The idea and most of the implementation is taken from the ARM64's
> implementation of the same feature. The idea is to place a pointer to
> the ftrace_ops as a literal at a fixed offset from the function entry
> point, which can be recovered by the common ftrace trampoline.
Not really a review, but some more background; Another rationale (on-top
of the improved per-call performance!) for CALL_OPS was to use it to
build ftrace direct call support (which BPF uses a lot!). Mark, please
correct me if I'm lying here!
On Arm64, CALL_OPS makes it possible to implement direct calls, while
only patching one BL instruction -- nice!
On RISC-V we cannot use use the same ideas as Arm64 straight off,
because the range of jal (compare to BL) is simply too short (+/-1M).
So, on RISC-V we need to use a full auipc/jal pair (the text patching
story is another chapter, but let's leave that aside for now). Since we
have to patch multiple instructions, the cmodx situation doesn't really
improve with CALL_OPS.
Let's say that we continue building on your patch and implement direct
calls on CALL_OPS for RISC-V as well.
From Florent's commit message for direct calls:
| There are a few cases to distinguish:
| - If a direct call ops is the only one tracing a function:
| - If the direct called trampoline is within the reach of a BL
| instruction
| -> the ftrace patchsite jumps to the trampoline
| - Else
| -> the ftrace patchsite jumps to the ftrace_caller trampoline which
| reads the ops pointer in the patchsite and jumps to the direct
| call address stored in the ops
| - Else
| -> the ftrace patchsite jumps to the ftrace_caller trampoline and its
| ops literal points to ftrace_list_ops so it iterates over all
| registered ftrace ops, including the direct call ops and calls its
| call_direct_funcs handler which stores the direct called
| trampoline's address in the ftrace_regs and the ftrace_caller
| trampoline will return to that address instead of returning to the
| traced function
On RISC-V, where auipc/jalr is used, the direct called trampoline would
always be reachable, and then first Else-clause would never be entered.
This means the the performance for direct calls would be the same as the
one we have today (i.e. no regression!).
RISC-V does like x86 does (-ish) -- patch multiple instructions, long
reach.
Arm64 uses CALL_OPS and patch one instruction BL.
Now, with this background in mind, compared to what we have today,
CALL_OPS would give us (again assuming we're using it for direct calls):
* Better performance for tracer per-call (faster ops lookup) GOOD
* Larger text size (function alignment + extra nops) BAD
* Same direct call performance NEUTRAL
* Same complicated text patching required NEUTRAL
It would be interesting to see how the per-call performance would
improve on x86 with CALL_OPS! ;-)
I'm trying to wrap my head if it makes sense to have it on RISC-V, given
that we're a bit different from Arm64. Does the scale tip to the GOOD
side?
Oh, and we really need to see performance numbers on real HW! I have a
VF2 that I could try this series on.
Björn
Hi Björn,
On Thu, Mar 7, 2024 at 8:27 PM Björn Töpel <[email protected]> wrote:
>
> Puranjay!
>
> Puranjay Mohan <[email protected]> writes:
>
> > This patch enables support for DYNAMIC_FTRACE_WITH_CALL_OPS on RISC-V.
> > This allows each ftrace callsite to provide an ftrace_ops to the common
> > ftrace trampoline, allowing each callsite to invoke distinct tracer
> > functions without the need to fall back to list processing or to
> > allocate custom trampolines for each callsite. This significantly speeds
> > up cases where multiple distinct trace functions are used and callsites
> > are mostly traced by a single tracer.
> >
> > The idea and most of the implementation is taken from the ARM64's
> > implementation of the same feature. The idea is to place a pointer to
> > the ftrace_ops as a literal at a fixed offset from the function entry
> > point, which can be recovered by the common ftrace trampoline.
>
> Not really a review, but some more background; Another rationale (on-top
> of the improved per-call performance!) for CALL_OPS was to use it to
> build ftrace direct call support (which BPF uses a lot!). Mark, please
> correct me if I'm lying here!
>
> On Arm64, CALL_OPS makes it possible to implement direct calls, while
> only patching one BL instruction -- nice!
>
> On RISC-V we cannot use use the same ideas as Arm64 straight off,
> because the range of jal (compare to BL) is simply too short (+/-1M).
> So, on RISC-V we need to use a full auipc/jal pair (the text patching
> story is another chapter, but let's leave that aside for now). Since we
> have to patch multiple instructions, the cmodx situation doesn't really
> improve with CALL_OPS.
>
> Let's say that we continue building on your patch and implement direct
> calls on CALL_OPS for RISC-V as well.
>
> From Florent's commit message for direct calls:
>
> | There are a few cases to distinguish:
> | - If a direct call ops is the only one tracing a function:
> | - If the direct called trampoline is within the reach of a BL
> | instruction
> | -> the ftrace patchsite jumps to the trampoline
> | - Else
> | -> the ftrace patchsite jumps to the ftrace_caller trampoline which
> | reads the ops pointer in the patchsite and jumps to the direct
> | call address stored in the ops
> | - Else
> | -> the ftrace patchsite jumps to the ftrace_caller trampoline and its
> | ops literal points to ftrace_list_ops so it iterates over all
> | registered ftrace ops, including the direct call ops and calls its
> | call_direct_funcs handler which stores the direct called
> | trampoline's address in the ftrace_regs and the ftrace_caller
> | trampoline will return to that address instead of returning to the
> | traced function
>
> On RISC-V, where auipc/jalr is used, the direct called trampoline would
> always be reachable, and then first Else-clause would never be entered.
> This means the the performance for direct calls would be the same as the
> one we have today (i.e. no regression!).
>
> RISC-V does like x86 does (-ish) -- patch multiple instructions, long
> reach.
>
> Arm64 uses CALL_OPS and patch one instruction BL.
>
> Now, with this background in mind, compared to what we have today,
> CALL_OPS would give us (again assuming we're using it for direct calls):
>
> * Better performance for tracer per-call (faster ops lookup) GOOD
^ this was the only motivation for me to implement this patch.
I don't think implementing direct calls over call ops is fruitful for
RISC-V because once
the auipc/jalr can be patched atomically, the direct call trampoline
is always reachable.
Solving the atomic text patching problem would be fun!! I am eager to
see how it will be
solved.
> * Larger text size (function alignment + extra nops) BAD
> * Same direct call performance NEUTRAL
> * Same complicated text patching required NEUTRAL
>
> It would be interesting to see how the per-call performance would
> improve on x86 with CALL_OPS! ;-)
If I remember from Steven's talk, x86 uses dynamically allocated trampolines
for per callsite tracers, would CALL_OPS provide better performance than that?
>
> I'm trying to wrap my head if it makes sense to have it on RISC-V, given
> that we're a bit different from Arm64. Does the scale tip to the GOOD
> side?
>
> Oh, and we really need to see performance numbers on real HW! I have a
> VF2 that I could try this series on.
It would be great if you can do it :D.
Thanks,
Puranjay
On Thu, Mar 7, 2024 at 8:19 AM Puranjay Mohan <[email protected]> wrote:
>
> Hi Alex,
>
> On Wed, Mar 6, 2024 at 9:35 PM Alexandre Ghiti <[email protected]> wrote:
> >
> > Hi Puranjay,
> >
> > On 06/03/2024 17:59, Puranjay Mohan wrote:
> > > This patch enables support for DYNAMIC_FTRACE_WITH_CALL_OPS on RISC-V.
> > > This allows each ftrace callsite to provide an ftrace_ops to the common
> > > ftrace trampoline, allowing each callsite to invoke distinct tracer
> > > functions without the need to fall back to list processing or to
> > > allocate custom trampolines for each callsite. This significantly speeds
> > > up cases where multiple distinct trace functions are used and callsites
> > > are mostly traced by a single tracer.
> > >
> > > The idea and most of the implementation is taken from the ARM64's
> > > implementation of the same feature. The idea is to place a pointer to
> > > the ftrace_ops as a literal at a fixed offset from the function entry
> > > point, which can be recovered by the common ftrace trampoline.
> > >
> > > We use -fpatchable-function-entry to reserve 8 bytes above the function
> > > entry by emitting 2 4 byte or 4 2 byte nops depending on the presence of
> > > CONFIG_RISCV_ISA_C. These 8 bytes are patched at runtime with a pointer
> > > to the associated ftrace_ops for that callsite. Functions are aligned to
> > > 8 bytes to make sure that the accesses to this literal are atomic.
> > >
> > > This approach allows for directly invoking ftrace_ops::func even for
> > > ftrace_ops which are dynamically-allocated (or part of a module),
> > > without going via ftrace_ops_list_func.
> > >
> > > I've benchamrked this with the ftrace_ops sample module on Qemu, with
> > > the next version, I will provide benchmarks on real hardware:
> > >
> > > Without this patch:
> > >
> > > +-----------------------+-----------------+----------------------------+
> > > | Number of tracers | Total time (ns) | Per-call average time |
> > > |-----------------------+-----------------+----------------------------|
> > > | Relevant | Irrelevant | 100000 calls | Total (ns) | Overhead (ns) |
> > > |----------+------------+-----------------+------------+---------------|
> > > | 0 | 0 | 15615700 | 156 | - |
> > > | 0 | 1 | 15917600 | 159 | - |
> > > | 0 | 2 | 15668000 | 156 | - |
> > > | 0 | 10 | 14971500 | 149 | - |
> > > | 0 | 100 | 15417600 | 154 | - |
> > > | 0 | 200 | 15387000 | 153 | - |
> > > |----------+------------+-----------------+------------+---------------|
> > > | 1 | 0 | 119906800 | 1199 | 1043 |
> > > | 1 | 1 | 137428600 | 1374 | 1218 |
> > > | 1 | 2 | 159562400 | 1374 | 1218 |
> > > | 1 | 10 | 302099900 | 3020 | 2864 |
> > > | 1 | 100 | 2008785500 | 20087 | 19931 |
> > > | 1 | 200 | 3965221900 | 39652 | 39496 |
> > > |----------+------------+-----------------+------------+---------------|
> > > | 1 | 0 | 119166700 | 1191 | 1035 |
> > > | 2 | 0 | 157999900 | 1579 | 1423 |
> > > | 10 | 0 | 425370100 | 4253 | 4097 |
> > > | 100 | 0 | 3595252100 | 35952 | 35796 |
> > > | 200 | 0 | 7023485700 | 70234 | 70078 |
> > > +----------+------------+-----------------+------------+---------------+
> > >
> > > Note: per-call overhead is estimated relative to the baseline case with
> > > 0 relevant tracers and 0 irrelevant tracers.
> > >
> > > With this patch:
> > >
> > > +-----------------------+-----------------+----------------------------+
> > > | Number of tracers | Total time (ns) | Per-call average time |
> > > |-----------------------+-----------------+----------------------------|
> > > | Relevant | Irrelevant | 100000 calls | Total (ns) | Overhead (ns) |
> > > |----------+------------+-----------------+------------+---------------|
> > > | 0 | 0 | 15254600 | 152 | - |
> > > | 0 | 1 | 16136700 | 161 | - |
> > > | 0 | 2 | 15329500 | 153 | - |
> > > | 0 | 10 | 15148800 | 151 | - |
> > > | 0 | 100 | 15746900 | 157 | - |
> > > | 0 | 200 | 15737400 | 157 | - |
> > > |----------+------------+-----------------+------------+---------------|
> > > | 1 | 0 | 47909000 | 479 | 327 |
> > > | 1 | 1 | 48297400 | 482 | 330 |
> > > | 1 | 2 | 47314100 | 473 | 321 |
> > > | 1 | 10 | 47844900 | 478 | 326 |
> > > | 1 | 100 | 46591900 | 465 | 313 |
> > > | 1 | 200 | 47178900 | 471 | 319 |
> > > |----------+------------+-----------------+------------+---------------|
> > > | 1 | 0 | 46715800 | 467 | 315 |
> > > | 2 | 0 | 155134500 | 1551 | 1399 |
> > > | 10 | 0 | 442672800 | 4426 | 4274 |
> > > | 100 | 0 | 4092353900 | 40923 | 40771 |
> > > | 200 | 0 | 7135796400 | 71357 | 71205 |
> > > +----------+------------+-----------------+------------+---------------+
> > >
> > > Note: per-call overhead is estimated relative to the baseline case with
> > > 0 relevant tracers and 0 irrelevant tracers.
> > >
> > > As can be seen from the above:
> > >
> > > a) Whenever there is a single relevant tracer function associated with a
> > > tracee, the overhead of invoking the tracer is constant, and does not
> > > scale with the number of tracers which are *not* associated with that
> > > tracee.
> > >
> > > b) The overhead for a single relevant tracer has dropped to ~1/3 of the
> > > overhead prior to this series (from 1035ns to 315ns). This is largely
> > > due to permitting calls to dynamically-allocated ftrace_ops without
> > > going through ftrace_ops_list_func.
> > >
> > > Why is this patch a RFC patch:
> > > 1. I saw some rcu stalls on Qemu and need to debug them and see if they
> > > were introduced by this patch.
> >
> >
> > FYI, I'm currently working on debugging such issues (and other) with the
> > *current* ftrace implementation, so probably not caused by your
> > patchset. But keep debugging too, maybe this introduces other issues or
> > even better, you'll find the root cause :)
> >
> >
> > > 2. This needs to be tested thoroughly on real hardware.
> > > 3. Seeking reviews to fix any fundamental problems with this patch that I
> > > might have missed due to my lack of RISC-V architecture knowledge.
> > > 4. I would like to benchmark this on real hardware and put the results in
> > > the commit message.
> > >
> > > Signed-off-by: Puranjay Mohan <[email protected]>
> > > ---
> > > arch/riscv/Kconfig | 2 ++
> > > arch/riscv/Makefile | 8 +++++
> > > arch/riscv/include/asm/ftrace.h | 3 ++
> > > arch/riscv/kernel/asm-offsets.c | 3 ++
> > > arch/riscv/kernel/ftrace.c | 59 +++++++++++++++++++++++++++++++++
> > > arch/riscv/kernel/mcount-dyn.S | 42 ++++++++++++++++++++---
> > > 6 files changed, 112 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > index 0bfcfec67ed5..e474742e23b2 100644
> > > --- a/arch/riscv/Kconfig
> > > +++ b/arch/riscv/Kconfig
> > > @@ -78,6 +78,7 @@ config RISCV
> > > select EDAC_SUPPORT
> > > select FRAME_POINTER if PERF_EVENTS || (FUNCTION_TRACER && !DYNAMIC_FTRACE)
> > > select FTRACE_MCOUNT_USE_PATCHABLE_FUNCTION_ENTRY if DYNAMIC_FTRACE
> > > + select FUNCTION_ALIGNMENT_8B if DYNAMIC_FTRACE_WITH_CALL_OPS
> >
> >
> > A recent discussion [1] states that -falign-functions cannot guarantee
> > this alignment for all code and that gcc developers came up with a new
> > option [2]: WDYT? I have added Andy and Evgenii in +cc to help on that.
>
> I saw arm64 uses the same and assumes this guarantee, maybe it is broken too?
> Will look into the discussion and try to use the other option. I am
> currently using Clang to build the kernel.
IIRC it should be fine if you're building with Clang. The
"-falign-function" flag for gcc is not aligning cold functions. So I
think we'd need some Kconfig checks for the difference.
>
> >
> > [1]
> > https://lore.kernel.org/linux-riscv/[email protected]/
> > [2]
> > https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=0f5a9a00e3ab1fe96142f304cfbcf3f63b15f326
> >
> >
> > > select GENERIC_ARCH_TOPOLOGY
> > > select GENERIC_ATOMIC64 if !64BIT
> > > select GENERIC_CLOCKEVENTS_BROADCAST if SMP
> > > @@ -127,6 +128,7 @@ config RISCV
> > > select HAVE_DYNAMIC_FTRACE if !XIP_KERNEL && MMU && (CLANG_SUPPORTS_DYNAMIC_FTRACE || GCC_SUPPORTS_DYNAMIC_FTRACE)
> > > select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> > > select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE
> > > + select HAVE_DYNAMIC_FTRACE_WITH_CALL_OPS if (DYNAMIC_FTRACE_WITH_REGS && !CFI_CLANG)
> > > select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
> > > select HAVE_FUNCTION_GRAPH_TRACER
> > > select HAVE_FUNCTION_GRAPH_RETVAL if HAVE_FUNCTION_GRAPH_TRACER
> > > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> > > index 252d63942f34..875ad5dc3d32 100644
> > > --- a/arch/riscv/Makefile
> > > +++ b/arch/riscv/Makefile
> > > @@ -14,12 +14,20 @@ endif
> > > ifeq ($(CONFIG_DYNAMIC_FTRACE),y)
> > > LDFLAGS_vmlinux += --no-relax
> > > KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY
> > > +ifeq ($(CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS), y)
> > > +ifeq ($(CONFIG_RISCV_ISA_C),y)
> > > + CC_FLAGS_FTRACE := -fpatchable-function-entry=8,4
> > > +else
> > > + CC_FLAGS_FTRACE := -fpatchable-function-entry=4,2
> > > +endif
> > > +else
> > > ifeq ($(CONFIG_RISCV_ISA_C),y)
> > > CC_FLAGS_FTRACE := -fpatchable-function-entry=4
> > > else
> > > CC_FLAGS_FTRACE := -fpatchable-function-entry=2
> > > endif
> > > endif
> > > +endif
> > >
> > > ifeq ($(CONFIG_CMODEL_MEDLOW),y)
> > > KBUILD_CFLAGS_MODULE += -mcmodel=medany
> > > diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
> > > index 329172122952..c9a84222c9ea 100644
> > > --- a/arch/riscv/include/asm/ftrace.h
> > > +++ b/arch/riscv/include/asm/ftrace.h
> > > @@ -28,6 +28,9 @@
> > > void MCOUNT_NAME(void);
> > > static inline unsigned long ftrace_call_adjust(unsigned long addr)
> > > {
> > > + if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS))
> > > + return addr + 8;
> > > +
> > > return addr;
> > > }
> > >
> > > diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c
> > > index a03129f40c46..7d7c4b486852 100644
> > > --- a/arch/riscv/kernel/asm-offsets.c
> > > +++ b/arch/riscv/kernel/asm-offsets.c
> > > @@ -488,4 +488,7 @@ void asm_offsets(void)
> > > DEFINE(STACKFRAME_SIZE_ON_STACK, ALIGN(sizeof(struct stackframe), STACK_ALIGN));
> > > OFFSET(STACKFRAME_FP, stackframe, fp);
> > > OFFSET(STACKFRAME_RA, stackframe, ra);
> > > +#ifdef CONFIG_FUNCTION_TRACER
> > > + DEFINE(FTRACE_OPS_FUNC, offsetof(struct ftrace_ops, func));
> > > +#endif
> > > }
> > > diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
> > > index f5aa24d9e1c1..e2e75e15d32e 100644
> > > --- a/arch/riscv/kernel/ftrace.c
> > > +++ b/arch/riscv/kernel/ftrace.c
> > > @@ -82,9 +82,52 @@ static int __ftrace_modify_call(unsigned long hook_pos, unsigned long target,
> > > return 0;
> > > }
> > >
> > > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
> > > +static const struct ftrace_ops *riscv64_rec_get_ops(struct dyn_ftrace *rec)
> > > +{
> > > + const struct ftrace_ops *ops = NULL;
> > > +
> > > + if (rec->flags & FTRACE_FL_CALL_OPS_EN) {
> > > + ops = ftrace_find_unique_ops(rec);
> > > + WARN_ON_ONCE(!ops);
> > > + }
> > > +
> > > + if (!ops)
> > > + ops = &ftrace_list_ops;
> > > +
> > > + return ops;
> > > +}
> > > +
> > > +static int ftrace_rec_set_ops(const struct dyn_ftrace *rec,
> > > + const struct ftrace_ops *ops)
> > > +{
> > > + unsigned long literal = rec->ip - 8;
> > > +
> > > + return patch_text_nosync((void *)literal, &ops, sizeof(ops));
>
> ^^
> I will change this to use a new function that does a 64 bit write and
> doesn't do flush_icache_range() as naturally aligned writes are
> atomic and therefore synchronization is not required here.
Yes, it's atomic. But it doesn't guarantee ordering if fence.i is not
executed, IIUC. I am not sure if CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
assumes seeing the updated ftrace_rec argument right after the patch
completes. If that is the case then probably a batched fence.i is
needed on local + remote cores.
>
> > > +}
> > > +
> > > +static int ftrace_rec_set_nop_ops(struct dyn_ftrace *rec)
> > > +{
> > > + return ftrace_rec_set_ops(rec, &ftrace_nop_ops);
> > > +}
> > > +
> > > +static int ftrace_rec_update_ops(struct dyn_ftrace *rec)
> > > +{
> > > + return ftrace_rec_set_ops(rec, riscv64_rec_get_ops(rec));
> > > +}
> > > +#else
> > > +static int ftrace_rec_set_nop_ops(struct dyn_ftrace *rec) { return 0; }
> > > +static int ftrace_rec_update_ops(struct dyn_ftrace *rec) { return 0; }
> > > +#endif
> > > +
> > > int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
> > > {
> > > unsigned int call[2];
> > > + int ret;
> > > +
> > > + ret = ftrace_rec_update_ops(rec);
> > > + if (ret)
> > > + return ret;
> > >
> > > make_call_t0(rec->ip, addr, call);
> > >
> > > @@ -98,6 +141,11 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
> > > unsigned long addr)
> > > {
> > > unsigned int nops[2] = {NOP4, NOP4};
> > > + int ret;
> > > +
> > > + ret = ftrace_rec_set_nop_ops(rec);
> > > + if (ret)
> > > + return ret;
> > >
> > > if (patch_text_nosync((void *)rec->ip, nops, MCOUNT_INSN_SIZE))
> > > return -EPERM;
> > > @@ -125,6 +173,13 @@ int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> > >
> > > int ftrace_update_ftrace_func(ftrace_func_t func)
> > > {
> > > + /*
> > > + * When using CALL_OPS, the function to call is associated with the
> > > + * call site, and we don't have a global function pointer to update.
> > > + */
> > > + if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS))
> > > + return 0;
> > > +
> > > int ret = __ftrace_modify_call((unsigned long)&ftrace_call,
> > > (unsigned long)func, true, true);
> > > if (!ret) {
> > > @@ -147,6 +202,10 @@ int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
> > > make_call_t0(caller, old_addr, call);
> > > ret = ftrace_check_current_call(caller, call);
> > >
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = ftrace_rec_update_ops(rec);
> > > if (ret)
> > > return ret;
> > >
> > > diff --git a/arch/riscv/kernel/mcount-dyn.S b/arch/riscv/kernel/mcount-dyn.S
> > > index b7561288e8da..cb241e36e514 100644
> > > --- a/arch/riscv/kernel/mcount-dyn.S
> > > +++ b/arch/riscv/kernel/mcount-dyn.S
> > > @@ -191,11 +191,35 @@
> > > .endm
> > >
> > > .macro PREPARE_ARGS
> > > - addi a0, t0, -FENTRY_RA_OFFSET
> > > + addi a0, t0, -FENTRY_RA_OFFSET // ip (callsite's auipc insn)
> > > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
> > > + /*
> > > + * When CALL_OPS is enabled (2 or 4) nops [8B] are placed before the
> > > + * function entry, these are later overwritten with the pointer to the
> > > + * associated struct ftrace_ops.
> > > + *
> > > + * -8: &ftrace_ops of the associated tracer function.
> > > + *<ftrace enable>:
> > > + * 0: auipc t0/ra, 0x?
> > > + * 4: jalr t0/ra, ?(t0/ra)
> > > + *
> > > + * -8: &ftrace_nop_ops
> > > + *<ftrace disable>:
> > > + * 0: nop
> > > + * 4: nop
> > > + *
> > > + * t0 is set to ip+8 after the jalr is executed at the callsite,
> > > + * so we find the associated op at t0-16.
> > > + */
> > > + mv a1, ra // parent_ip
> > > + REG_L a2, -16(t0) // op
> > > + REG_L ra, FTRACE_OPS_FUNC(a2) // op->func
> > > +#else
> > > la a1, function_trace_op
> > > - REG_L a2, 0(a1)
> > > - mv a1, ra
> > > - mv a3, sp
> > > + REG_L a2, 0(a1) // op
> > > + mv a1, ra // parent_ip
> > > +#endif
> > > + mv a3, sp // regs
> > > .endm
> > >
> > > #endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
> > > @@ -233,8 +257,12 @@ SYM_FUNC_START(ftrace_regs_caller)
> > > SAVE_ABI_REGS 1
> > > PREPARE_ARGS
> > >
> > > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
> > > + jalr ra
> > > +#else
> > > SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
> > > call ftrace_stub
> > > +#endif
> > >
> > > RESTORE_ABI_REGS 1
> > > bnez t1, .Ldirect
> > > @@ -247,9 +275,13 @@ SYM_FUNC_START(ftrace_caller)
> > > SAVE_ABI_REGS 0
> > > PREPARE_ARGS
> > >
> > > -SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
>
> ^^ this hunk is a mistake, I will fix it in the next version.
>
> > > +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
> > > + jalr ra
> > > +#else
> > > +SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
> > > call ftrace_stub
> > >
> > > +#endif
> > > RESTORE_ABI_REGS 0
> > > jr t0
> > > SYM_FUNC_END(ftrace_caller)
> >
> >
> > As I'm diving into ftrace right now, I'll give a proper review soon. But
> > as a note, I noticed that the function_graph tracer, when enabled, makes
> > the whole system unresponsive (but still up, just very slow). A fix I
> > sent recently seems to really improve that if you're interested in
> > testing it (I am :)). You can find it here:
> > https://lore.kernel.org/linux-riscv/[email protected]/
>
> I saw the same issue where function_graph was making the system slow on Qemu.
> What hardware do you use for testing? or are you testing on Qemu as well?
>
> I tested your patch, it speeds up the process of patching the
> instructions so the following
> command completes ~2.5 seconds faster compared to without your patch.
> $ time echo function_graph > current_tracer
>
> But at runtime the system is still slow and laggy with function_graph,
> I guess because my
> Qemu setup is not powerful enough to run function_graph.
>
> Thanks,
> Puranjay
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Hi Puranjay,
On Fri, Mar 8, 2024 at 3:53 AM Puranjay Mohan <[email protected]> wrote:
>
> Hi Björn,
>
> On Thu, Mar 7, 2024 at 8:27 PM Björn Töpel <[email protected]> wrote:
> >
> > Puranjay!
> >
> > Puranjay Mohan <[email protected]> writes:
> >
> > > This patch enables support for DYNAMIC_FTRACE_WITH_CALL_OPS on RISC-V.
> > > This allows each ftrace callsite to provide an ftrace_ops to the common
> > > ftrace trampoline, allowing each callsite to invoke distinct tracer
> > > functions without the need to fall back to list processing or to
> > > allocate custom trampolines for each callsite. This significantly speeds
> > > up cases where multiple distinct trace functions are used and callsites
> > > are mostly traced by a single tracer.
> > >
> > > The idea and most of the implementation is taken from the ARM64's
> > > implementation of the same feature. The idea is to place a pointer to
> > > the ftrace_ops as a literal at a fixed offset from the function entry
> > > point, which can be recovered by the common ftrace trampoline.
> >
> > Not really a review, but some more background; Another rationale (on-top
> > of the improved per-call performance!) for CALL_OPS was to use it to
> > build ftrace direct call support (which BPF uses a lot!). Mark, please
> > correct me if I'm lying here!
> >
> > On Arm64, CALL_OPS makes it possible to implement direct calls, while
> > only patching one BL instruction -- nice!
> >
> > On RISC-V we cannot use use the same ideas as Arm64 straight off,
> > because the range of jal (compare to BL) is simply too short (+/-1M).
> > So, on RISC-V we need to use a full auipc/jal pair (the text patching
> > story is another chapter, but let's leave that aside for now). Since we
> > have to patch multiple instructions, the cmodx situation doesn't really
> > improve with CALL_OPS.
> >
> > Let's say that we continue building on your patch and implement direct
> > calls on CALL_OPS for RISC-V as well.
> >
> > From Florent's commit message for direct calls:
> >
> > | There are a few cases to distinguish:
> > | - If a direct call ops is the only one tracing a function:
> > | - If the direct called trampoline is within the reach of a BL
> > | instruction
> > | -> the ftrace patchsite jumps to the trampoline
> > | - Else
> > | -> the ftrace patchsite jumps to the ftrace_caller trampoline which
> > | reads the ops pointer in the patchsite and jumps to the direct
> > | call address stored in the ops
> > | - Else
> > | -> the ftrace patchsite jumps to the ftrace_caller trampoline and its
> > | ops literal points to ftrace_list_ops so it iterates over all
> > | registered ftrace ops, including the direct call ops and calls its
> > | call_direct_funcs handler which stores the direct called
> > | trampoline's address in the ftrace_regs and the ftrace_caller
> > | trampoline will return to that address instead of returning to the
> > | traced function
> >
> > On RISC-V, where auipc/jalr is used, the direct called trampoline would
> > always be reachable, and then first Else-clause would never be entered.
> > This means the the performance for direct calls would be the same as the
> > one we have today (i.e. no regression!).
> >
> > RISC-V does like x86 does (-ish) -- patch multiple instructions, long
> > reach.
> >
> > Arm64 uses CALL_OPS and patch one instruction BL.
> >
> > Now, with this background in mind, compared to what we have today,
> > CALL_OPS would give us (again assuming we're using it for direct calls):
> >
> > * Better performance for tracer per-call (faster ops lookup) GOOD
>
> ^ this was the only motivation for me to implement this patch.
>
> I don't think implementing direct calls over call ops is fruitful for
> RISC-V because once
> the auipc/jalr can be patched atomically, the direct call trampoline
> is always reachable.
Yes, the auipc/jalr instruction pair can be patched atomically just as
long as their size is naturally aligned on. However, we cannot prevent
2 instructions from being fetched atomically :P
> Solving the atomic text patching problem would be fun!! I am eager to
> see how it will be
> solved.
I have a patch series to solve the atomic code patching issue, which I
am about to respin [1]. The idea is to solve it with yet another layer
of indirection. We add a 8-B aligned space at each function entry. The
space is a pointer to the ftrace entry. During boot, each function
entry code is updated to perform a load and then take the jump from
the 8-B space. When ftrace is disabled, we patch the first 4B-aligned
instruction to a jump so as to skip the ftrace entry.
We are still discussing with Alex to see if we have a better way to do
it. If not then I'd update the patchset and re-send it. There's a
pending improvement in the series to reduce complexity. The 8-B
aligned space can be added before the function entry (just like your
patch).
>
> > * Larger text size (function alignment + extra nops) BAD
> > * Same direct call performance NEUTRAL
> > * Same complicated text patching required NEUTRAL
> >
> > It would be interesting to see how the per-call performance would
> > improve on x86 with CALL_OPS! ;-)
>
> If I remember from Steven's talk, x86 uses dynamically allocated trampolines
> for per callsite tracers, would CALL_OPS provide better performance than that?
>
> >
> > I'm trying to wrap my head if it makes sense to have it on RISC-V, given
> > that we're a bit different from Arm64. Does the scale tip to the GOOD
> > side?
> >
> > Oh, and we really need to see performance numbers on real HW! I have a
> > VF2 that I could try this series on.
>
> It would be great if you can do it :D.
>
> Thanks,
> Puranjay
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
- [1] https://yhbt.net/lore/all/CAJF2gTSn3_cDYsF9D8dt-br2Wf_M8y02A09xgRq8kXi91sN3Aw@mail.gmail.com/T/
Regards,
Andy
Puranjay Mohan <[email protected]> writes:
> Hi Björn,
>
> On Thu, Mar 7, 2024 at 8:27 PM Björn Töpel <[email protected]> wrote:
>>
>> Puranjay!
>>
>> Puranjay Mohan <[email protected]> writes:
>>
>> > This patch enables support for DYNAMIC_FTRACE_WITH_CALL_OPS on RISC-V.
>> > This allows each ftrace callsite to provide an ftrace_ops to the common
>> > ftrace trampoline, allowing each callsite to invoke distinct tracer
>> > functions without the need to fall back to list processing or to
>> > allocate custom trampolines for each callsite. This significantly speeds
>> > up cases where multiple distinct trace functions are used and callsites
>> > are mostly traced by a single tracer.
>> >
>> > The idea and most of the implementation is taken from the ARM64's
>> > implementation of the same feature. The idea is to place a pointer to
>> > the ftrace_ops as a literal at a fixed offset from the function entry
>> > point, which can be recovered by the common ftrace trampoline.
>>
>> Not really a review, but some more background; Another rationale (on-top
>> of the improved per-call performance!) for CALL_OPS was to use it to
>> build ftrace direct call support (which BPF uses a lot!). Mark, please
>> correct me if I'm lying here!
>>
>> On Arm64, CALL_OPS makes it possible to implement direct calls, while
>> only patching one BL instruction -- nice!
>>
>> On RISC-V we cannot use use the same ideas as Arm64 straight off,
>> because the range of jal (compare to BL) is simply too short (+/-1M).
>> So, on RISC-V we need to use a full auipc/jal pair (the text patching
>> story is another chapter, but let's leave that aside for now). Since we
>> have to patch multiple instructions, the cmodx situation doesn't really
>> improve with CALL_OPS.
>>
>> Let's say that we continue building on your patch and implement direct
>> calls on CALL_OPS for RISC-V as well.
>>
>> From Florent's commit message for direct calls:
>>
>> | There are a few cases to distinguish:
>> | - If a direct call ops is the only one tracing a function:
>> | - If the direct called trampoline is within the reach of a BL
>> | instruction
>> | -> the ftrace patchsite jumps to the trampoline
>> | - Else
>> | -> the ftrace patchsite jumps to the ftrace_caller trampoline which
>> | reads the ops pointer in the patchsite and jumps to the direct
>> | call address stored in the ops
>> | - Else
>> | -> the ftrace patchsite jumps to the ftrace_caller trampoline and its
>> | ops literal points to ftrace_list_ops so it iterates over all
>> | registered ftrace ops, including the direct call ops and calls its
>> | call_direct_funcs handler which stores the direct called
>> | trampoline's address in the ftrace_regs and the ftrace_caller
>> | trampoline will return to that address instead of returning to the
>> | traced function
>>
>> On RISC-V, where auipc/jalr is used, the direct called trampoline would
>> always be reachable, and then first Else-clause would never be entered.
>> This means the the performance for direct calls would be the same as the
>> one we have today (i.e. no regression!).
>>
>> RISC-V does like x86 does (-ish) -- patch multiple instructions, long
>> reach.
>>
>> Arm64 uses CALL_OPS and patch one instruction BL.
>>
>> Now, with this background in mind, compared to what we have today,
>> CALL_OPS would give us (again assuming we're using it for direct calls):
>>
>> * Better performance for tracer per-call (faster ops lookup) GOOD
>
> ^ this was the only motivation for me to implement this patch.
>
> I don't think implementing direct calls over call ops is fruitful for
> RISC-V because once
> the auipc/jalr can be patched atomically, the direct call trampoline
> is always reachable.
> Solving the atomic text patching problem would be fun!! I am eager to
> see how it will be
> solved.
Given the upcoming Zjid spec, we'll soon be in a much better place where
we can reason about cmodx.
>> * Larger text size (function alignment + extra nops) BAD
>> * Same direct call performance NEUTRAL
>> * Same complicated text patching required NEUTRAL
>>
>> It would be interesting to see how the per-call performance would
>> improve on x86 with CALL_OPS! ;-)
>
> If I remember from Steven's talk, x86 uses dynamically allocated trampolines
> for per callsite tracers, would CALL_OPS provide better performance than that?
Probably not, and it was really a tongue-in-cheek comment -- nothing I
encourage you to do!
Now, I think a better approach for RISC-V would be implementing what x86
has (arch_ftrace_update_trampoline()), rather than CALL_OPS for RISC-V.
Thoughts?
Björn
Hi Andy,
> >
> > I don't think implementing direct calls over call ops is fruitful for
> > RISC-V because once
> > the auipc/jalr can be patched atomically, the direct call trampoline
> > is always reachable.
>
> Yes, the auipc/jalr instruction pair can be patched atomically just as
> long as their size is naturally aligned on. However, we cannot prevent
> 2 instructions from being fetched atomically :P
>
> > Solving the atomic text patching problem would be fun!! I am eager to
> > see how it will be
> > solved.
>
> I have a patch series to solve the atomic code patching issue, which I
> am about to respin [1]. The idea is to solve it with yet another layer
> of indirection. We add a 8-B aligned space at each function entry. The
> space is a pointer to the ftrace entry. During boot, each function
> entry code is updated to perform a load and then take the jump from
> the 8-B space. When ftrace is disabled, we patch the first 4B-aligned
> instruction to a jump so as to skip the ftrace entry.
>
> We are still discussing with Alex to see if we have a better way to do
> it. If not then I'd update the patchset and re-send it. There's a
> pending improvement in the series to reduce complexity. The 8-B
> aligned space can be added before the function entry (just like your
> patch).
I briefly looked at the series and it looks promising. It looks similar to
how BPF programs jump to trampolines on ARM64. If the choice has to
be made about keeping the 8-B aligned space below or above the
function entry and both choices are viable then I would prefer keeping
the 8-B space below the function entry.
I am saying this because when nops are added above the function entry
clang's kcfi[1] doesn't work properly.
Thanks,
Puranjay
[1] https://clang.llvm.org/docs/ControlFlowIntegrity.html#fsanitize-kcfi
Hi Björn,
On Fri, Mar 8, 2024 at 11:16 AM Björn Töpel <bjorn@kernelorg> wrote:
> >
> > If I remember from Steven's talk, x86 uses dynamically allocated trampolines
> > for per callsite tracers, would CALL_OPS provide better performance than that?
>
> Probably not, and it was really a tongue-in-cheek comment -- nothing I
> encourage you to do!
>
> Now, I think a better approach for RISC-V would be implementing what x86
> has (arch_ftrace_update_trampoline()), rather than CALL_OPS for RISC-V.
>
> Thoughts?
I am going to spin some patches for implementing
arch_ftrace_update_trampoline() for
RISC-V, then we can compare the two approaches and see which is
better. But I agree
that arch_ftrace_update_trampoline() is a better approach given that
we can jump anywhere
with auipc/jalr.
Thanks,
Puranjay
Puranjay Mohan <[email protected]> writes:
>> Now, I think a better approach for RISC-V would be implementing what x86
>> has (arch_ftrace_update_trampoline()), rather than CALL_OPS for RISC-V.
>>
>> Thoughts?
>
> I am going to spin some patches for implementing
> arch_ftrace_update_trampoline() for
> RISC-V, then we can compare the two approaches and see which is
> better. But I agree
> that arch_ftrace_update_trampoline() is a better approach given that
> we can jump anywhere
> with auipc/jalr.
Yup, and the text size wont blow up.
Cheers,
Björn
On Fri, Mar 08, 2024 at 05:18:21PM +0800, Andy Chiu wrote:
> Hi Puranjay,
>
> On Fri, Mar 8, 2024 at 3:53 AM Puranjay Mohan <[email protected]> wrote:
> >
> > Hi Björn,
> >
> > On Thu, Mar 7, 2024 at 8:27 PM Björn Töpel <[email protected]> wrote:
> > >
> > > Puranjay!
> > >
> > > Puranjay Mohan <[email protected]> writes:
> > >
> > > > This patch enables support for DYNAMIC_FTRACE_WITH_CALL_OPS on RISC-V.
> > > > This allows each ftrace callsite to provide an ftrace_ops to the common
> > > > ftrace trampoline, allowing each callsite to invoke distinct tracer
> > > > functions without the need to fall back to list processing or to
> > > > allocate custom trampolines for each callsite. This significantly speeds
> > > > up cases where multiple distinct trace functions are used and callsites
> > > > are mostly traced by a single tracer.
> > > >
> > > > The idea and most of the implementation is taken from the ARM64's
> > > > implementation of the same feature. The idea is to place a pointer to
> > > > the ftrace_ops as a literal at a fixed offset from the function entry
> > > > point, which can be recovered by the common ftrace trampoline.
> > >
> > > Not really a review, but some more background; Another rationale (on-top
> > > of the improved per-call performance!) for CALL_OPS was to use it to
> > > build ftrace direct call support (which BPF uses a lot!). Mark, please
> > > correct me if I'm lying here!
> > >
> > > On Arm64, CALL_OPS makes it possible to implement direct calls, while
> > > only patching one BL instruction -- nice!
> > >
> > > On RISC-V we cannot use use the same ideas as Arm64 straight off,
> > > because the range of jal (compare to BL) is simply too short (+/-1M).
> > > So, on RISC-V we need to use a full auipc/jal pair (the text patching
> > > story is another chapter, but let's leave that aside for now). Since we
> > > have to patch multiple instructions, the cmodx situation doesn't really
> > > improve with CALL_OPS.
> > >
> > > Let's say that we continue building on your patch and implement direct
> > > calls on CALL_OPS for RISC-V as well.
> > >
> > > From Florent's commit message for direct calls:
> > >
> > > | There are a few cases to distinguish:
> > > | - If a direct call ops is the only one tracing a function:
> > > | - If the direct called trampoline is within the reach of a BL
> > > | instruction
> > > | -> the ftrace patchsite jumps to the trampoline
> > > | - Else
> > > | -> the ftrace patchsite jumps to the ftrace_caller trampoline which
> > > | reads the ops pointer in the patchsite and jumps to the direct
> > > | call address stored in the ops
> > > | - Else
> > > | -> the ftrace patchsite jumps to the ftrace_caller trampoline and its
> > > | ops literal points to ftrace_list_ops so it iterates over all
> > > | registered ftrace ops, including the direct call ops and calls its
> > > | call_direct_funcs handler which stores the direct called
> > > | trampoline's address in the ftrace_regs and the ftrace_caller
> > > | trampoline will return to that address instead of returning to the
> > > | traced function
> > >
> > > On RISC-V, where auipc/jalr is used, the direct called trampoline would
> > > always be reachable, and then first Else-clause would never be entered.
> > > This means the the performance for direct calls would be the same as the
> > > one we have today (i.e. no regression!).
> > >
> > > RISC-V does like x86 does (-ish) -- patch multiple instructions, long
> > > reach.
> > >
> > > Arm64 uses CALL_OPS and patch one instruction BL.
> > >
> > > Now, with this background in mind, compared to what we have today,
> > > CALL_OPS would give us (again assuming we're using it for direct calls):
> > >
> > > * Better performance for tracer per-call (faster ops lookup) GOOD
> >
> > ^ this was the only motivation for me to implement this patch.
> >
> > I don't think implementing direct calls over call ops is fruitful for
> > RISC-V because once
> > the auipc/jalr can be patched atomically, the direct call trampoline
> > is always reachable.
>
> Yes, the auipc/jalr instruction pair can be patched atomically just as
> long as their size is naturally aligned on. However, we cannot prevent
> 2 instructions from being fetched atomically :P
There are some micro-arch optimization methods here, such as:
- Disable interrupt when auipc retired.
- When auipc -> auipc, the second one still could cause an
interruption.
>
> > Solving the atomic text patching problem would be fun!! I am eager to
> > see how it will be
> > solved.
>
> I have a patch series to solve the atomic code patching issue, which I
> am about to respin [1]. The idea is to solve it with yet another layer
> of indirection. We add a 8-B aligned space at each function entry. The
> space is a pointer to the ftrace entry. During boot, each function
> entry code is updated to perform a load and then take the jump from
> the 8-B space. When ftrace is disabled, we patch the first 4B-aligned
> instruction to a jump so as to skip the ftrace entry.
>
> We are still discussing with Alex to see if we have a better way to do
> it. If not then I'd update the patchset and re-send it. There's a
> pending improvement in the series to reduce complexity. The 8-B
> aligned space can be added before the function entry (just like your
> patch).
>
> >
> > > * Larger text size (function alignment + extra nops) BAD
> > > * Same direct call performance NEUTRAL
> > > * Same complicated text patching required NEUTRAL
> > >
> > > It would be interesting to see how the per-call performance would
> > > improve on x86 with CALL_OPS! ;-)
> >
> > If I remember from Steven's talk, x86 uses dynamically allocated trampolines
> > for per callsite tracers, would CALL_OPS provide better performance than that?
> >
> > >
> > > I'm trying to wrap my head if it makes sense to have it on RISC-V, given
> > > that we're a bit different from Arm64. Does the scale tip to the GOOD
> > > side?
> > >
> > > Oh, and we really need to see performance numbers on real HW! I have a
> > > VF2 that I could try this series on.
> >
> > It would be great if you can do it :D.
> >
> > Thanks,
> > Puranjay
> >
> > _______________________________________________
> > linux-riscv mailing list
> > [email protected]
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>
> - [1] https://yhbt.net/lore/all/CAJF2gTSn3_cDYsF9D8dt-br2Wf_M8y02A09xgRq8kXi91sN3Aw@mail.gmail.com/T/
>
> Regards,
> Andy
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Hi,
On 07.03.2024 03:17, Puranjay Mohan wrote:
>
> Hi Alex,
>
> On Wed, Mar 6, 2024 at 9:35 PM Alexandre Ghiti <[email protected]> wrote:
>>
>> Hi Puranjay,
>>
>> On 06/03/2024 17:59, Puranjay Mohan wrote:
>>> This patch enables support for DYNAMIC_FTRACE_WITH_CALL_OPS on RISC-V.
>>> This allows each ftrace callsite to provide an ftrace_ops to the common
>>> ftrace trampoline, allowing each callsite to invoke distinct tracer
>>> functions without the need to fall back to list processing or to
>>> allocate custom trampolines for each callsite. This significantly speeds
>>> up cases where multiple distinct trace functions are used and callsites
>>> are mostly traced by a single tracer.
>>>
>>> The idea and most of the implementation is taken from the ARM64's
>>> implementation of the same feature. The idea is to place a pointer to
>>> the ftrace_ops as a literal at a fixed offset from the function entry
>>> point, which can be recovered by the common ftrace trampoline.
>>>
>>> We use -fpatchable-function-entry to reserve 8 bytes above the function
>>> entry by emitting 2 4 byte or 4 2 byte nops depending on the presence of
>>> CONFIG_RISCV_ISA_C. These 8 bytes are patched at runtime with a pointer
>>> to the associated ftrace_ops for that callsite. Functions are aligned to
>>> 8 bytes to make sure that the accesses to this literal are atomic.
>>>
>>> This approach allows for directly invoking ftrace_ops::func even for
>>> ftrace_ops which are dynamically-allocated (or part of a module),
>>> without going via ftrace_ops_list_func.
>>>
>>> I've benchamrked this with the ftrace_ops sample module on Qemu, with
>>> the next version, I will provide benchmarks on real hardware:
>>>
>>> Without this patch:
>>>
>>> +-----------------------+-----------------+----------------------------+
>>> | Number of tracers | Total time (ns) | Per-call average time |
>>> |-----------------------+-----------------+----------------------------|
>>> | Relevant | Irrelevant | 100000 calls | Total (ns) | Overhead (ns) |
>>> |----------+------------+-----------------+------------+---------------|
>>> | 0 | 0 | 15615700 | 156 | - |
>>> | 0 | 1 | 15917600 | 159 | - |
>>> | 0 | 2 | 15668000 | 156 | - |
>>> | 0 | 10 | 14971500 | 149 | - |
>>> | 0 | 100 | 15417600 | 154 | - |
>>> | 0 | 200 | 15387000 | 153 | - |
>>> |----------+------------+-----------------+------------+---------------|
>>> | 1 | 0 | 119906800 | 1199 | 1043 |
>>> | 1 | 1 | 137428600 | 1374 | 1218 |
>>> | 1 | 2 | 159562400 | 1374 | 1218 |
>>> | 1 | 10 | 302099900 | 3020 | 2864 |
>>> | 1 | 100 | 2008785500 | 20087 | 19931 |
>>> | 1 | 200 | 3965221900 | 39652 | 39496 |
>>> |----------+------------+-----------------+------------+---------------|
>>> | 1 | 0 | 119166700 | 1191 | 1035 |
>>> | 2 | 0 | 157999900 | 1579 | 1423 |
>>> | 10 | 0 | 425370100 | 4253 | 4097 |
>>> | 100 | 0 | 3595252100 | 35952 | 35796 |
>>> | 200 | 0 | 7023485700 | 70234 | 70078 |
>>> +----------+------------+-----------------+------------+---------------+
>>>
>>> Note: per-call overhead is estimated relative to the baseline case with
>>> 0 relevant tracers and 0 irrelevant tracers.
>>>
>>> With this patch:
>>>
>>> +-----------------------+-----------------+----------------------------+
>>> | Number of tracers | Total time (ns) | Per-call average time |
>>> |-----------------------+-----------------+----------------------------|
>>> | Relevant | Irrelevant | 100000 calls | Total (ns) | Overhead (ns) |
>>> |----------+------------+-----------------+------------+---------------|
>>> | 0 | 0 | 15254600 | 152 | - |
>>> | 0 | 1 | 16136700 | 161 | - |
>>> | 0 | 2 | 15329500 | 153 | - |
>>> | 0 | 10 | 15148800 | 151 | - |
>>> | 0 | 100 | 15746900 | 157 | - |
>>> | 0 | 200 | 15737400 | 157 | - |
>>> |----------+------------+-----------------+------------+---------------|
>>> | 1 | 0 | 47909000 | 479 | 327 |
>>> | 1 | 1 | 48297400 | 482 | 330 |
>>> | 1 | 2 | 47314100 | 473 | 321 |
>>> | 1 | 10 | 47844900 | 478 | 326 |
>>> | 1 | 100 | 46591900 | 465 | 313 |
>>> | 1 | 200 | 47178900 | 471 | 319 |
>>> |----------+------------+-----------------+------------+---------------|
>>> | 1 | 0 | 46715800 | 467 | 315 |
>>> | 2 | 0 | 155134500 | 1551 | 1399 |
>>> | 10 | 0 | 442672800 | 4426 | 4274 |
>>> | 100 | 0 | 4092353900 | 40923 | 40771 |
>>> | 200 | 0 | 7135796400 | 71357 | 71205 |
>>> +----------+------------+-----------------+------------+---------------+
>>>
>>> Note: per-call overhead is estimated relative to the baseline case with
>>> 0 relevant tracers and 0 irrelevant tracers.
>>>
>>> As can be seen from the above:
>>>
>>> a) Whenever there is a single relevant tracer function associated with a
>>> tracee, the overhead of invoking the tracer is constant, and does not
>>> scale with the number of tracers which are *not* associated with that
>>> tracee.
>>>
>>> b) The overhead for a single relevant tracer has dropped to ~1/3 of the
>>> overhead prior to this series (from 1035ns to 315ns). This is largely
>>> due to permitting calls to dynamically-allocated ftrace_ops without
>>> going through ftrace_ops_list_func.
>>>
>>> Why is this patch a RFC patch:
>>> 1. I saw some rcu stalls on Qemu and need to debug them and see if they
>>> were introduced by this patch.
>>
>>
>> FYI, I'm currently working on debugging such issues (and other) with the
>> *current* ftrace implementation, so probably not caused by your
>> patchset. But keep debugging too, maybe this introduces other issues or
>> even better, you'll find the root cause :)
>>
>>
>>> 2. This needs to be tested thoroughly on real hardware.
>>> 3. Seeking reviews to fix any fundamental problems with this patch that I
>>> might have missed due to my lack of RISC-V architecture knowledge.
>>> 4. I would like to benchmark this on real hardware and put the results in
>>> the commit message.
>>>
>>> Signed-off-by: Puranjay Mohan <[email protected]>
>>> ---
>>> arch/riscv/Kconfig | 2 ++
>>> arch/riscv/Makefile | 8 +++++
>>> arch/riscv/include/asm/ftrace.h | 3 ++
>>> arch/riscv/kernel/asm-offsets.c | 3 ++
>>> arch/riscv/kernel/ftrace.c | 59 +++++++++++++++++++++++++++++++++
>>> arch/riscv/kernel/mcount-dyn.S | 42 ++++++++++++++++++++---
>>> 6 files changed, 112 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>>> index 0bfcfec67ed5..e474742e23b2 100644
>>> --- a/arch/riscv/Kconfig
>>> +++ b/arch/riscv/Kconfig
>>> @@ -78,6 +78,7 @@ config RISCV
>>> select EDAC_SUPPORT
>>> select FRAME_POINTER if PERF_EVENTS || (FUNCTION_TRACER && !DYNAMIC_FTRACE)
>>> select FTRACE_MCOUNT_USE_PATCHABLE_FUNCTION_ENTRY if DYNAMIC_FTRACE
>>> + select FUNCTION_ALIGNMENT_8B if DYNAMIC_FTRACE_WITH_CALL_OPS
>>
>>
>> A recent discussion [1] states that -falign-functions cannot guarantee
>> this alignment for all code and that gcc developers came up with a new
>> option [2]: WDYT? I have added Andy and Evgenii in +cc to help on that.
>
Yes, the recent GCC supports -fmin-function-alignment option which makes
sure all functions are properly aligned.
I used the following patch on top of Andy's patchset [3]:
-----------
diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
index b33b787c8b07..b831ae62672d 100644
--- a/arch/riscv/Makefile
+++ b/arch/riscv/Makefile
@@ -14,10 +14,15 @@ endif
ifeq ($(CONFIG_DYNAMIC_FTRACE),y)
LDFLAGS_vmlinux += --no-relax
KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY
+ifeq ($(CONFIG_CC_IS_GCC),y)
+ CC_FLAGS_FTRACE := -fmin-function-alignment=4
+else
+ CC_FLAGS_FTRACE := -falign-functions=4
+endif
ifeq ($(CONFIG_RISCV_ISA_C),y)
- CC_FLAGS_FTRACE := -fpatchable-function-entry=12 -falign-functions=4
+ CC_FLAGS_FTRACE += -fpatchable-function-entry=12
else
- CC_FLAGS_FTRACE := -fpatchable-function-entry=6 -falign-functions=4
+ CC_FLAGS_FTRACE += -fpatchable-function-entry=6
endif
endif
-----------
We probably need something like this in arch/riscv/Kconfig as well, to
allow dynamic Ftrace only if GCC supports -fmin-function-alignment:
-----------
config GCC_SUPPORTS_DYNAMIC_FTRACE
def_bool CC_IS_GCC
- depends on $(cc-option,-fpatchable-function-entry=8)
+ depends on $(cc-option,-fpatchable-function-entry=6)
+ depends on $(cc-option,-fmin-function-alignment=4)
-----------
> I saw arm64 uses the same and assumes this guarantee, maybe it is broken too?
> Will look into the discussion and try to use the other option. I am
> currently using Clang to build the kernel.
I tried dynamic Ftrace with clang too (LLVM 17.0.6 built from the
official RISC-V toolchain). It seems, clang has no problems with
alignment of functions, unlike GCC, and 'falign-functions=4' is enough
to align everything properly.
However, there is a serious issue with clang and dynamic Ftrace: [4].
In short: for static functions with 9 or more arguments, like
__find_rr_leaf
(https://elixir.bootlin.com/linux/v6.8-rc4/source/net/ipv6/route.c#L786),
clang may switch to "fastcall" calling convention. This way, the 9th
argument will be passed in the register t2, the 10th - in t3, etc.,
rather than on the stack like RISC-V ABI requires.
This causes kernel craches when trying to enable "function",
"function_graph" and other tracers for such functions. The
implementation of Ftrace assumes that t0 - t3 can be safely clobbered at
the beginning of the function. clang assumes otherwise.
Unless a compiler option or something to disable switching to fastcall
convention is added (there is currently no such option in clang, it
seems), dynamic Ftrace cannot be used safely with clang-built kernels.
>
>>
>> [1]
>> https://lore.kernel.org/linux-riscv/[email protected]/
>> [2]
>> https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=0f5a9a00e3ab1fe96142f304cfbcf3f63b15f326
>>
>>
>>> select GENERIC_ARCH_TOPOLOGY
>>> select GENERIC_ATOMIC64 if !64BIT
>>> select GENERIC_CLOCKEVENTS_BROADCAST if SMP
>>> @@ -127,6 +128,7 @@ config RISCV
>>> select HAVE_DYNAMIC_FTRACE if !XIP_KERNEL && MMU && (CLANG_SUPPORTS_DYNAMIC_FTRACE || GCC_SUPPORTS_DYNAMIC_FTRACE)
>>> select HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
>>> select HAVE_DYNAMIC_FTRACE_WITH_REGS if HAVE_DYNAMIC_FTRACE
>>> + select HAVE_DYNAMIC_FTRACE_WITH_CALL_OPS if (DYNAMIC_FTRACE_WITH_REGS && !CFI_CLANG)
>>> select HAVE_FTRACE_MCOUNT_RECORD if !XIP_KERNEL
>>> select HAVE_FUNCTION_GRAPH_TRACER
>>> select HAVE_FUNCTION_GRAPH_RETVAL if HAVE_FUNCTION_GRAPH_TRACER
>>> diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
>>> index 252d63942f34..875ad5dc3d32 100644
>>> --- a/arch/riscv/Makefile
>>> +++ b/arch/riscv/Makefile
>>> @@ -14,12 +14,20 @@ endif
>>> ifeq ($(CONFIG_DYNAMIC_FTRACE),y)
>>> LDFLAGS_vmlinux += --no-relax
>>> KBUILD_CPPFLAGS += -DCC_USING_PATCHABLE_FUNCTION_ENTRY
>>> +ifeq ($(CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS), y)
>>> +ifeq ($(CONFIG_RISCV_ISA_C),y)
>>> + CC_FLAGS_FTRACE := -fpatchable-function-entry=8,4
>>> +else
>>> + CC_FLAGS_FTRACE := -fpatchable-function-entry=4,2
>>> +endif
>>> +else
>>> ifeq ($(CONFIG_RISCV_ISA_C),y)
>>> CC_FLAGS_FTRACE := -fpatchable-function-entry=4
>>> else
>>> CC_FLAGS_FTRACE := -fpatchable-function-entry=2
>>> endif
>>> endif
>>> +endif
>>>
>>> ifeq ($(CONFIG_CMODEL_MEDLOW),y)
>>> KBUILD_CFLAGS_MODULE += -mcmodel=medany
>>> diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
>>> index 329172122952..c9a84222c9ea 100644
>>> --- a/arch/riscv/include/asm/ftrace.h
>>> +++ b/arch/riscv/include/asm/ftrace.h
>>> @@ -28,6 +28,9 @@
>>> void MCOUNT_NAME(void);
>>> static inline unsigned long ftrace_call_adjust(unsigned long addr)
>>> {
>>> + if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS))
>>> + return addr + 8;
>>> +
>>> return addr;
>>> }
>>>
>>> diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c
>>> index a03129f40c46..7d7c4b486852 100644
>>> --- a/arch/riscv/kernel/asm-offsets.c
>>> +++ b/arch/riscv/kernel/asm-offsets.c
>>> @@ -488,4 +488,7 @@ void asm_offsets(void)
>>> DEFINE(STACKFRAME_SIZE_ON_STACK, ALIGN(sizeof(struct stackframe), STACK_ALIGN));
>>> OFFSET(STACKFRAME_FP, stackframe, fp);
>>> OFFSET(STACKFRAME_RA, stackframe, ra);
>>> +#ifdef CONFIG_FUNCTION_TRACER
>>> + DEFINE(FTRACE_OPS_FUNC, offsetof(struct ftrace_ops, func));
>>> +#endif
>>> }
>>> diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
>>> index f5aa24d9e1c1..e2e75e15d32e 100644
>>> --- a/arch/riscv/kernel/ftrace.c
>>> +++ b/arch/riscv/kernel/ftrace.c
>>> @@ -82,9 +82,52 @@ static int __ftrace_modify_call(unsigned long hook_pos, unsigned long target,
>>> return 0;
>>> }
>>>
>>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
>>> +static const struct ftrace_ops *riscv64_rec_get_ops(struct dyn_ftrace *rec)
>>> +{
>>> + const struct ftrace_ops *ops = NULL;
>>> +
>>> + if (rec->flags & FTRACE_FL_CALL_OPS_EN) {
>>> + ops = ftrace_find_unique_ops(rec);
>>> + WARN_ON_ONCE(!ops);
>>> + }
>>> +
>>> + if (!ops)
>>> + ops = &ftrace_list_ops;
>>> +
>>> + return ops;
>>> +}
>>> +
>>> +static int ftrace_rec_set_ops(const struct dyn_ftrace *rec,
>>> + const struct ftrace_ops *ops)
>>> +{
>>> + unsigned long literal = rec->ip - 8;
>>> +
>>> + return patch_text_nosync((void *)literal, &ops, sizeof(ops));
>
> ^^
> I will change this to use a new function that does a 64 bit write and
> doesn't do flush_icache_range() as naturally aligned writes are
> atomic and therefore synchronization is not required here.
>
>>> +}
>>> +
>>> +static int ftrace_rec_set_nop_ops(struct dyn_ftrace *rec)
>>> +{
>>> + return ftrace_rec_set_ops(rec, &ftrace_nop_ops);
>>> +}
>>> +
>>> +static int ftrace_rec_update_ops(struct dyn_ftrace *rec)
>>> +{
>>> + return ftrace_rec_set_ops(rec, riscv64_rec_get_ops(rec));
>>> +}
>>> +#else
>>> +static int ftrace_rec_set_nop_ops(struct dyn_ftrace *rec) { return 0; }
>>> +static int ftrace_rec_update_ops(struct dyn_ftrace *rec) { return 0; }
>>> +#endif
>>> +
>>> int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
>>> {
>>> unsigned int call[2];
>>> + int ret;
>>> +
>>> + ret = ftrace_rec_update_ops(rec);
>>> + if (ret)
>>> + return ret;
>>>
>>> make_call_t0(rec->ip, addr, call);
>>>
>>> @@ -98,6 +141,11 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
>>> unsigned long addr)
>>> {
>>> unsigned int nops[2] = {NOP4, NOP4};
>>> + int ret;
>>> +
>>> + ret = ftrace_rec_set_nop_ops(rec);
>>> + if (ret)
>>> + return ret;
>>>
>>> if (patch_text_nosync((void *)rec->ip, nops, MCOUNT_INSN_SIZE))
>>> return -EPERM;
>>> @@ -125,6 +173,13 @@ int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
>>>
>>> int ftrace_update_ftrace_func(ftrace_func_t func)
>>> {
>>> + /*
>>> + * When using CALL_OPS, the function to call is associated with the
>>> + * call site, and we don't have a global function pointer to update.
>>> + */
>>> + if (IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS))
>>> + return 0;
>>> +
>>> int ret = __ftrace_modify_call((unsigned long)&ftrace_call,
>>> (unsigned long)func, true, true);
>>> if (!ret) {
>>> @@ -147,6 +202,10 @@ int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
>>> make_call_t0(caller, old_addr, call);
>>> ret = ftrace_check_current_call(caller, call);
>>>
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = ftrace_rec_update_ops(rec);
>>> if (ret)
>>> return ret;
>>>
>>> diff --git a/arch/riscv/kernel/mcount-dyn.S b/arch/riscv/kernel/mcount-dyn.S
>>> index b7561288e8da..cb241e36e514 100644
>>> --- a/arch/riscv/kernel/mcount-dyn.S
>>> +++ b/arch/riscv/kernel/mcount-dyn.S
>>> @@ -191,11 +191,35 @@
>>> .endm
>>>
>>> .macro PREPARE_ARGS
>>> - addi a0, t0, -FENTRY_RA_OFFSET
>>> + addi a0, t0, -FENTRY_RA_OFFSET // ip (callsite's auipc insn)
>>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
>>> + /*
>>> + * When CALL_OPS is enabled (2 or 4) nops [8B] are placed before the
>>> + * function entry, these are later overwritten with the pointer to the
>>> + * associated struct ftrace_ops.
>>> + *
>>> + * -8: &ftrace_ops of the associated tracer function.
>>> + *<ftrace enable>:
>>> + * 0: auipc t0/ra, 0x?
>>> + * 4: jalr t0/ra, ?(t0/ra)
>>> + *
>>> + * -8: &ftrace_nop_ops
>>> + *<ftrace disable>:
>>> + * 0: nop
>>> + * 4: nop
>>> + *
>>> + * t0 is set to ip+8 after the jalr is executed at the callsite,
>>> + * so we find the associated op at t0-16.
>>> + */
>>> + mv a1, ra // parent_ip
>>> + REG_L a2, -16(t0) // op
>>> + REG_L ra, FTRACE_OPS_FUNC(a2) // op->func
>>> +#else
>>> la a1, function_trace_op
>>> - REG_L a2, 0(a1)
>>> - mv a1, ra
>>> - mv a3, sp
>>> + REG_L a2, 0(a1) // op
>>> + mv a1, ra // parent_ip
>>> +#endif
>>> + mv a3, sp // regs
>>> .endm
>>>
>>> #endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
>>> @@ -233,8 +257,12 @@ SYM_FUNC_START(ftrace_regs_caller)
>>> SAVE_ABI_REGS 1
>>> PREPARE_ARGS
>>>
>>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
>>> + jalr ra
>>> +#else
>>> SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
>>> call ftrace_stub
>>> +#endif
>>>
>>> RESTORE_ABI_REGS 1
>>> bnez t1, .Ldirect
>>> @@ -247,9 +275,13 @@ SYM_FUNC_START(ftrace_caller)
>>> SAVE_ABI_REGS 0
>>> PREPARE_ARGS
>>>
>>> -SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
>
> ^^ this hunk is a mistake, I will fix it in the next version.
>
>>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_CALL_OPS
>>> + jalr ra
>>> +#else
>>> +SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
>>> call ftrace_stub
>>>
>>> +#endif
>>> RESTORE_ABI_REGS 0
>>> jr t0
>>> SYM_FUNC_END(ftrace_caller)
>>
>>
>> As I'm diving into ftrace right now, I'll give a proper review soon. But
>> as a note, I noticed that the function_graph tracer, when enabled, makes
>> the whole system unresponsive (but still up, just very slow). A fix I
>> sent recently seems to really improve that if you're interested in
>> testing it (I am :)). You can find it here:
>> https://lore.kernel.org/linux-riscv/[email protected]/
>
> I saw the same issue where function_graph was making the system slow on Qemu.
> What hardware do you use for testing? or are you testing on Qemu as well?
>
> I tested your patch, it speeds up the process of patching the
> instructions so the following
> command completes ~2.5 seconds faster compared to without your patch.
> $ time echo function_graph > current_tracer
>
> But at runtime the system is still slow and laggy with function_graph,
> I guess because my
> Qemu setup is not powerful enough to run function_graph.
>
> Thanks,
> Puranjay
>
[3]
https://lore.kernel.org/all/CAJF2gTRc487qEme5tQw4ich5xuf8_DYhTB8fp=TXav3Gs4KWnQ@mail.gmail.com/T/#mdba180cfd0c9a6df3da49e9c330649ce5151a24a
[4] https://github.com/llvm/llvm-project/issues/83111 - "RISC-V ABI
break: x7/t2 register is used to pass the 9th argument to a function in
certain cases"
Regards,
Evgenii
Hi Bjorn
(apologies, my corporate mail server has butchered your name here).
There's a big info dump below; I realise this sounds like a sales pitch for
CALL_OPS, but my intent is more to say "here are some dragons you may not have
spotted".
On Thu, Mar 07, 2024 at 08:27:40PM +0100, Bj"orn T"opel wrote:
> Puranjay!
>
> Puranjay Mohan <[email protected]> writes:
>
> > This patch enables support for DYNAMIC_FTRACE_WITH_CALL_OPS on RISC-V.
> > This allows each ftrace callsite to provide an ftrace_ops to the common
> > ftrace trampoline, allowing each callsite to invoke distinct tracer
> > functions without the need to fall back to list processing or to
> > allocate custom trampolines for each callsite. This significantly speeds
> > up cases where multiple distinct trace functions are used and callsites
> > are mostly traced by a single tracer.
> >
> > The idea and most of the implementation is taken from the ARM64's
> > implementation of the same feature. The idea is to place a pointer to
> > the ftrace_ops as a literal at a fixed offset from the function entry
> > point, which can be recovered by the common ftrace trampoline.
>
> Not really a review, but some more background; Another rationale (on-top
> of the improved per-call performance!) for CALL_OPS was to use it to
> build ftrace direct call support (which BPF uses a lot!). Mark, please
> correct me if I'm lying here!
Yep; it gives us the ability to do a number of per-callsite things, including
direct calls.
> On Arm64, CALL_OPS makes it possible to implement direct calls, while
> only patching one BL instruction -- nice!
The key thing here isn't that we patch a single instruction (since we have ot
patch the ops pointer too!); it's that we can safely patch either of the ops
pointer or BL/NOP at any time while threads are concurrently executing.
If you have a multi-instruction sequence, then threads can be preempted
mid-sequence, and it's very painful/complex to handle all of the races that
entails.
For example, if your callsites use a sequence:
AUIPC <tmp>, <funcptr>
JALR <tmp2>, <funcptr>(<tmp>)
Using stop_machine() won't allow you to patch that safely as some threads
could be stuck mid-sequence, e.g.
AUIPC <tmp>, <funcptr>
[ preempted here ]
JALR <tmp2>, <funcptr>(<tmp>)
.. and you can't update the JALR to use a new funcptr immediate until those
have completed the sequence.
There are ways around that, but they're complicated and/or expensive, e.g.
* Use a sequence of multiple patches, starting with replacing the JALR with an
exception-generating instruction with a fixup handler, which is sort-of what
x86 does with UD2. This may require multiple passes with
synchronize_rcu_tasks() to make sure all threads have seen the latest
instructions, and that cannot be done under stop_machine(), so if you need
stop_machine() for CMODx reasons, you may need to use that several times with
intervening calls to synchronize_rcu_tasks().
* Have the patching logic manually go over each thread and fix up the pt_regs
for the interrupted thread. This is pretty horrid since you could have nested
exceptions and a task could have several pt_regs which might require
updating.
The CALL_OPS approach is a bit easier to deal with as we can patch the
per-callsite pointer atomically, then we can (possibly) enable/disable the
callsite's branch, then wait for threads to drain once.
As a heads-up, there are some latent/generic issues with DYNAMIC_FTRACE
generally in this area (CALL_OPs happens to side-step those, but trampoline
usage is currently affected):
https://lore.kernel.org/lkml/Zenx_Q0UiwMbSAdP@FVFF77S0Q05N/
.. I'm looking into fixing that at the moment, and it looks like that's likely
to require some per-architecture changes.
> On RISC-V we cannot use use the same ideas as Arm64 straight off,
> because the range of jal (compare to BL) is simply too short (+/-1M).
> So, on RISC-V we need to use a full auipc/jal pair (the text patching
> story is another chapter, but let's leave that aside for now). Since we
> have to patch multiple instructions, the cmodx situation doesn't really
> improve with CALL_OPS.
The branch range thing is annoying, but I think this boils down to the same
problem as arm64 has with needing a "MOV <tmp>, LR" instruction that we have to
patch in once at boot time. You could do the same and patch in the AUIPC once,
e.g. have
| NOP
| NOP
| func:
| AUIPC <tmp>, <common_ftrace_caller>
| JALR <tmp2>, <common_ftrace_caller>(<tmp>) // patched with NOP
.. which'd look very similar to arm64's sequence:
| NOP
| NOP
| func:
| MOV X9, LR
| BL ftrace_caller // patched with NOP
.. which I think means it *might* be better from a cmodx perspective?
> Let's say that we continue building on your patch and implement direct
> calls on CALL_OPS for RISC-V as well.
>
> From Florent's commit message for direct calls:
>
> | There are a few cases to distinguish:
> | - If a direct call ops is the only one tracing a function:
> | - If the direct called trampoline is within the reach of a BL
> | instruction
> | -> the ftrace patchsite jumps to the trampoline
> | - Else
> | -> the ftrace patchsite jumps to the ftrace_caller trampoline which
> | reads the ops pointer in the patchsite and jumps to the direct
> | call address stored in the ops
> | - Else
> | -> the ftrace patchsite jumps to the ftrace_caller trampoline and its
> | ops literal points to ftrace_list_ops so it iterates over all
> | registered ftrace ops, including the direct call ops and calls its
> | call_direct_funcs handler which stores the direct called
> | trampoline's address in the ftrace_regs and the ftrace_caller
> | trampoline will return to that address instead of returning to the
> | traced function
>
> On RISC-V, where auipc/jalr is used, the direct called trampoline would
> always be reachable, and then first Else-clause would never be entered.
> This means the the performance for direct calls would be the same as the
> one we have today (i.e. no regression!).
>
> RISC-V does like x86 does (-ish) -- patch multiple instructions, long
> reach.
>
> Arm64 uses CALL_OPS and patch one instruction BL.
>
> Now, with this background in mind, compared to what we have today,
> CALL_OPS would give us (again assuming we're using it for direct calls):
>
> * Better performance for tracer per-call (faster ops lookup) GOOD
> * Larger text size (function alignment + extra nops) BAD
> * Same direct call performance NEUTRAL
> * Same complicated text patching required NEUTRAL
Is your current sequence safe for preemptible kernels (i.e. with PREEMPT_FULL=y
or PREEMPT_DYNAMIC=y + "preempt=full" on the kernel cmdline) ?
Looking at v6.8, IIUC you have:
// unpatched //patched
NOP AUIPC
NOP JALR
What prevents a thread being preempted mid-sequence such that it executes:
NOP
[ preempted ]
[ stop_machine() used to patch text ]
[ restored ]
JALR
.. ?
.. or when changing the call:
AUIPC // old funcptr
[ preempted ]
[ stop_machine() used to patch text ]
[ restored ]
JALR // new funcptr
.. ?
I suspect those might both be broken, but it's difficult to hit the race and so
testing hasn't revealed that so far.
> It would be interesting to see how the per-call performance would
> improve on x86 with CALL_OPS! ;-)
Heh. ;)
> I'm trying to wrap my head if it makes sense to have it on RISC-V, given
> that we're a bit different from Arm64. Does the scale tip to the GOOD
> side?
>
> Oh, and we really need to see performance numbers on real HW! I have a
> VF2 that I could try this series on.
I'll have to leave those two to you. I suspect the preempt/stop_machine()
might just tip that from NEUTRAL to GOOD.
Mark.
Mark Rutland <[email protected]> writes:
> Hi Bjorn
>
> (apologies, my corporate mail server has butchered your name here).
Ha! That's the price I have to pay for carrying double-umlauts
everywhere. Thanks for getting back with a really useful answer!
>> On Arm64, CALL_OPS makes it possible to implement direct calls, while
>> only patching one BL instruction -- nice!
>
> The key thing here isn't that we patch a single instruction (since we have ot
> patch the ops pointer too!); it's that we can safely patch either of the ops
> pointer or BL/NOP at any time while threads are concurrently executing.
..which indeed is a very nice property!
> If you have a multi-instruction sequence, then threads can be preempted
> mid-sequence, and it's very painful/complex to handle all of the races that
> entails.
Oh yes. RISC-V is currently using auipc/jalr with stop_machine(), and
also requires that preemtion is off. Unusable to put it blunt.
> For example, if your callsites use a sequence:
>
> AUIPC <tmp>, <funcptr>
> JALR <tmp2>, <funcptr>(<tmp>)
>
> Using stop_machine() won't allow you to patch that safely as some threads
> could be stuck mid-sequence, e.g.
>
> AUIPC <tmp>, <funcptr>
> [ preempted here ]
> JALR <tmp2>, <funcptr>(<tmp>)
>
> ... and you can't update the JALR to use a new funcptr immediate until those
> have completed the sequence.
>
> There are ways around that, but they're complicated and/or expensive, e.g.
>
> * Use a sequence of multiple patches, starting with replacing the JALR with an
> exception-generating instruction with a fixup handler, which is sort-of what
> x86 does with UD2. This may require multiple passes with
> synchronize_rcu_tasks() to make sure all threads have seen the latest
> instructions, and that cannot be done under stop_machine(), so if you need
> stop_machine() for CMODx reasons, you may need to use that several times with
> intervening calls to synchronize_rcu_tasks().
>
> * Have the patching logic manually go over each thread and fix up the pt_regs
> for the interrupted thread. This is pretty horrid since you could have nested
> exceptions and a task could have several pt_regs which might require
> updating.
Yup, and both of these have rather unplesant overhead.
> The CALL_OPS approach is a bit easier to deal with as we can patch the
> per-callsite pointer atomically, then we can (possibly) enable/disable the
> callsite's branch, then wait for threads to drain once.
>
> As a heads-up, there are some latent/generic issues with DYNAMIC_FTRACE
> generally in this area (CALL_OPs happens to side-step those, but trampoline
> usage is currently affected):
>
> https://lore.kernel.org/lkml/Zenx_Q0UiwMbSAdP@FVFF77S0Q05N/
>
> ... I'm looking into fixing that at the moment, and it looks like that's likely
> to require some per-architecture changes.
>
>> On RISC-V we cannot use use the same ideas as Arm64 straight off,
>> because the range of jal (compare to BL) is simply too short (+/-1M).
>> So, on RISC-V we need to use a full auipc/jal pair (the text patching
>> story is another chapter, but let's leave that aside for now). Since we
>> have to patch multiple instructions, the cmodx situation doesn't really
>> improve with CALL_OPS.
>
> The branch range thing is annoying, but I think this boils down to the same
> problem as arm64 has with needing a "MOV <tmp>, LR" instruction that we have to
> patch in once at boot time. You could do the same and patch in the AUIPC once,
> e.g. have
>
> | NOP
> | NOP
> | func:
> | AUIPC <tmp>, <common_ftrace_caller>
> | JALR <tmp2>, <common_ftrace_caller>(<tmp>) // patched with NOP
>
> ... which'd look very similar to arm64's sequence:
>
> | NOP
> | NOP
> | func:
> | MOV X9, LR
> | BL ftrace_caller // patched with NOP
>
> ... which I think means it *might* be better from a cmodx perspective?
>
>> Let's say that we continue building on your patch and implement direct
>> calls on CALL_OPS for RISC-V as well.
>>
>> From Florent's commit message for direct calls:
>>
>> | There are a few cases to distinguish:
>> | - If a direct call ops is the only one tracing a function:
>> | - If the direct called trampoline is within the reach of a BL
>> | instruction
>> | -> the ftrace patchsite jumps to the trampoline
>> | - Else
>> | -> the ftrace patchsite jumps to the ftrace_caller trampoline which
>> | reads the ops pointer in the patchsite and jumps to the direct
>> | call address stored in the ops
>> | - Else
>> | -> the ftrace patchsite jumps to the ftrace_caller trampoline and its
>> | ops literal points to ftrace_list_ops so it iterates over all
>> | registered ftrace ops, including the direct call ops and calls its
>> | call_direct_funcs handler which stores the direct called
>> | trampoline's address in the ftrace_regs and the ftrace_caller
>> | trampoline will return to that address instead of returning to the
>> | traced function
>>
>> On RISC-V, where auipc/jalr is used, the direct called trampoline would
>> always be reachable, and then first Else-clause would never be entered.
>> This means the the performance for direct calls would be the same as the
>> one we have today (i.e. no regression!).
>>
>> RISC-V does like x86 does (-ish) -- patch multiple instructions, long
>> reach.
>>
>> Arm64 uses CALL_OPS and patch one instruction BL.
>>
>> Now, with this background in mind, compared to what we have today,
>> CALL_OPS would give us (again assuming we're using it for direct calls):
>>
>> * Better performance for tracer per-call (faster ops lookup) GOOD
>> * Larger text size (function alignment + extra nops) BAD
>> * Same direct call performance NEUTRAL
>> * Same complicated text patching required NEUTRAL
>
> Is your current sequence safe for preemptible kernels (i.e. with PREEMPT_FULL=y
> or PREEMPT_DYNAMIC=y + "preempt=full" on the kernel cmdline) ?
It's very much not, and was in-fact presented by Andy (Cc) discussed at
length at Plumbers two years back.
Hmm, depending on RISC-V's CMODX path, the pro/cons CALL_OPS vs dynamic
trampolines changes quite a bit.
The more I look at the pains of patching two instruction ("split
immediates"), the better "patch data" + one insn patching look.
I which we had longer instructions, that could fit a 48b address or
more! ;-)
Again, thanks for a thought provoking reply!
Björn
Björn Töpel <[email protected]> writes:
>
> Hmm, depending on RISC-V's CMODX path, the pro/cons CALL_OPS vs dynamic
> trampolines changes quite a bit.
>
> The more I look at the pains of patching two instruction ("split
> immediates"), the better "patch data" + one insn patching look.
I was looking at how dynamic trampolines would be implemented for RISC-V.
With CALL-OPS we need to patch the auipc+jalr at function entry only, the
ops pointer above the function can be patched atomically.
With a dynamic trampoline we need a auipc+jalr pair at function entry to jump
to the trampoline and then another auipc+jalr pair to jump from trampoline to
ops->func. When the ops->func is modified, we would need to update the
auipc+jalr at in the trampoline.
So, I am not sure how to move forward here, CALL-OPS or Dynamic trampolines?
Thanks,
Puranjay
Puranjay Mohan <[email protected]> writes:
> Björn Töpel <[email protected]> writes:
>
>>
>> Hmm, depending on RISC-V's CMODX path, the pro/cons CALL_OPS vs dynamic
>> trampolines changes quite a bit.
>>
>> The more I look at the pains of patching two instruction ("split
>> immediates"), the better "patch data" + one insn patching look.
>
> I was looking at how dynamic trampolines would be implemented for RISC-V.
>
> With CALL-OPS we need to patch the auipc+jalr at function entry only, the
> ops pointer above the function can be patched atomically.
>
> With a dynamic trampoline we need a auipc+jalr pair at function entry to jump
> to the trampoline and then another auipc+jalr pair to jump from trampoline to
> ops->func. When the ops->func is modified, we would need to update the
> auipc+jalr at in the trampoline.
>
> So, I am not sure how to move forward here, CALL-OPS or Dynamic trampolines?
Yeah. Honestly, we need to figure out the patching story prior
choosing the path, so let's start there.
After reading Mark's reply, and discussing with OpenJDK folks (who does
the most crazy text patching on all platforms), having to patch multiple
instructions (where the address materialization is split over multiple
instructions) is a no-go. It's just a too big can of worms. So, if we
can only patch one insn, it's CALL_OPS.
A couple of options (in addition to Andy's), and all require a
per-function landing address ala CALL_OPS) tweaking what Mark is doing
on Arm (given the poor branch range).
..and maybe we'll get RISC-V rainbows/unicorns in the future getting
better reach (full 64b! ;-)).
A) Use auipc/jalr, only patch jalr to take us to a common
dispatcher/trampoline
| <func_trace_target_data_8B> # probably on a data cache-line != func .text to avoid ping-pong
| ...
| func:
| ...make sure ra isn't messed up...
| aupic
| nop <=> jalr # Text patch point -> common_dispatch
| ACTUAL_FUNC
|
| common_dispatch:
| load <func_trace_target_data_8B> based on ra
| jalr
| ...
The auipc is never touched, and will be overhead. Also, we need a mv to
store ra in a scratch register as well -- like Arm. We'll have two insn
per-caller overhead for a disabled caller.
B) Use jal, which can only take us +/-1M, and requires multiple
dispatchers (and tracking which one to use, and properly distribute
them. Ick.)
| <func_trace_target_data_8B> # probably on a data cache-line != func .text to avoid ping-pong
| ...
| func:
| ...make sure ra isn't messed up...
| nop <=> jal # Text patch point -> within_1M_to_func_dispatch
| ACTUAL_FUNC
|
| within_1M_to_func_dispatch:
| load <func_trace_target_data_8B> based on ra
| jalr
C) Use jal, which can only take us +/-1M, and use a per-function
trampoline requires multiple dispatchers (and tracking which one to
use). Blows up text size A LOT.
| <func_trace_target_data_8B> # somewhere, but probably on a different cacheline than the .text to avoid ping-ongs
| ...
| per_func_dispatch
| load <func_trace_target_data_8B> based on ra
| jalr
| func:
| ...make sure ra isn't messed up...
| nop <=> jal # Text patch point -> per_func_dispatch
| ACTUAL_FUNC
It's a bit sad that we'll always have to have a dispatcher/trampoline,
but it's still better than stop_machine(). (And we'll need a fencei IPI
as well, but only one. ;-))
Today, I'm leaning towards A (which is what Mark suggested, and also
Robbin).. Any other options?
[Now how do we implement OPTPROBES? I'm kidding. ;-)]
Björn
Björn Töpel <[email protected]> writes:
> Puranjay Mohan <[email protected]> writes:
>
>> Björn Töpel <[email protected]> writes:
>>
>>>
>>> Hmm, depending on RISC-V's CMODX path, the pro/cons CALL_OPS vs dynamic
>>> trampolines changes quite a bit.
>>>
>>> The more I look at the pains of patching two instruction ("split
>>> immediates"), the better "patch data" + one insn patching look.
>>
>> I was looking at how dynamic trampolines would be implemented for RISC-V.
>>
>> With CALL-OPS we need to patch the auipc+jalr at function entry only, the
>> ops pointer above the function can be patched atomically.
>>
>> With a dynamic trampoline we need a auipc+jalr pair at function entry to jump
>> to the trampoline and then another auipc+jalr pair to jump from trampoline to
>> ops->func. When the ops->func is modified, we would need to update the
>> auipc+jalr at in the trampoline.
>>
>> So, I am not sure how to move forward here, CALL-OPS or Dynamic trampolines?
>
> Yeah. Honestly, we need to figure out the patching story prior
> choosing the path, so let's start there.
>
> After reading Mark's reply, and discussing with OpenJDK folks (who does
> the most crazy text patching on all platforms), having to patch multiple
> instructions (where the address materialization is split over multiple
> instructions) is a no-go. It's just a too big can of worms. So, if we
> can only patch one insn, it's CALL_OPS.
>
> A couple of options (in addition to Andy's), and all require a
> per-function landing address ala CALL_OPS) tweaking what Mark is doing
> on Arm (given the poor branch range).
>
> ...and maybe we'll get RISC-V rainbows/unicorns in the future getting
> better reach (full 64b! ;-)).
>
> A) Use auipc/jalr, only patch jalr to take us to a common
> dispatcher/trampoline
>
> | <func_trace_target_data_8B> # probably on a data cache-line != func text to avoid ping-pong
> | ...
> | func:
> | ...make sure ra isn't messed up...
> | aupic
> | nop <=> jalr # Text patch point -> common_dispatch
> | ACTUAL_FUNC
> |
> | common_dispatch:
> | load <func_trace_target_data_8B> based on ra
> | jalr
> | ...
>
> The auipc is never touched, and will be overhead. Also, we need a mv to
> store ra in a scratch register as well -- like Arm. We'll have two insn
> per-caller overhead for a disabled caller.
>
> B) Use jal, which can only take us +/-1M, and requires multiple
> dispatchers (and tracking which one to use, and properly distribute
> them. Ick.)
>
> | <func_trace_target_data_8B> # probably on a data cache-line != func text to avoid ping-pong
> | ...
> | func:
> | ...make sure ra isn't messed up...
> | nop <=> jal # Text patch point -> within_1M_to_func_dispatch
> | ACTUAL_FUNC
> |
> | within_1M_to_func_dispatch:
> | load <func_trace_target_data_8B> based on ra
> | jalr
>
> C) Use jal, which can only take us +/-1M, and use a per-function
> trampoline requires multiple dispatchers (and tracking which one to
> use). Blows up text size A LOT.
>
> | <func_trace_target_data_8B> # somewhere, but probably on a different cacheline than the .text to avoid ping-ongs
> | ...
> | per_func_dispatch
> | load <func_trace_target_data_8B> based on ra
> | jalr
> | func:
> | ...make sure ra isn't messed up...
> | nop <=> jal # Text patch point -> per_func_dispatch
> | ACTUAL_FUNC
Brendan proposed yet another option, "in-function dispatch":
D)
| <func_trace_target_data_8B_per_function> # idk somewhere
| ...
| func:
| mv tmp1, ra
| aupic tmp2, <func_trace_target_data_8B_per_function:upper>
| mv tmp3, zero <=> ld tmp3, tmp2<func_trace_target_data_8B_per_function:lower>
| nop <=> jalr ra, tmp3
| ACTUAL_FUNC
There are 4 CMODX possiblities:
mv, nop: fully disabled, no problems
mv, jalr: We will jump to zero. We would need to have the inst
page/access fault handler take care of this case. Especially
if we align the instructions so that they can be patched
together, being interrupted in the middle and taking this
path will be rare.
ld, nop: no problems
ld, jalr: fully enabled, no problems
Patching is a 64b store/sd, and we only need a fence.i at the end, since
we can handle all 4 possibilities.
For the disabled case we'll have:
A) mv, aupic, nop
D) mv, aupic, mv, nop.
Puranjay, I've flipped. Let's go Mark's CALL_OPS together with a new
text patch mechanism w/o stop_machine().
Björn
On Thu, Mar 14, 2024 at 04:07:33PM +0100, Bj"orn T"opel wrote:
> After reading Mark's reply, and discussing with OpenJDK folks (who does
> the most crazy text patching on all platforms), having to patch multiple
> instructions (where the address materialization is split over multiple
> instructions) is a no-go. It's just a too big can of worms. So, if we
> can only patch one insn, it's CALL_OPS.
>
> A couple of options (in addition to Andy's), and all require a
> per-function landing address ala CALL_OPS) tweaking what Mark is doing
> on Arm (given the poor branch range).
>
> ..and maybe we'll get RISC-V rainbows/unicorns in the future getting
> better reach (full 64b! ;-)).
>
> A) Use auipc/jalr, only patch jalr to take us to a common
> dispatcher/trampoline
>
> | <func_trace_target_data_8B> # probably on a data cache-line != func .text to avoid ping-pong
> | ...
> | func:
> | ...make sure ra isn't messed up...
> | aupic
> | nop <=> jalr # Text patch point -> common_dispatch
> | ACTUAL_FUNC
> |
> | common_dispatch:
> | load <func_trace_target_data_8B> based on ra
> | jalr
> | ...
>
> The auipc is never touched, and will be overhead. Also, we need a mv to
> store ra in a scratch register as well -- like Arm. We'll have two insn
> per-caller overhead for a disabled caller.
Is the AUIPC a significant overhead? IIUC that's similar to Arm's ADRP, and I'd
have expected that to be pretty cheap.
IIUC your JALR can choose which destination register to store the return
address in, and if so, you could leave the original ra untouched (and recover
that in the common trampoline). Have I misunderstood that?
Maybe that doesn't play nicely with something else?
> B) Use jal, which can only take us +/-1M, and requires multiple
> dispatchers (and tracking which one to use, and properly distribute
> them. Ick.)
>
> | <func_trace_target_data_8B> # probably on a data cache-line != func .text to avoid ping-pong
> | ...
> | func:
> | ...make sure ra isn't messed up...
> | nop <=> jal # Text patch point -> within_1M_to_func_dispatch
> | ACTUAL_FUNC
> |
> | within_1M_to_func_dispatch:
> | load <func_trace_target_data_8B> based on ra
> | jalr
>
> C) Use jal, which can only take us +/-1M, and use a per-function
> trampoline requires multiple dispatchers (and tracking which one to
> use). Blows up text size A LOT.
>
> | <func_trace_target_data_8B> # somewhere, but probably on a different cacheline than the .text to avoid ping-ongs
> | ...
> | per_func_dispatch
> | load <func_trace_target_data_8B> based on ra
> | jalr
> | func:
> | ...make sure ra isn't messed up...
> | nop <=> jal # Text patch point -> per_func_dispatch
> | ACTUAL_FUNC
Beware that with option (C) you'll need to handle that in your unwinder for
RELIABLE_STACKTRACE. If you don't have a symbol for per_func_dispatch (or
func_trace_target_data_8B), PC values within per_func_dispatch would be
symbolized as the prior function/data.
> It's a bit sad that we'll always have to have a dispatcher/trampoline,
> but it's still better than stop_machine(). (And we'll need a fencei IPI
> as well, but only one. ;-))
>
> Today, I'm leaning towards A (which is what Mark suggested, and also
> Robbin).. Any other options?
Assuming my understanding of JALR above is correct, I reckon A is the nicest
option out of A/B/C.
Mark.
Andy,
Pulling out the A option:
>> > A) Use auipc/jalr, only patch jalr to take us to a common
>> > dispatcher/trampoline
>> >
>> > | <func_trace_target_data_8B> # probably on a data cache-line != func .text to avoid ping-pong
>> > | ...
>> > | func:
>> > | ...make sure ra isn't messed up...
>> > | aupic
>> > | nop <=> jalr # Text patch point -> common_dispatch
>> > | ACTUAL_FUNC
>> > |
>> > | common_dispatch:
>> > | load <func_trace_target_data_8B> based on ra
>> > | jalr
>> > | ...
>> >
>> > The auipc is never touched, and will be overhead. Also, we need a mv to
>> > store ra in a scratch register as well -- like Arm. We'll have two insn
>> > per-caller overhead for a disabled caller.
>
> My patch series takes a similar "in-function dispatch" approach. A
> difference is that the <func_trace_target_data_8B_per_function> is
> embedded within each function entry. I'd like to have it moved to a
> run-time allocated array to reduce total text size.
This is what arm64 has as well. It's a 8B + 1-2 dirt cheap movish like
instructions (save ra, prepare jump with auipc). I think that's a
reasonable overhead.
> Another difference is that my series changes the first instruction to
> "j ACTUAL_FUNC" for the "ftrace disable" case. As long as the
> architecture guarantees the atomicity of the first instruction, then
> we are safe. For example, we are safe if the first instruction could
> only be "mv tmp, ra" or "j ACTUAL_FUNC". And since the loaded address is
> always valid, we can fix "mv + jalr" down so we don't have to
> play with the exception handler trick. The guarantee from arch would
> require ziccif (in RVA22) though, but I think it is the same for us
> (unless with stop_machine). For ziccif, I would rather call that out
> during boot than blindly assume.
I'm maybe biased, but I'd prefer the A) over your version with the
unconditional jump. A) has the overhead of two, I'd say, free
instructions (again "Meten is Weten!" ;-)).
> However, one thing I am not very sure is: do we need a destination
> address in a "per-function" manner? It seems like most of the time the
> destination address can only be ftrace_call, or ftrace_regs_call. If
> the number of destination addresses is very few, then we could
> potentially reduce the size of
> <func_trace_target_data_8B_per_function>.
Yes, we do need a per-function manner. BPF, e.g., uses
dynamically/JIT:ed trampolines/targets.
Björn
Mark,
Mark Rutland <[email protected]> writes:
>> A) Use auipc/jalr, only patch jalr to take us to a common
>> dispatcher/trampoline
>>
>> | <func_trace_target_data_8B> # probably on a data cache-line != func .text to avoid ping-pong
>> | ...
>> | func:
>> | ...make sure ra isn't messed up...
>> | aupic
>> | nop <=> jalr # Text patch point -> common_dispatch
>> | ACTUAL_FUNC
>> |
>> | common_dispatch:
>> | load <func_trace_target_data_8B> based on ra
>> | jalr
>> | ...
>>
>> The auipc is never touched, and will be overhead. Also, we need a mv to
>> store ra in a scratch register as well -- like Arm. We'll have two insn
>> per-caller overhead for a disabled caller.
>
> Is the AUIPC a significant overhead? IIUC that's similar to Arm's ADRP, and I'd
> have expected that to be pretty cheap.
No, reg-to-reg moves are dirt cheap in my book.
> IIUC your JALR can choose which destination register to store the return
> address in, and if so, you could leave the original ra untouched (and recover
> that in the common trampoline). Have I misunderstood that?
>
> Maybe that doesn't play nicely with something else?
No, you're right, we can link to another register, and shave off an
instruction. I can imagine that some implementation prefer x1/x5 for
branch prediction reasons, but that's something that we can measure on.
So, 1-2 movs + nop are unconditionally executed on the disabled case.
(1-2 depending on the ra save/jalr reg strategy).
>> B) Use jal, which can only take us +/-1M, and requires multiple
>> dispatchers (and tracking which one to use, and properly distribute
>> them. Ick.)
>>
>> | <func_trace_target_data_8B> # probably on a data cache-line != func .text to avoid ping-pong
>> | ...
>> | func:
>> | ...make sure ra isn't messed up...
>> | nop <=> jal # Text patch point -> within_1M_to_func_dispatch
>> | ACTUAL_FUNC
>> |
>> | within_1M_to_func_dispatch:
>> | load <func_trace_target_data_8B> based on ra
>> | jalr
>>
>> C) Use jal, which can only take us +/-1M, and use a per-function
>> trampoline requires multiple dispatchers (and tracking which one to
>> use). Blows up text size A LOT.
>>
>> | <func_trace_target_data_8B> # somewhere, but probably on a different cacheline than the .text to avoid ping-ongs
>> | ...
>> | per_func_dispatch
>> | load <func_trace_target_data_8B> based on ra
>> | jalr
>> | func:
>> | ...make sure ra isn't messed up...
>> | nop <=> jal # Text patch point -> per_func_dispatch
>> | ACTUAL_FUNC
>
> Beware that with option (C) you'll need to handle that in your unwinder for
> RELIABLE_STACKTRACE. If you don't have a symbol for per_func_dispatch (or
> func_trace_target_data_8B), PC values within per_func_dispatch would be
> symbolized as the prior function/data.
Good point (but I don't like C much...)!
>> It's a bit sad that we'll always have to have a dispatcher/trampoline,
>> but it's still better than stop_machine(). (And we'll need a fencei IPI
>> as well, but only one. ;-))
>>
>> Today, I'm leaning towards A (which is what Mark suggested, and also
>> Robbin).. Any other options?
>
> Assuming my understanding of JALR above is correct, I reckon A is the nicest
> option out of A/B/C.
Yes! +1!
Björn
On Tue, 12 Mar 2024 13:42:28 +0000
Mark Rutland <[email protected]> wrote:
> > It would be interesting to see how the per-call performance would
> > improve on x86 with CALL_OPS! ;-)
>
> Heh. ;)
But this would require adding -fpatchable-function-entry on x86, which
would increase the size of text, which could possibly have a performance
regression when tracing is disabled.
I'd have no problem if someone were to implement it, but there's a strict
requirement that it does not slow down the system when tracing is disabled.
As tracing is a second class citizen compared to the rest of the kernel.
-- Steve
Andy Chiu <[email protected]> writes:
> On Thu, Mar 21, 2024 at 4:48 PM Björn Töpel <[email protected]> wrote:
>>
>> Andy,
>>
>> Pulling out the A option:
>>
>> >> > A) Use auipc/jalr, only patch jalr to take us to a common
>> >> > dispatcher/trampoline
>> >> >
>> >> > | <func_trace_target_data_8B> # probably on a data cache-line != func .text to avoid ping-pong
>> >> > | ...
>> >> > | func:
>> >> > | ...make sure ra isn't messed up...
>> >> > | aupic
>> >> > | nop <=> jalr # Text patch point -> common_dispatch
>> >> > | ACTUAL_FUNC
>> >> > |
>> >> > | common_dispatch:
>> >> > | load <func_trace_target_data_8B> based on ra
>> >> > | jalr
>> >> > | ...
>> >> >
>> >> > The auipc is never touched, and will be overhead. Also, we need a mv to
>> >> > store ra in a scratch register as well -- like Arm. We'll have two insn
>> >> > per-caller overhead for a disabled caller.
>> >
>> > My patch series takes a similar "in-function dispatch" approach. A
>> > difference is that the <func_trace_target_data_8B_per_function> is
>> > embedded within each function entry. I'd like to have it moved to a
>> > run-time allocated array to reduce total text size.
>>
>> This is what arm64 has as well. It's a 8B + 1-2 dirt cheap movish like
>> instructions (save ra, prepare jump with auipc). I think that's a
>> reasonable overhead.
>>
>> > Another difference is that my series changes the first instruction to
>> > "j ACTUAL_FUNC" for the "ftrace disable" case. As long as the
>> > architecture guarantees the atomicity of the first instruction, then
>> > we are safe. For example, we are safe if the first instruction could
>> > only be "mv tmp, ra" or "j ACTUAL_FUNC". And since the loaded address is
>> > always valid, we can fix "mv + jalr" down so we don't have to
>> > play with the exception handler trick. The guarantee from arch would
>> > require ziccif (in RVA22) though, but I think it is the same for us
>> > (unless with stop_machine). For ziccif, I would rather call that out
>> > during boot than blindly assume.
>>
>> I'm maybe biased, but I'd prefer the A) over your version with the
>> unconditional jump. A) has the overhead of two, I'd say, free
>> instructions (again "Meten is Weten!" ;-)).
>
> Yes, I'd also prefer A for less overall patch size. We can also
> optimize the overhead with a direct jump if that makes sense. Though,
> we need to sort out a way to map functions to corresponding
> trampolines. A direct way I could image is CALL_OPS'ish patching
> style, if the ftrace destination has to be patched in a per-function
> manner. For example:
>
> <index-in-dispatch-list>
> func_symbol:
> auipc t0, common_dispatch:high <=> j actual_func:
> jalr t0, common_dispatch:low(t0)
>
> common_dispatch:
> load t1, index + dispatch-list
> ld t1, 0(t1)
> jr t1
Yup, exactly like that (but I'd put the acutal target ptr in there,
instead of an additional indirection. Exactly what Mark does for arm64).
When we enter the common_dispatch, the ptr <index-in-dispatch-list>
would be -12(t0).
As for patching auipc or jalr, I guess we need to measure what's best.
My knee-jerk would be always auipc is better than jump -- but let's
measure. ;-)
Björn
On Tue, 12 Mar 2024 13:42:28 +0000
Mark Rutland <[email protected]> wrote:
> There are ways around that, but they're complicated and/or expensive, e.g.
>
> * Use a sequence of multiple patches, starting with replacing the JALR with an
> exception-generating instruction with a fixup handler, which is sort-of what
> x86 does with UD2. This may require multiple passes with
> synchronize_rcu_tasks() to make sure all threads have seen the latest
> instructions, and that cannot be done under stop_machine(), so if you need
> stop_machine() for CMODx reasons, you may need to use that several times with
> intervening calls to synchronize_rcu_tasks().
Just for clarification. x86 doesn't use UD2 for updating the call sites. It
uses the breakpoint (0xcc) operation. This is because x86 instructions are
not a fixed size and can cross cache boundaries, making updates to text
sections dangerous as another CPU may get half the old instruction and half
the new one!
Thus, when we have:
0f 1f 44 00 00 nop
and want to convert it to:
e8 e7 57 07 00 call ftrace_caller
We have to first add a breakpoint:
cc 17 44 00 00
Send an IPI to all CPUs so that they see the breakpoint (IPI is equivalent
to a memory barrier).
We have a ftrace breakpoint handler that will look at the function that the
breakpoint happened on. If it was a nop, it will just skip over the rest of
the instruction, and return from the break point handler just after the
"cc 17 44 00 00". If it's supposed to be a function, it will push the
return to after the instruction onto the stack, and return from the break
point handler to ftrace_caller. (Although things changed a little since
this update is now handled by text_poke_bp_batch()).
Then it changes the rest of the instruction:
cc e7 57 07 00
Sends out another IPI to all CPUs and removes the break point with the new
instruction op.
e8 e7 57 07 00
and now all the callers of this function will call the ftrace_caller.
-- Steve
Hey,
> <index-in-dispatch-list>
> func_symbol:
> auipc t0, common_dispatch:high <=> j actual_func:
> jalr t0, common_dispatch:low(t0)
>
If you are patching in a jump, I don't see why you wouldn't jump over
ld+jalr? (no need for common dispatch)
Patching jalr with nop, and keeping auipc, addresses the issue with
having to jump in the disabled case.
But needs either common dispatch or per func dispatch.
Thanks, Robbin
> common_dispatch:
> load t1, index + dispatch-list
> ld t1, 0(t1)
> jr t1
>
>
> >
> > > However, one thing I am not very sure is: do we need a destination
> > > address in a "per-function" manner? It seems like most of the time the
> > > destination address can only be ftrace_call, or ftrace_regs_call. If
> > > the number of destination addresses is very few, then we could
> > > potentially reduce the size of
> > > <func_trace_target_data_8B_per_function>.
> >
> > Yes, we do need a per-function manner. BPF, e.g., uses
> > dynamically/JIT:ed trampolines/targets.
> >
> >
> >
> > Björn
>
> Cheers,
> Andy