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> <87a61z2n55.fsf@all.your.base.are.belong.to.us>
In-Reply-To: <87a61z2n55.fsf@all.your.base.are.belong.to.us>
From:   Guo Ren <guoren@kernel.org>
Date:   Tue, 31 Jan 2023 16:30:56 +0800
Message-ID: <CAJF2gTQ7ZfF7JdpxP8O6RZSvPrUqk79geXuyQLweu6U15XjAzA@mail.gmail.com>
Subject: Re: [PATCH] riscv: kprobe: Optimize kprobe with accurate atomicity
To:     =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn@kernel.org>
Cc:     Mark Rutland <mark.rutland@arm.com>,
        "liaochang (A)" <liaochang1@huawei.com>, palmer@dabbelt.com,
        paul.walmsley@sifive.com, mhiramat@kernel.org,
        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 Tue, Jan 31, 2023 at 3:12 PM Bj=C3=B6rn T=C3=B6pel <bjorn@kernel.org> wr=
ote:
>
> Guo Ren <guoren@kernel.org> writes:
>
> > On Mon, Jan 30, 2023 at 11:49 PM Mark Rutland <mark.rutland@arm.com> wr=
ote:
> >>
> >> Hi Bjorn,
> >>
> >> On Mon, Jan 30, 2023 at 04:28:15PM +0100, Bj=C3=B6rn T=C3=B6pel wrote:
> >> > Guo Ren <guoren@kernel.org> writes:
> >> >
> >> > >> In the serie of RISCV OPTPROBES [1], it patches a long-jump instr=
uctions 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 st=
op other CPUs
> >> > >> via patch_text API, or you have any better solution to achieve th=
e purpose?
> >> > >  - The stop_machine is an expensive way all architectures should
> >> > > avoid, and you could keep that in your OPTPROBES implementation fi=
les
> >> > > 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'r=
e
> >> > replacing multiple instructions (see Mark's post at [1]). The
> >> > stop_machine() dance might work when you're replacing *one* instruct=
ion,
> >> > 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 rea=
dy
> >> 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.or=
g/
Thx for sharing :)

--=20
Best Regards
 Guo Ren