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. Oleg.
