On Wed, Sep 11, 2024 at 8:26 AM Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote: > > On Tue, 10 Sep 2024 17:44:11 -0700 > Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > > On Tue, Sep 10, 2024 at 5:39 PM Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote: > > > > > > On Tue, 10 Sep 2024 13:29:57 -0700 > > > Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > > > > > > You are probably talking about [0]. But I was asking about [1], i.e., > > > > adding HAVE_RETHOOK support to ARM64. Despite all your emotions above, > > > > can I still get a meaningful answer as for why that wasn't landed and > > > > what prevents it from landing right now before Masami's 20-patch > > > > series lands? > > > > > > As I replied to your last email, Mark discovered that [1] is incorrect. > > > From the bpf perspective, it may be fine that struct pt_regs is missing > > > some architecture-specific registers, but from an API perspective, > > > it is a problem. > > > > > > Actually kretprobes on arm64 still does not do it correctly, but I also > > > know most of users does not care. So currently I keep it as it is. But > > > after fixing this issue on fprobe. I would like to update kretprobe so > > > that it will use sw-breakpoint to handle it. It will increase the overhead > > > of kretprobe, but it should be replaced by fprobe at that moment. > > > > Ok, given kretprobes already have this issue, can we add this support > > for BPF multi-kprobe/kretprobe only? We can have an extra Kconfig > > option or whatever necessary. It's sad that we don't have entire > > feature just because a few registers can't be set (and I bet no BPF > > users ever reads those registers from pt_regs). It's not the first, > > nor last case where pt_regs isn't complete (e.g., tracepoints set only > > a few fields in pt_regs, the rest are zero; and that's fine). > > pt_regs things are asked by PeterZ. It is not recommended to use pt_regs > if it is not actual pt_regs because user expects it works as full pt_regs. Yes, but it is what it is today. Tracepoints, for example, have like 4 fields set to real values and the rest are zeroes. So sure, as complete data as possible is best, but the reality is different. My point is, the feature being available with one or two pt_regs fields not having a real value set is *much* better than no feature availability. There is just no comparison here. And you said yourself, current kretprobe implementation has similar problems, so nothing is regressed. > So I and Steve decided to use ftrace_regs for this faked registers. > (I think tracepoint should also use ftrace_regs or another one.) > > Anyway, we're almost at a goal we can all agree on. I think we would better > push fprobe on fgraph series instead of such ad-hoc change. I really hope you are right, but you see yourself that there are all the small things that pop up and need debugging, fixing, investigation. Performance regression is pretty noticeable. So this might take more time than all of us would like. > > Thank you, > > > > > > > > > Thank you, > > > > > > > > > > > [0] https://lore.kernel.org/linux-trace-kernel/172398527264.293426.2050093948411376857.stgit@devnote2/ > > > > [1] https://lore.kernel.org/bpf/164338038439.2429999.17564843625400931820.stgit@devnote2/ > > > > > > > > > > > > > > Again, just letting you know. > > > > > > > > > > -- Steve > > > > > > > > > -- > > > Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> > > > -- > Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
