On Fri, Jul 5, 2019 at 1:46 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote: > > On 07/04/2019 02:57 AM, Andrii Nakryiko wrote: > > On Wed, Jul 3, 2019 at 9:47 AM Andrii Nakryiko > > <andrii.nakryiko@xxxxxxxxx> wrote: > [...] > >> [1] https://lore.kernel.org/bpf/20190621045555.4152743-4-andriin@xxxxxx/T/#m6cfc141e7b57970bc948134bf671a46972b95134 > >> > >>> with bpf_link with destructor looks good to me, but my feedback from back then was > >>> that all the kprobe/uprobe/tracepoint/raw_tracepoint should be split API-wise, so > >>> you'll end up with something like the below, that is, 1) a set of functions that > >>> only /create/ the bpf_link handle /once/, and 2) a helper that allows /attaching/ > >>> progs to one or multiple bpf_links. The set of APIs would look like: > >>> > >>> struct bpf_link *bpf_link__create_kprobe(bool retprobe, const char *func_name); > >>> struct bpf_link *bpf_link__create_uprobe(bool retprobe, pid_t pid, > >>> const char *binary_path, > >>> size_t func_offset); > >>> int bpf_program__attach_to_link(struct bpf_link *link, struct bpf_program *prog); > >>> int bpf_link__destroy(struct bpf_link *link); > >>> > >>> This seems much more natural to me. Right now you sort of do both in one single API. > >> > >> It felt that way for me as well, until I implemented it and used it in > >> selftests. And then it felt unnecessarily verbose without giving any > >> benefit. I still have a local patchset with that change, I can post it > >> as RFC, if you don't trust my judgement. Please let me know. > >> > >>> Detangling the bpf_program__attach_{uprobe,kprobe}() would also avoid that you have > >>> to redo all the perf_event_open_probe() work over and over in order to get the pfd > > > > So re-reading this again, I wonder if you meant that with separate > > bpf_link (or rather bpf_hook in that case) creation and attachment > > operations, one would be able to create single bpf_hook for same > > kprobe and then attach multiple BPF programs to that single pfd > > representing that specific probe. > > > > If that's how I should have read it, I agree that it probably would be > > possible for some types of hooks, but not for every type of hook. But > > furthermore, how often in practice same application attaches many > > different BPF programs to the same hook? And it's also hard to imagine > > that hook creation (i.e., creating such FD for BPF hook), would ever > > be a bottleneck. > > > > So I still think it's not a strong reason to go with API that's harder > > to use for typical use cases just because of hypothetical benefits in > > some extreme cases. > > Was thinking along that lines, yes, as we run over an array of BPF progs, > but I just double checked the kernel code again and the relationship of > a BPF prog to perf_event is really just 1:1, just that the backing tp_event > (trace_event_call) contains the shared array. Given that, all makes sense > and there is no point in splitting. Therefore, applied, thanks! Great, thanks a lot!
