On Fri, Aug 04, 2023 at 12:42:06AM +0900, Masami Hiramatsu wrote: SNIP > > > > On the other hand, untangling all code paths that come from > > trampolines (with a light regs structure) from those that come from an > > exception (with a pt_regs) could lead to a lot of duplicated code, and > > converting between each subsystem's idea of a light regs structure > > (what if perf introduces a perf_regs now ?) would be tedious and slow > > (lots of copies ?). > > This is one discussion point I think. Actually, using pt_regs in kretprobe > (and rethook) is histrical accident. Originally, it had put a kprobe on > the function return trampoline to hook it. So keep the API compatiblity > I made the hand assembled code to save the pt_regs on the stack. > > My another question is if we have the fprobe to trace (hook) the function > return, why we still need the kretprobe itself. I think we can remove > kretprobe and use fprobe exit handler, because "function" probing will > be done by fprobe, not kprobe. And then, we can simplify the kprobe > interface and clarify what it is -- "kprobe is a wrapper of software > breakpoint". And we don't need to think about duplicated code anymore :) 1+ sounds like good idea > > > > > > Otherwise, ftrace_regs() has support on arm64 for getting to the argument > > > registers and the stack. Even live kernel patching now uses ftrace_regs(). > > > > > > > > > > > If you guys decide to convert fprobe to ftrace_regs please > > > > make it depend on kconfig or something. > > > > bpf side needs full pt_regs. > > > > Some wild ideas that I brought up once in a BPF office hour: BPF > > "multi_kprobe" could provide a fake pt_regs (either by constructing a > > sparse one on the stack or by JIT-ing different offset accesses and/or > > by having the verifier deny access to unpopulated fields) or break the > > current API (is it conceivable to phase out BPF "multi_kprobe" > > programs in favor of BPF "fprobe" programs that don't lie about their > > API and guarantees and just provide a ftrace_regs ?) > > +1 :) so multi_kprobe link was created to allow fast attach of BPF kprobe-type programs to multiple functions.. I don't think there's need for new fprobe program > > > > > > Then use kprobes. When I asked Masami what the difference between fprobes > > > and kprobes was, he told me that it would be that it would no longer rely > > > on the slower FTRACE_WITH_REGS. But currently, it still does. > > > > Actually... Moving fprobe to ftrace_regs should get even more spicy! > > :) Fprobe also wraps "rethook" which is basically the same thing as > > kretprobe: a return trampoline that saves a pt_regs, to the point that > > on x86 kretprobe's trampoline got dropped in favor of rethook's > > trampoline. But for the same reasons that we don't want ftrace to save > > pt_regs on arm64, rethook should probably also just save a ftrace_regs > > ? (also, to keep the fprobe callback signatures consistent between > > pre- and post- handlers). But if we want fprobe "post" callbacks to > > save a ftrace_regs now, either we need to re-introduce the kretprobe > > trampoline or also change the API of kretprobe (and break its symmetry > > with kprobe and we'd have the same problem all over again with BPF > > kretprobe program types...). All of this is "beautifully" entangled... > > :) > > As I said, I would like to phase out the kretprobe itself because it > provides the same feature of fprobe, which is confusing. jprobe was > removed a while ago, and now kretprobe is. But we can not phase out > it at once. So I think we will keep current kretprobe trampoline on > arm64 and just add new ftrace_regs based rethook. Then remove the > API next release. (after all users including systemtap is moved) > > > > > > The reason I started the FTRACE_WITH_ARGS (which gave us ftrace_regs) in > > > the first place, was because of the overhead you reported to me with > > > ftrace_regs_caller and why you wanted to go the direct trampoline approach. > > > That's when I realized I could use a subset because those registers were > > > already being saved. The only reason FTRACE_WITH_REGS was created was it > > > had to supply full pt_regs (including flags) and emulate a breakpoint for > > > the kprobes interface. But in reality, nothing really needs all that. > > > > > > > It's not about access to args. > > > > pt_regs is passed from bpf prog further into all kinds of perf event > > > > functions including stack walking. > > > > If all accesses are done in BPF bytecode, we could (theoretically) > > have the verifier and JIT work together to deny accesses to > > unpopulated fields, or relocate pt_regs accesses to ftrace_regs > > accesses to keep backward compatibility with existing multi_kprobe BPF > > programs. > > Yeah, that is what I would like to suggest, and what my patch does. > (let me update rethook too, it'll be a bit tricky since I don't want > break anything) > > Thanks, > > > > > Is there a risk that a "multi_kprobe" program could call into a BPF > > helper or kfunc that reads this pt_regs pointer and expect certain > > fields to be set ? I suppose we could also deny giving that "pt_regs" > > pointer to a helper... :/ I think Alexei answered this earlier in the thread: >From bpf side we don't care that such pt_regs is 100% filled in or >only partial as long as this pt_regs pointer is valid for perf_event_output >and stack walking that consume pt_regs. >I believe that was and still is the case for both x86 and arm64. jirka
