Re: [PATCH RFC v2 riscv/for-next 0/5] Enable ftrace with kernel preemption for RISC-V

From: Andy Chiu <andy.chiu@sifive.com>
To: Alexandre Ghiti <alex@ghiti.fr>
Cc: "Björn Töpel" <bjorn@kernel.org>,
	"Evgenii Shatokhin" <e.shatokhin@yadro.com>,
	palmer@dabbelt.com, paul.walmsley@sifive.com,
	aou@eecs.berkeley.edu, rostedt@goodmis.org, mingo@redhat.com,
	peterz@infradead.org, jpoimboe@kernel.org, jbaron@akamai.com,
	ardb@kernel.org, greentime.hu@sifive.com, zong.li@sifive.com,
	guoren@kernel.org, "Jessica Clarke" <jrtc27@jrtc27.com>,
	kernel@esmil.dk, linux-riscv@lists.infradead.org,
	linux@yadro.com, "Samuel Holland" <samuel.holland@sifive.com>
Subject: Re: [PATCH RFC v2 riscv/for-next 0/5] Enable ftrace with kernel preemption for RISC-V
Date: Thu, 21 Mar 2024 00:37:00 +0800	[thread overview]
Message-ID: <CABgGipVssDpW26b8_JdrgP+BFuaAG_A4xi7+o6NU9fmZA8D2YQ@mail.gmail.com> (raw)
In-Reply-To: <79b47f1d-8332-476c-b827-28043d7b3b76@ghiti.fr>

On Tue, Mar 19, 2024 at 10:50 PM Alexandre Ghiti <alex@ghiti.fr> wrote:
>
> On 11/03/2024 15:24, Andy Chiu wrote:
> > On Thu, Mar 7, 2024 at 11:57 PM Samuel Holland
> > <samuel.holland@sifive.com> wrote:
> >> Hi Alex,
> >>
> >> On 2024-03-07 7:21 AM, Alexandre Ghiti wrote:
> >>> But TBH, I have started thinking about the issue your patch is trying to deal
> >>> with. IIUC you're trying to avoid traps (or silent errors) that could happen
> >>> because of concurrent accesses when patching is happening on a pair auipc/jarl.
> >>>
> >>> I'm wondering if instead, we could not actually handle the potential traps:
> >>> before storing the auipc + jalr pair, we could use a well-identified trapping
> >>> instruction that could be recognized in the trap handler as a legitimate trap.
> >>> For example:
> >>>
> >>>
> >>> auipc  -->  auipc  -->  XXXX  -->  XXXX  -->  auipc
> >>> jalr        XXXX        XXXX       jalr       jalr
> >>>
> >>>
> >>> If a core traps on a XXXX instruction, we know this address is being patched, so
> >>> we can return and probably the patching will be over. We could also identify
> >>> half patched word instruction (I mean with only XX).
> >> Unfortunately this does not work without some fence.i in the middle. The
> >> processor is free to fetch any instruction that has been written to a location
> >> since the last fence.i instruction. So it would be perfectly valid to fetch the
> >> old aiupc and new jalr or vice versa and not trap. This would happen if, for
> >> example, the two instructions were in different cache lines, and only one of the
> >> cache lines got evicted and refilled.
> >>
> >> But sending an IPI to run the fence.i probably negates the performance benefit.
> > Maybe something like x86, we can hook ftrace_replace_code() out and
> > batch send IPIs to prevent storms of remote fences. The solution Alex
> > proposed can save the code size for function entries. But we have to
> > send out remote fences at each "-->" transition, which is 4 sets of
> > remote IPIs. On the other hand, this series increases the per-function
> > patch size to 24 bytes. However, it decreases the number of remote
> > fences to 1 set.
> >
> > The performance hit could be observable for the auipc + jalr case,
> > because all remote cores will be executing on XXXX instructions and
> > take a trap at each function entry during code patching.
> >
> > Besides, this series would give us a chance not to send any remote
> > fences if we were to change only the destination of ftrace (e.g. to a
> > custom ftrace trampoline). As it would be a regular store for the
> > writer and regular load for readers, only fence w,w is needed.
> > However, I am not very certain on how often would be for this
> > particular use case. I'd need some time to investigate it.
> >
> >> Maybe there is some creative way to overcome this.
> >>
> >>> But please let me know if that's completely stupid and I did not understand the
> >>> problem, since my patchset to support svvptc, I am wondering if it is not more
> >>> performant to actually take very unlikely traps instead of trying to avoid them.
> >> I agree in general it is a good idea to optimize the hot path like this.
> >>
> >> Regards,
> >> Samuel
> >>
> > Regards,
> > Andy
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>
>
> So indeed my solution was way too naive and we've been discussing that
> with Björn lately. He worked a lot on that and came up with the solution
> he proposed here
> https://lore.kernel.org/linux-riscv/87zfv0onre.fsf@all.your.base.are.belong.to.us/
>
> The thing is ftrace seems to be quite broken as the ftrace kselftests
> raise a lot of issues which I have started to debug but are not that
> easy, so we are wondering if *someone* should not work on Bjorn's
> solution (or another, open to discussions) for 6.10. @Andy WDYT? Do you
> have free cycles? Björn could work on that too (and I'll help if needed).

Do you mean the FTRACE_STARTUP_TEST, or something else? I am also
happy to help on text patching issues. It would be great if we could
define the remaining works and share them. Currently I am focusing on
having dynamic ftrace with preemption and getting rid of
stop_machine() while patching code. I am going to spin a revision of
this patch series in a few days if possible. There are quite some
things needed to be discussed and I'd like to join any conversation!

>
> Let me know what you think!
>
> Alex
>
>

Cheers,
Andy

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv