From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Daniel Borkmann <daniel@iogearbox.net>
Cc: Andrii Nakryiko <andriin@fb.com>, Alexei Starovoitov <ast@fb.com>,
Stanislav Fomichev <sdf@fomichev.me>, bpf <bpf@vger.kernel.org>,
Networking <netdev@vger.kernel.org>,
Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH v2 bpf-next 3/7] libbpf: add kprobe/uprobe attach API
Date: Wed, 26 Jun 2019 15:15:34 -0700 [thread overview]
Message-ID: <CAEf4BzYy4Eorj0VxzArZg+V4muJCvDTX_VVfoouzZUcrBwTa1w@mail.gmail.com> (raw)
In-Reply-To: <a7780057-1d70-9ace-960b-ff65867dc277@iogearbox.net>
On Wed, Jun 26, 2019 at 7:25 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 06/21/2019 06:55 AM, Andrii Nakryiko wrote:
> > Add ability to attach to kernel and user probes and retprobes.
> > Implementation depends on perf event support for kprobes/uprobes.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
<snip>
> > +}
>
> I do like that we facilitate usage by adding these APIs to libbpf, but my $0.02
> would be that they should be designed slightly different. See it as a nit, but
> given it's exposed in libbpf.map and therefore immutable in future it's worth
> considering; right now with this set here you have:
>
> int bpf_program__attach_kprobe(struct bpf_program *prog, bool retprobe,
> const char *func_name)
> int bpf_program__attach_uprobe(struct bpf_program *prog, bool retprobe,
> pid_t pid, const char *binary_path,
> size_t func_offset)
> int bpf_program__attach_tracepoint(struct bpf_program *prog,
> const char *tp_category,
> const char *tp_name)
> int bpf_program__attach_raw_tracepoint(struct bpf_program *prog,
> const char *tp_name)
> int bpf_program__attach_perf_event(struct bpf_program *prog, int pfd)
> int libbpf_perf_event_disable_and_close(int pfd)
>
> So the idea is that all the bpf_program__attach_*() APIs return an fd that you
> can later on pass into libbpf_perf_event_disable_and_close(). I think there is
> a bit of a disconnect in that the bpf_program__attach_*() APIs try to do too
> many things at once. For example, the bpf_program__attach_raw_tracepoint() fd
> has nothing to do with perf, so passing to libbpf_perf_event_disable_and_close()
> kind of works, but is hacky since there's no PERF_EVENT_IOC_DISABLE for it so this
> would always error if a user cares to check the return code. In the kernel, we
Yeah, you are absolutely right, missed that it's not creating perf
event under cover, to be honest.
> use anon inode for this kind of object. Also, if a user tries to add more than
> one program to the same event, we need to recreate a new event fd every time.
>
> What this boils down to is that this should get a proper abstraction, e.g. as
> in struct libbpf_event which holds the event object. There should be helper
> functions like libbpf_event_create_{kprobe,uprobe,tracepoint,raw_tracepoint} returning
> such an struct libbpf_event object on success, and a single libbpf_event_destroy()
> that does the event specific teardown. bpf_program__attach_event() can then take
> care of only attaching the program to it. Having an object for this is also more
> extensible than just a fd number. Nice thing is that this can also be completely
> internal to libbpf.c as with struct bpf_program and other abstractions where we
> don't expose the internals in the public header.
Yeah, I totally agree, I think this is a great idea! I don't
particularly like "event" name, that seems very overloaded term. Do
you mind if I call this "bpf_hook" instead of "libbpf_event"? I've
always thought about these different points in the system to which one
can attach BPF program as hooks exposed from kernel :)
Would it also make sense to do attaching to non-tracing hooks using
the same mechanism (e.g., all the per-cgroup stuff, sysctl, etc)? Not
sure how people do that today, will check to see how it's done, but I
think nothing should conceptually prevent doing that using the same
abstract bpf_hook way, right?
>
> Thanks,
> Daniel
next prev parent reply other threads:[~2019-06-26 22:15 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-21 4:55 [PATCH v2 bpf-next 0/7] libbpf: add tracing attach APIs Andrii Nakryiko
2019-06-21 4:55 ` [PATCH v2 bpf-next 1/7] libbpf: make libbpf_strerror_r agnostic to sign of error Andrii Nakryiko
2019-06-21 4:55 ` [PATCH v2 bpf-next 2/7] libbpf: add ability to attach/detach BPF to perf event Andrii Nakryiko
2019-06-21 4:55 ` [PATCH v2 bpf-next 3/7] libbpf: add kprobe/uprobe attach API Andrii Nakryiko
2019-06-26 14:25 ` Daniel Borkmann
2019-06-26 22:15 ` Andrii Nakryiko [this message]
2019-06-27 21:16 ` Daniel Borkmann
2019-06-27 21:42 ` Andrii Nakryiko
2019-06-21 4:55 ` [PATCH v2 bpf-next 4/7] libbpf: add tracepoint/raw tracepoint " Andrii Nakryiko
2019-06-21 4:55 ` [PATCH v2 bpf-next 5/7] selftests/bpf: switch test to new attach_perf_event API Andrii Nakryiko
2019-06-21 4:55 ` [PATCH v2 bpf-next 6/7] selftests/bpf: add kprobe/uprobe selftests Andrii Nakryiko
2019-06-21 4:55 ` [PATCH v2 bpf-next 7/7] selftests/bpf: convert existing tracepoint tests to new APIs Andrii Nakryiko
2019-06-21 21:21 ` [PATCH v2 bpf-next 0/7] libbpf: add tracing attach APIs Stanislav Fomichev
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAEf4BzYy4Eorj0VxzArZg+V4muJCvDTX_VVfoouzZUcrBwTa1w@mail.gmail.com \
--to=andrii.nakryiko@gmail.com \
--cc=andriin@fb.com \
--cc=ast@fb.com \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kernel-team@fb.com \
--cc=netdev@vger.kernel.org \
--cc=sdf@fomichev.me \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).