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

From: Andy Chiu <andy.chiu@sifive.com>
To: Evgenii Shatokhin <e.shatokhin@yadro.com>
Cc: palmer@dabbelt.com, paul.walmsley@sifive.com,
	aou@eecs.berkeley.edu,  rostedt@goodmis.org, mingo@redhat.com,
	peterz@infradead.org,  jpoimboe@kernel.org, jbaron@akamai.com,
	ardb@kernel.org,  greentime.hu@sifive.com, zong.li@sifive.com,
	guoren@kernel.org,  Jessica Clarke <jrtc27@jrtc27.com>,
	kernel@esmil.dk, linux-riscv@lists.infradead.org,
	 linux@yadro.com
Subject: Re: [PATCH RFC v2 riscv/for-next 0/5] Enable ftrace with kernel preemption for RISC-V
Date: Wed, 21 Feb 2024 13:27:39 +0800	[thread overview]
Message-ID: <CABgGipUGCVjy5wG1TMDpgf1Zn7Jcx5Lg1CUzQ0YLr5nOPf-TUg@mail.gmail.com> (raw)
In-Reply-To: <4fe4567b-96be-4102-952b-7d39109b2186@yadro.com>

On Wed, Feb 14, 2024 at 3:42 AM Evgenii Shatokhin <e.shatokhin@yadro.com> wrote:
>
> 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?

Hi Evgenii,

Thanks for the update. Indeed, it is essential to this patch for
toolchain to provide forced alignment. We can test this flag in the
Makefile to sort out if toolchain supports it or not. Meanwhile, I had
figured out a way for this to work on any 2-B align addresses but
hadn't implemented it out yet. Basically it would require more
patching space for us to do software alignment. I would opt for a
special toolchain flag if the toolchain just supports it.

Let me take some time to look and get back to you soon.

>
> Regards,
> Evgenii

Regards,
Andy

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