| Message ID | 20220927022435.129965-4-liaochang1@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kprobe: Optimize the performance of patching ss | expand |
On Tue, Sep 27, 2022 at 10:24:35AM +0800, Liao Chang wrote: > Single-step slot would not be used until kprobe is enabled, that means > no race condition occurs on it under SMP, hence it is safe to pacth ss > slot without stopping machine. > > Since I and D caches are coherent within single-step slot from > aarch64_insn_patch_text_nosync(), hence no need to do it again via > flush_icache_range(). > > Acked-by: Will Deacon <will@kernel.org> > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > Signed-off-by: Liao Chang <liaochang1@huawei.com> > --- > arch/arm64/kernel/probes/kprobes.c | 27 +++++++++++++++++++++------ > 1 file changed, 21 insertions(+), 6 deletions(-) What's your expectation with this series, should the arch maintainers just pick the individual patches?
在 2022/9/30 0:50, Catalin Marinas 写道: > On Tue, Sep 27, 2022 at 10:24:35AM +0800, Liao Chang wrote: >> Single-step slot would not be used until kprobe is enabled, that means >> no race condition occurs on it under SMP, hence it is safe to pacth ss >> slot without stopping machine. >> >> Since I and D caches are coherent within single-step slot from >> aarch64_insn_patch_text_nosync(), hence no need to do it again via >> flush_icache_range(). >> >> Acked-by: Will Deacon <will@kernel.org> >> Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> >> Signed-off-by: Liao Chang <liaochang1@huawei.com> >> --- >> arch/arm64/kernel/probes/kprobes.c | 27 +++++++++++++++++++++------ >> 1 file changed, 21 insertions(+), 6 deletions(-) > > What's your expectation with this series, should the arch maintainers > just pick the individual patches? Yes, or should i split this series into individual patch? Thanks. >
On Fri, Sep 30, 2022 at 09:02:20AM +0800, liaochang (A) wrote: > > > 在 2022/9/30 0:50, Catalin Marinas 写道: > > On Tue, Sep 27, 2022 at 10:24:35AM +0800, Liao Chang wrote: > >> Single-step slot would not be used until kprobe is enabled, that means > >> no race condition occurs on it under SMP, hence it is safe to pacth ss > >> slot without stopping machine. > >> > >> Since I and D caches are coherent within single-step slot from > >> aarch64_insn_patch_text_nosync(), hence no need to do it again via > >> flush_icache_range(). > >> > >> Acked-by: Will Deacon <will@kernel.org> > >> Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > >> Signed-off-by: Liao Chang <liaochang1@huawei.com> > >> --- > >> arch/arm64/kernel/probes/kprobes.c | 27 +++++++++++++++++++++------ > >> 1 file changed, 21 insertions(+), 6 deletions(-) > > > > What's your expectation with this series, should the arch maintainers > > just pick the individual patches? > > Yes, or should i split this series into individual patch? No need to, I can pick the arm64 patch. If the other maintainers don't merge the patches, you might want to post them again individually as to avoid confusion.
diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c index d1d182320245..c9e4d0720285 100644 --- a/arch/arm64/kernel/probes/kprobes.c +++ b/arch/arm64/kernel/probes/kprobes.c @@ -44,13 +44,28 @@ post_kprobe_handler(struct kprobe *, struct kprobe_ctlblk *, struct pt_regs *); static void __kprobes arch_prepare_ss_slot(struct kprobe *p) { kprobe_opcode_t *addr = p->ainsn.api.insn; - void *addrs[] = {addr, addr + 1}; - u32 insns[] = {p->opcode, BRK64_OPCODE_KPROBES_SS}; - /* prepare insn slot */ - aarch64_insn_patch_text(addrs, insns, 2); - - flush_icache_range((uintptr_t)addr, (uintptr_t)(addr + MAX_INSN_SIZE)); + /* + * Prepare insn slot, Mark Rutland points out it depends on a coupe of + * subtleties: + * + * - That the I-cache maintenance for these instructions is complete + * *before* the kprobe BRK is written (and aarch64_insn_patch_text_nosync() + * ensures this, but just omits causing a Context-Synchronization-Event + * on all CPUS). + * + * - That the kprobe BRK results in an exception (and consequently a + * Context-Synchronoization-Event), which ensures that the CPU will + * fetch thesingle-step slot instructions *after* this, ensuring that + * the new instructions are used + * + * It supposes to place ISB after patching to guarantee I-cache maintenance + * is observed on all CPUS, however, single-step slot is installed in + * the BRK exception handler, so it is unnecessary to generate + * Contex-Synchronization-Event via ISB again. + */ + aarch64_insn_patch_text_nosync(addr, p->opcode); + aarch64_insn_patch_text_nosync(addr + 1, BRK64_OPCODE_KPROBES_SS); /* * Needs restoring of return address after stepping xol.