On Sun, Sep 8, 2024 at 6:15 AM Oleg Nesterov <oleg@xxxxxxxxxx> wrote: > > On 09/08, Tianyi Liu wrote: > > > > On Mon, Sep 06, 2024 at 18:43:00AM +0800, Jiri Olsa wrote: > > > > > would you consider sending another version addressing Oleg's points > > > for changelog above? > > > > My pleasure, I'll resend the updated patch in a new thread. > > > > Based on previous discussions, `uprobe_perf_filter` acts as a preliminary > > filter that removes breakpoints when they are no longer needed. > > Well. Not only. See the usage of consumer_filter() and filter_chain() in > register_for_each_vma(). > > > More complex filtering mechanisms related to perf are implemented in > > perf-specific paths. > > The perf paths in __uprobe_perf_func() do the filtering based on > perf_event->hw.target, that is all. > > But uprobe_perf_filter() or any other consumer->filter() simply can't rely > on pid/task, it has to check ->mm. > > > From my understanding, the original patch attempted to partially implement > > UPROBE_HANDLER_REMOVE (since it didn't actually remove the breakpoint but > > only prevented it from entering the BPF-related code). > > Confused... > > Your patch can help bpftrace although it (or any other change in > trace_uprobe.c) can't not actually fix all the problems with bpf/filtering > even if we forget about ret-probes. > > And I don't understand how this relates to UPROBE_HANDLER_REMOVE... > > > I'm trying to provide a complete implementation, i.e., removing the > > breakpoint when `uprobe_perf_filter` returns false, similar to how uprobe > > functions. However, this would require merging the following functions, > > because they will almost be the same: > > > > uprobe_perf_func / uretprobe_perf_func > > uprobe_dispatcher / uretprobe_dispatcher > > handler_chain / handle_uretprobe_chain > > Sorry, I don't understand... Yes, uprobe_dispatcher and uretprobe_dispatcher > can share more code or even unified, but > > > I suspect that uretprobe might have been implemented later than uprobe > > Yes, > > > and was only partially implemented. > > what do you mean? > > But whatever you meant, I agree that this code doesn't look pretty and can > be cleanuped. > > > In your opinion, does uretprobe need UPROBE_HANDLER_REMOVE? > > Probably. But this has absolutely nothing to do with the filtering problem? > Can we discuss this separately? > > > I'm aware that using `uprobe_perf_filter` in `uretprobe_perf_func` is not > > the solution for BPF filtering. I'm just trying to alleviate the issue > > in some simple cases. > > Agreed. > > ------------------------------------------------------------------------------- > To summarise. > > This code is very old, and it was written for /usr/bin/perf which attaches > to the tracepoint. So multiple instances of perf-record will share the same > consumer/trace_event_call/filter. uretprobe_perf_func() doesn't call > uprobe_perf_filter() because (if /usr/bin/perf is the only user) in the likely > case it would burn CPU and return true. Quite possibly this design was not > optimal from the very beginning, I simply can't recall why the is_ret_probe() > consumer has ->handler != NULL, but it was not buggy. > > Now we have bpf, create_local_trace_uprobe(), etc. So lets add another > uprobe_perf_filter() into uretprobe_perf_func() as your patch did. > > Then we can probably change uprobe_handle_trampoline() to do > unapply_uprobe() if all the ret-handlers return UPROBE_HANDLER_REMOVE, like > handler_chain() does. > > Then we can probably cleanup/simplify trace_uprobe.c, in partucular we can > change alloc_trace_uprobe() > > - tu->consumer.handler = uprobe_dispatcher; > - if (is_ret) > - tu->consumer.ret_handler = uretprobe_dispatcher; > + if (is_ret) > + tu->consumer.ret_handler = uretprobe_dispatcher; > + else > + tu->consumer.handler = uprobe_dispatcher; > > and do more (including unrelated) cleanups. > > But lets do this step-by-step. > > And lets not mix the filtering issues with the UPROBE_HANDLER_REMOVE logic, > to me this adds the unnecessary confusion. +100 to all of the above. This has been a very long and winding thread, but uretprobes are still being triggered unnecessarily :) Let's fix the bleeding and talk and work on improving all of the other issues in separate efforts. > > Oleg. >
