Message-ID: <aJMsWB2Sxb7-66zs@J2N7QTR9R3>
Date: Wed, 6 Aug 2025 11:20:08 +0100
From: Mark Rutland <mark.rutland@....com>
To: Jiri Olsa <olsajiri@...il.com>
Cc: Steven Rostedt <rostedt@...nel.org>, Florent Revest <revest@...gle.com>,
	bpf@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-trace-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	Alexei Starovoitov <ast@...nel.org>,
	Daniel Borkmann <daniel@...earbox.net>,
	Andrii Nakryiko <andrii@...nel.org>,
	Menglong Dong <menglong8.dong@...il.com>,
	Naveen N Rao <naveen@...nel.org>,
	Michael Ellerman <mpe@...erman.id.au>,
	Björn Töpel <bjorn@...osinc.com>,
	Andy Chiu <andybnac@...il.com>,
	Alexandre Ghiti <alexghiti@...osinc.com>,
	Palmer Dabbelt <palmer@...belt.com>
Subject: Re: [RFC 00/10] ftrace,bpf: Use single direct ops for bpf trampolines

On Sat, Aug 02, 2025 at 11:26:46PM +0200, Jiri Olsa wrote:
> On Fri, Aug 01, 2025 at 10:49:56AM +0100, Mark Rutland wrote:
> > On Wed, Jul 30, 2025 at 01:19:51PM +0200, Jiri Olsa wrote:
> > > On Tue, Jul 29, 2025 at 06:57:40PM +0100, Mark Rutland wrote:
> > > > 
> > > > On Tue, Jul 29, 2025 at 12:28:03PM +0200, Jiri Olsa wrote:
> > > > > hi,
> > > > > while poking the multi-tracing interface I ended up with just one
> > > > > ftrace_ops object to attach all trampolines.
> > > > > 
> > > > > This change allows to use less direct API calls during the attachment
> > > > > changes in the future code, so in effect speeding up the attachment.
> > > > 
> > > > How important is that, and what sort of speedup does this result in? I
> > > > ask due to potential performance hits noted below, and I'm lacking
> > > > context as to why we want to do this in the first place -- what is this
> > > > intended to enable/improve?
> > > 
> > > so it's all work on PoC stage, the idea is to be able to attach many
> > > (like 20,30,40k) functions to their trampolines quickly, which at the
> > > moment is slow because all the involved interfaces work with just single
> > > function/tracempoline relation
> > 
> > Do you know which aspect of that is slow? e.g. is that becuase you have
> > to update each ftrace_ops independently, and pay the synchronization
> > overhead per-ops?
> > 
> > I ask because it might be possible to do some more batching there, at
> > least for architectures like arm64 that use the CALL_OPS approach.
> 
> IIRC it's the rcu sync in register_ftrace_direct and ftrace_shutdown
> I'll try to profile that case again, there  might have been changes
> since the last time we did that

Do you mean synchronize_rcu_tasks()?

The call in register_ftrace_direct() was removed in commit:

  33f137143e651321 ("ftrace: Use asynchronous grace period for register_ftrace_direct()")

... but in ftrace_shutdown() we still have a call to synchronize_rcu_tasks(),
and to synchronize_rcu_tasks_rude().

The call to synchronize_rcu_tasks() is still necessary, but we might be
abel to batch that better with API changes.

I think we might be able to remove the call to
synchronize_rcu_tasks_rude() on architectures with ARCH_WANTS_NO_INSTR,
since there shouldn't be any instrumentable functions called with RCU
not watching. That'd need to be checked.

[...]

> > > sorry I probably forgot/missed discussion on this, but doing the fast path like in
> > > x86_64 is not an option in arm, right?
> > 
> > On arm64 we have a fast path, BUT branch range limitations means that we
> > cannot always branch directly from the instrumented function to the
> > direct func with a single branch instruction. We use ops->direct_call to
> > handle that case within a common trampoline, which is significantly
> > cheaper that iterating over the ops and/or looking up the direct func
> > from a hash.
> > 
> > With CALL_OPS, we place a pointer to the ops immediately before the
> > instrumented function, and have the instrumented function branch to a
> > common trampoline which can load that pointer (and can then branch to
> > any direct func as necessary).
> > 
> > The instrumented function looks like:
> > 
> > 	# Aligned to 8 bytes
> > 	func - 8:
> > 		< pointer to ops >
> 
> stupid question.. so there's ftrace_ops pointer stored for each function at
> 'func - 8` ?  why not store the func's direct trampoline address in there?

Once reason is that today we don't have trampolines for all ops. Since
branch range limitations can require bouncing through the common ops,
it's simpler/better to bounce from that to the regular call than to
bounce from that to a trampoline which makes the regular call.

We *could* consider adding trampolines, but that comes with a jump in
complexity that we originally tried to avoid, and a potential
performance hit for regular ftrace calls. IIUC that will require similar
synchronization to what we have today, so it's not clearly a win
generally.

I'd like to better understand what the real bottleneck is; AFAICT it's
the tasks-rcu synchronization, and sharing the hash means that you only
need to do that once. I think that it should be possible to share that
synchronization across multiple ops updates with some API changes (e.g.
something like the batching of text_poke on x86).

If we can do that, it might benefit other users too (e.g.
live-patching), even if trampolines aren't being used, and would keep
the arch bits simple/maintainable.

[...]

> thanks for all the details, I'll check if both the new change and ops->direct_call
> could live together for x86 and other arch, but it will probably complicate
> things a lot more

Thanks; please let me know if there's any challenges there!

Mark.

Powered by blists - more mailing lists