Date:   Sat, 10 Sep 2022 11:24:35 +0900
From:   Masami Hiramatsu (Google) <mhiramat@...nel.org>
To:     "liaochang (A)" <liaochang1@...wei.com>
Cc:     Jisheng Zhang <jszhang@...nel.org>, <paul.walmsley@...ive.com>,
        <palmer@...belt.com>, <aou@...s.berkeley.edu>,
        <rostedt@...dmis.org>, <linux-riscv@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] riscv/kprobe: Optimize the performance of patching
 instruction slot

On Fri, 9 Sep 2022 09:55:08 +0800
"liaochang (A)" <liaochang1@...wei.com> wrote:
> 
> 
> 在 2022/9/8 20:49, Masami Hiramatsu (Google) 写道:
> > On Thu, 8 Sep 2022 09:43:45 +0800
> > "liaochang (A)" <liaochang1@...wei.com> wrote:
> > 
> >> Thanks for comment.
> >>
> >> 在 2022/9/8 1:21, Jisheng Zhang 写道:
> >>> On Wed, Sep 07, 2022 at 10:33:27AM +0800, Liao Chang wrote:
> >>>> Since no race condition occurs on each instruction slot, hence it is
> >>>> safe to patch instruction slot without stopping machine.
> >>>
> >>> hmm, IMHO there's race when arming kprobe under SMP, so stopping
> >>> machine is necessary here. Maybe I misundertand something.
> >>>
> >>
> >> It is indeed necessary to stop machine when arm kprobe under SMP,
> >> but i don't think it need to stop machine when prepare instruction slot,
> >> two reasons:
> >>
> >> 1. Instruction slot is dynamically allocated data.
> >> 2. Kernel would not execute instruction slot until original instruction
> >>    is replaced by breakpoint.
> > 
> > Ah, this is for ss (single step out of line) slot. So until
> > kprobe is enabled, this should not be used from other cores.
> > OK, then it should be safe.
> 
> Exactly, Masami, and i find out this optimization could be applied to some other
> architectures, such as arm64 and csky, do you think it is good time to do them all.

Yes, we should reduce the stop_machine() usage. Thanks for pointing it!

> 
> Thanks.
> 
> > 
> > 
> >>>>
> >>>> Signed-off-by: Liao Chang <liaochang1@...wei.com>
> >>>> ---
> >>>>  arch/riscv/kernel/probes/kprobes.c | 8 +++++---
> >>>>  1 file changed, 5 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
> >>>> index e6e950b7cf32..eff7d7fab535 100644
> >>>> --- a/arch/riscv/kernel/probes/kprobes.c
> >>>> +++ b/arch/riscv/kernel/probes/kprobes.c
> >>>> @@ -24,12 +24,14 @@ post_kprobe_handler(struct kprobe *, struct kprobe_ctlblk *, struct pt_regs *);
> >>>>  static void __kprobes arch_prepare_ss_slot(struct kprobe *p)
> >>>>  {
> >>>>  	unsigned long offset = GET_INSN_LENGTH(p->opcode);
> >>>> +	const kprobe_opcode_t brk_insn = __BUG_INSN_32;
> >>>> +	kprobe_opcode_t slot[MAX_INSN_SIZE];
> >>>>  
> >>>>  	p->ainsn.api.restore = (unsigned long)p->addr + offset;
> >>>>  
> >>>> -	patch_text(p->ainsn.api.insn, p->opcode);
> >>>> -	patch_text((void *)((unsigned long)(p->ainsn.api.insn) + offset),
> >>>> -		   __BUG_INSN_32);
> >>>> +	memcpy(slot, &p->opcode, offset);
> >>>> +	memcpy((void *)((unsigned long)slot + offset), &brk_insn, 4);
> >>>> +	patch_text_nosync(p->ainsn.api.insn, slot, offset + 4);
> > 
> > BTW, didn't you have a macro for the size of __BUG_INSN_32?
> > 
> > Thank you,
> 
> I think you are saying GET_INSN_LENGTH, i will use it to caculate
> the size of __BUG_INSN_32 in v2, instead of magic number '4'.


Yeah, that's better.

Thank you!

> 
> Thanks.
> 
> > 
> > 
> >>>>  }
> >>>>  
> >>>>  static void __kprobes arch_prepare_simulate(struct kprobe *p)
> >>>> -- 
> >>>> 2.17.1
> >>>>
> >>>>
> >>>> _______________________________________________
> >>>> linux-riscv mailing list
> >>>> linux-riscv@...ts.infradead.org
> >>>> http://lists.infradead.org/mailman/listinfo/linux-riscv
> >>> .
> >>
> >> -- 
> >> BR,
> >> Liao, Chang
> > 
> > 
> 
> -- 
> BR,
> Liao, Chang


-- 
Masami Hiramatsu (Google) <mhiramat@...nel.org>

Powered by blists - more mailing lists