Re: [PATCH RFC v2 riscv/for-next 0/5] Enable ftrace with kernel preemption for RISC-V

[Evgenii Shatokhin <e.shatokhin@yadro.com>]

Hi,

On 13.09.2022 12:42, Andy Chiu wrote:
> This patch removes dependency of dynamic ftrace from calling
> stop_machine(), and makes it compatiable with kernel preemption.
> Originally, we ran into stack corruptions, or execution of partially
> updated instructions when starting or stopping ftrace on a fully
> preemptible kernel configuration. The reason is that kernel periodically
> calls rcu_momentary_dyntick_idle() on cores waiting for the code-patching
> core running in ftrace. Though rcu_momentary_dyntick_idle() itself is
> marked as notrace, it would call a bunch of tracable functions if we
> configured the kernel as preemptible. For example, these are some functions
> that happened to have a symbol and have not been marked as notrace on a
> RISC-V preemptible kernel compiled with GCC-11:
>   - __rcu_report_exp_rnp()
>   - rcu_report_exp_cpu_mult()
>   - rcu_preempt_deferred_qs()
>   - rcu_preempt_need_deferred_qs()
>   - rcu_preempt_deferred_qs_irqrestore()
> 
> Thus, this make it not ideal for us to rely on stop_machine() and
> handly marked "notrace"s to perform runtime code patching. To remove
> such dependency, we must make updates of code seemed atomic on running
> cores. This might not be obvious for RISC-V since it usaually uses a pair
> of AUIPC + JALR to perform a long jump, which cannot be modified and
> executed concurrently if we consider preemptions. As such, this patch
> proposed a way to make it possible. It embeds a 32-bit rel-address data
> into instructions of each ftrace prologue and jumps indirectly. In this
> way, we could store and load the address atomically so that the code
> patching core could run simutaneously with the rest of running cores.
> 
> After applying the patchset, we compiled a preemptible kernel with all
> tracers and ftrace-selftest enabled, and booted it on a 2-core QEMU virt
> machine. The kernel could boot up successfully, passing all ftrace
> testsuits. Besides, we ran a script that randomly pick a tracer on every
> 0~5 seconds. The kernel has sustained over 20K rounds of the test. In
> contrast, a preemptible kernel without our patch would panic in few
> rounds on the same machine.
> 
> Though we ran into errors when using hwlat or irqsoff tracers together
> with cpu-online stressor from stress-ng on a preemptible kernel. We
> believe the reason may be that  percpu workers of the tracers are being
> queued into unbounded workqueue when cpu get offlined and patches will go
> through tracing tree.
> 
> Additionally, we found patching of tracepoints unsafe since the
> instructions being patched are not naturally aligned. This may result in
> 2 half-word stores, which breaks atomicity, during the code patching.
> 
> changes in patch v2:
>   - Enforce alignments on all functions with a compiler workaround.
>   - Support 64bit addressing for ftrace targets if xlen == 64
>   - Initialize ftrace target addresses to avoid calling bad address in a
>     hypothesized case.
>   - Use LGPTR instead of SZPTR since .align is log-scaled for
>     mcount-dyn.S
>   - Require the nop instruction of all jump_labels aligns naturally on
>     4B.
> 
> Andy Chiu (5):
>    riscv: align ftrace to 4 Byte boundary and increase ftrace prologue
>      size
>    riscv: export patch_insn_write
>    riscv: ftrace: use indirect jump to work with kernel preemption
>    riscv: ftrace: do not use stop_machine to update code
>    riscv: align arch_static_branch function
> 
>   arch/riscv/Makefile                 |   2 +-
>   arch/riscv/include/asm/ftrace.h     |  24 ----
>   arch/riscv/include/asm/jump_label.h |   2 +
>   arch/riscv/include/asm/patch.h      |   1 +
>   arch/riscv/kernel/ftrace.c          | 179 ++++++++++++++++++++--------
>   arch/riscv/kernel/mcount-dyn.S      |  69 ++++++++---
>   arch/riscv/kernel/patch.c           |   4 +-
>   7 files changed, 188 insertions(+), 93 deletions(-)
> 

First of all, thank you for working on making dynamic Ftrace robust in 
preemptible kernels on RISC-V.
It is an important use case but, for now, dynamic Ftrace and related 
tracers cannot be safely used with such kernels.

Are there any updates on this series?
It needs a rebase, of course, but it looks doable.

If I understand the discussion correctly, the only blocker was that 
using "-falign-functions" was not enough to properly align cold 
functions and "-fno-guess-branch-probability" would likely have a 
performance cost.

It seems, GCC developers have recently provided a workaround for that 
(https://gcc.gnu.org/git/gitweb.cgi?p=gcc.git;h=0f5a9a00e3ab1fe96142f304cfbcf3f63b15f326, 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88345#c24).

"-fmin-function-alignment" should help but, I do not know, which GCC 
versions have got that patch already. In the meantime, one could 
probably check if "-fmin-function-alignment" is supported by the 
compiler and use it, if it is.

Thoughts?

Regards,
Evgenii

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv