Messages in this thread | | | From | Björn Töpel <> | Subject | Re: [PATCH] riscv: kprobe: Optimize kprobe with accurate atomicity | Date | Tue, 31 Jan 2023 08:12:22 +0100 |
| |
Guo Ren <guoren@kernel.org> writes:
> On Mon, Jan 30, 2023 at 11:49 PM Mark Rutland <mark.rutland@arm.com> wrote: >> >> Hi Bjorn, >> >> On Mon, Jan 30, 2023 at 04:28:15PM +0100, Björn Töpel wrote: >> > Guo Ren <guoren@kernel.org> writes: >> > >> > >> In the serie of RISCV OPTPROBES [1], it patches a long-jump instructions pair >> > >> AUIPC/JALR in kernel text, so in order to ensure other CPUs does not execute >> > >> in the instructions that will be modified, it is still need to stop other CPUs >> > >> via patch_text API, or you have any better solution to achieve the purpose? >> > > - The stop_machine is an expensive way all architectures should >> > > avoid, and you could keep that in your OPTPROBES implementation files >> > > with static functions. >> > > - The stop_machine couldn't work with PREEMPTION, so your >> > > implementation needs to work with !PREEMPTION. >> > >> > ...and stop_machine() with !PREEMPTION is broken as well, when you're >> > replacing multiple instructions (see Mark's post at [1]). The >> > stop_machine() dance might work when you're replacing *one* instruction, >> > not multiple as in the RISC-V case. I'll expand on this in a comment in >> > the OPTPROBES v6 series. >> >> Just to clarify, my comments in [1] were assuming that stop_machine() was not >> used, in which case there is a problem with or without PREEMPTION. >> >> I believe that when using stop_machine(), the !PREEMPTION case is fine, since >> stop_machine() schedules work rather than running work in IRQ context on the >> back of an IPI, so no CPUs should be mid-sequnce during the patching, and it's >> not possible for there to be threads which are preempted mid-sequence. >> >> That all said, IIUC optprobes is going to disappear once fprobe is ready >> everywhere, so that might be moot. > The optprobes could be in the middle of a function, but fprobe must be > the entry of a function, right? > > Does your fprobe here mean: ? > > The Linux kernel configuration item CONFIG_FPROBE: > > prompt: Kernel Function Probe (fprobe) > type: bool > depends on: ( CONFIG_FUNCTION_TRACER ) && ( > CONFIG_DYNAMIC_FTRACE_WITH_REGS ) && ( CONFIG_HAVE_RETHOOK ) > defined in kernel/trace/Kconfig
See the cover of [1]. It's about direct calls for BPF tracing (and more) on Arm, and you're completly right, that it's *not* related to optprobes at all.
[1] https://lore.kernel.org/all/20221108220651.24492-1-revest@chromium.org/
| |