MIME-Version: 1.0
References: <20230126161559.1467374-1-guoren@kernel.org> <0abbbdd4-6b85-9659-03ee-97c56a5b77c1@huawei.com>
 <CAJF2gTS0s4X_uwLaEeSqKAyRmxCR2vxRuHhz7-SP2w4bBqzr+Q@mail.gmail.com>
 <87r0vc9h4g.fsf@all.your.base.are.belong.to.us> <Y9fm+6LPXgtDSma/@FVFF77S0Q05N>
 <CAJF2gTRgze_owuWvJjnrPpBNs8+GY-km7wvHU4EuJzarQc+BPQ@mail.gmail.com>
 <Y9juYX8Bt1Z55lv0@FVFF77S0Q05N> <20230217002351.112635f4fb35f84002666d29@kernel.org>
In-Reply-To: <20230217002351.112635f4fb35f84002666d29@kernel.org>
From:   Guo Ren <guoren@kernel.org>
Date:   Tue, 21 Feb 2023 09:30:44 +0800
Message-ID: <CAJF2gTTt5n8Ur4sRSDWppE2JtYtuEF2jEDC_mCBOVkripgkj6A@mail.gmail.com>
Subject: Re: [PATCH] riscv: kprobe: Optimize kprobe with accurate atomicity
To:     Masami Hiramatsu <mhiramat@kernel.org>
Cc:     Mark Rutland <mark.rutland@arm.com>,
        =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn@kernel.org>,
        "liaochang (A)" <liaochang1@huawei.com>, palmer@dabbelt.com,
        paul.walmsley@sifive.com, conor.dooley@microchip.com,
        penberg@kernel.org, linux-riscv@lists.infradead.org,
        linux-kernel@vger.kernel.org, Guo Ren <guoren@linux.alibaba.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Feb 16, 2023 at 11:23 PM Masami Hiramatsu <mhiramat@kernel.org> wro=
te:
>
> Hi,
>
> Sorry I missed this thread.
>
> On Tue, 31 Jan 2023 10:33:05 +0000
> Mark Rutland <mark.rutland@arm.com> wrote:
>
> > On Tue, Jan 31, 2023 at 09:48:29AM +0800, Guo Ren wrote:
> > > 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=C3=B6rn T=C3=B6pel wro=
te:
> > > > > Guo Ren <guoren@kernel.org> writes:
> > > > >
> > > > > >> In the serie of RISCV OPTPROBES [1], it patches a long-jump in=
structions pair
> > > > > >> AUIPC/JALR in kernel text, so in order to ensure other CPUs do=
es 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 shoul=
d
> > > > > > 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 yo=
u're
> > > > > replacing multiple instructions (see Mark's post at [1]). The
> > > > > stop_machine() dance might work when you're replacing *one* instr=
uction,
> > > > > not multiple as in the RISC-V case. I'll expand on this in a comm=
ent 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 f=
ine, since
> > > > stop_machine() schedules work rather than running work in IRQ conte=
xt on the
> > > > back of an IPI, so no CPUs should be mid-sequnce during the patchin=
g, and it's
> > > > not possible for there to be threads which are preempted mid-sequen=
ce.
> > > >
> > > > 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 b=
e
> > > 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
> >
> > Yes.
> >
> > Masami, Steve, and I had a chat at the tracing summit late last year (w=
hich
> > unfortunately, was not recorded), and what we'd like to do is get each
> > architecture to have FPROBE (and FTRACE_WITH_ARGS), at which point OPTP=
ROBE
> > and KRETPROBE become redundant and could be removed.
>
> No, the fprobe will replace the KRETPROBE but not OPTPROBE. The OPTPROBE
> is completely different one. Fprobe is used only for function entry, but
> optprobe is applied to the function body.
>
> >
> > i.e. we'd keep KPROBES as a "you can trace any instruction" feature, bu=
t in the
> > few cases where OPTPROBES can make things fater by using FTRACE, you sh=
ould
> > just use that directly via FPROBE.
>
> I think what you are saying is KPROBE_ON_FTRACE, and that will be replace=
d by
> FPROBES.
I'm sorry, I'm not sure how FPROBES could replace KPROBE_ON_FTRACE? Do
you have some discussion on it?

>
> Thank you,
>
> >
> > Thanks,
> > Mark.
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>


--=20
Best Regards
 Guo Ren