diff mbox series

[RFC,bpf-next,13/16] libbpf: Add trampoline batch attach support

Message ID 20201022082138.2322434-14-jolsa@kernel.org
State Not Applicable
Delegated to: BPF Maintainers
Headers show
Series bpf: Speed up trampoline attach | expand

Checks

Context Check Description
jkicinski/stable success Stable not CCed
jkicinski/header_inline success Link
jkicinski/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
jkicinski/checkpatch warning CHECK: Alignment should match open parenthesis CHECK: Please use a blank line after function/struct/union/enum declarations
jkicinski/verify_fixes success Link
jkicinski/kdoc success Errors and warnings before: 0 this patch: 0
jkicinski/build_32bit success Errors and warnings before: 0 this patch: 0
jkicinski/module_param success Was 0 now: 0
jkicinski/verify_signedoff success Link
jkicinski/source_inline success Was 0 now: 0
jkicinski/subject_prefix success Link
jkicinski/tree_selection success Clearly marked for bpf-next
jkicinski/patch_count fail Series longer than 15 patches
jkicinski/fixes_present success Link
jkicinski/cover_letter success Link

Commit Message

Jiri Olsa Oct. 22, 2020, 8:21 a.m. UTC
Adding trampoline batch attach support so it's possible to use
batch mode to load tracing programs.

Adding trampoline_attach_batch bool to struct bpf_object_open_opts.
When set to true the bpf_object__attach_skeleton will try to load
all tracing programs via batch mode.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/lib/bpf/bpf.c      | 12 +++++++
 tools/lib/bpf/bpf.h      |  1 +
 tools/lib/bpf/libbpf.c   | 76 +++++++++++++++++++++++++++++++++++++++-
 tools/lib/bpf/libbpf.h   |  5 ++-
 tools/lib/bpf/libbpf.map |  1 +
 5 files changed, 93 insertions(+), 2 deletions(-)

Comments

Andrii Nakryiko Oct. 23, 2020, 8:09 p.m. UTC | #1
On Thu, Oct 22, 2020 at 2:03 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding trampoline batch attach support so it's possible to use
> batch mode to load tracing programs.
>
> Adding trampoline_attach_batch bool to struct bpf_object_open_opts.
> When set to true the bpf_object__attach_skeleton will try to load
> all tracing programs via batch mode.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---

Assuming we go with the current kernel API for batch-attach, why can't
libbpf just detect kernel support for it and just use it always,
without requiring users to opt into anything?

But I'm also confused a bit how this is supposed to be used with BPF
skeleton. You use case described in a cover letter (bpftrace glob
attach, right?) would have a single BPF program attached to many
different functions. While here you are trying to collect different
programs and attach each one to its respective kernel function. Do you
expect users to have hundreds of BPF programs in their skeletons? If
not, I don't really see why adding this complexity. What am I missing?

Now it also seems weird to me for the kernel API to allow attaching
many-to-many BPF programs-to-attach points. One BPF program-to-many
attach points seems like a more sane and common requirement, no?


>  tools/lib/bpf/bpf.c      | 12 +++++++
>  tools/lib/bpf/bpf.h      |  1 +
>  tools/lib/bpf/libbpf.c   | 76 +++++++++++++++++++++++++++++++++++++++-
>  tools/lib/bpf/libbpf.h   |  5 ++-
>  tools/lib/bpf/libbpf.map |  1 +
>  5 files changed, 93 insertions(+), 2 deletions(-)
>

[...]
Jiri Olsa Oct. 25, 2020, 7:11 p.m. UTC | #2
On Fri, Oct 23, 2020 at 01:09:26PM -0700, Andrii Nakryiko wrote:
> On Thu, Oct 22, 2020 at 2:03 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding trampoline batch attach support so it's possible to use
> > batch mode to load tracing programs.
> >
> > Adding trampoline_attach_batch bool to struct bpf_object_open_opts.
> > When set to true the bpf_object__attach_skeleton will try to load
> > all tracing programs via batch mode.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> 
> Assuming we go with the current kernel API for batch-attach, why can't
> libbpf just detect kernel support for it and just use it always,
> without requiring users to opt into anything?

yea, it's rfc ;-) I wanted some simple usage of the
interface so it's obvious how it works

if we'll end up with some batch interface I agree
we should use it as you suggested

> 
> But I'm also confused a bit how this is supposed to be used with BPF
> skeleton. You use case described in a cover letter (bpftrace glob
> attach, right?) would have a single BPF program attached to many
> different functions. While here you are trying to collect different
> programs and attach each one to its respective kernel function. Do you
> expect users to have hundreds of BPF programs in their skeletons? If
> not, I don't really see why adding this complexity. What am I missing?

AFAIU when you use trampoline program you declare the attach point
at the load time, so you actually can't use same program for different
kernel functions - which would be great speed up actually, because
that's where the rest of the cycles in bpftrace is spent (in that cover
letter example) - load/verifier check of all those programs

it's different for kprobe where you hook single kprobe via multiple
kprobe perf events to different kernel function

> 
> Now it also seems weird to me for the kernel API to allow attaching
> many-to-many BPF programs-to-attach points. One BPF program-to-many
> attach points seems like a more sane and common requirement, no?

right, but that's the consequence of what I wrote above

jirka

> 
> 
> >  tools/lib/bpf/bpf.c      | 12 +++++++
> >  tools/lib/bpf/bpf.h      |  1 +
> >  tools/lib/bpf/libbpf.c   | 76 +++++++++++++++++++++++++++++++++++++++-
> >  tools/lib/bpf/libbpf.h   |  5 ++-
> >  tools/lib/bpf/libbpf.map |  1 +
> >  5 files changed, 93 insertions(+), 2 deletions(-)
> >
> 
> [...]
>
Andrii Nakryiko Oct. 26, 2020, 11:15 p.m. UTC | #3
On Sun, Oct 25, 2020 at 12:12 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Fri, Oct 23, 2020 at 01:09:26PM -0700, Andrii Nakryiko wrote:
> > On Thu, Oct 22, 2020 at 2:03 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Adding trampoline batch attach support so it's possible to use
> > > batch mode to load tracing programs.
> > >
> > > Adding trampoline_attach_batch bool to struct bpf_object_open_opts.
> > > When set to true the bpf_object__attach_skeleton will try to load
> > > all tracing programs via batch mode.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> >
> > Assuming we go with the current kernel API for batch-attach, why can't
> > libbpf just detect kernel support for it and just use it always,
> > without requiring users to opt into anything?
>
> yea, it's rfc ;-) I wanted some simple usage of the
> interface so it's obvious how it works
>
> if we'll end up with some batch interface I agree
> we should use it as you suggested
>
> >
> > But I'm also confused a bit how this is supposed to be used with BPF
> > skeleton. You use case described in a cover letter (bpftrace glob
> > attach, right?) would have a single BPF program attached to many
> > different functions. While here you are trying to collect different
> > programs and attach each one to its respective kernel function. Do you
> > expect users to have hundreds of BPF programs in their skeletons? If
> > not, I don't really see why adding this complexity. What am I missing?
>
> AFAIU when you use trampoline program you declare the attach point
> at the load time, so you actually can't use same program for different
> kernel functions - which would be great speed up actually, because
> that's where the rest of the cycles in bpftrace is spent (in that cover
> letter example) - load/verifier check of all those programs

Ah, I see, you are right. And yes, I agree, it would be nice to not
have to clone the BPF program many times to attach to fentry/fexit, if
the program itself doesn't really change.

>
> it's different for kprobe where you hook single kprobe via multiple
> kprobe perf events to different kernel function
>
> >
> > Now it also seems weird to me for the kernel API to allow attaching
> > many-to-many BPF programs-to-attach points. One BPF program-to-many
> > attach points seems like a more sane and common requirement, no?
>
> right, but that's the consequence of what I wrote above

Well, maybe we should get rid of that limitation first ;)

>
> jirka
>
> >
> >
> > >  tools/lib/bpf/bpf.c      | 12 +++++++
> > >  tools/lib/bpf/bpf.h      |  1 +
> > >  tools/lib/bpf/libbpf.c   | 76 +++++++++++++++++++++++++++++++++++++++-
> > >  tools/lib/bpf/libbpf.h   |  5 ++-
> > >  tools/lib/bpf/libbpf.map |  1 +
> > >  5 files changed, 93 insertions(+), 2 deletions(-)
> > >
> >
> > [...]
> >
>
Jiri Olsa Oct. 27, 2020, 7:03 p.m. UTC | #4
On Mon, Oct 26, 2020 at 04:15:48PM -0700, Andrii Nakryiko wrote:
> On Sun, Oct 25, 2020 at 12:12 PM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > On Fri, Oct 23, 2020 at 01:09:26PM -0700, Andrii Nakryiko wrote:
> > > On Thu, Oct 22, 2020 at 2:03 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > >
> > > > Adding trampoline batch attach support so it's possible to use
> > > > batch mode to load tracing programs.
> > > >
> > > > Adding trampoline_attach_batch bool to struct bpf_object_open_opts.
> > > > When set to true the bpf_object__attach_skeleton will try to load
> > > > all tracing programs via batch mode.
> > > >
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > >
> > > Assuming we go with the current kernel API for batch-attach, why can't
> > > libbpf just detect kernel support for it and just use it always,
> > > without requiring users to opt into anything?
> >
> > yea, it's rfc ;-) I wanted some simple usage of the
> > interface so it's obvious how it works
> >
> > if we'll end up with some batch interface I agree
> > we should use it as you suggested
> >
> > >
> > > But I'm also confused a bit how this is supposed to be used with BPF
> > > skeleton. You use case described in a cover letter (bpftrace glob
> > > attach, right?) would have a single BPF program attached to many
> > > different functions. While here you are trying to collect different
> > > programs and attach each one to its respective kernel function. Do you
> > > expect users to have hundreds of BPF programs in their skeletons? If
> > > not, I don't really see why adding this complexity. What am I missing?
> >
> > AFAIU when you use trampoline program you declare the attach point
> > at the load time, so you actually can't use same program for different
> > kernel functions - which would be great speed up actually, because
> > that's where the rest of the cycles in bpftrace is spent (in that cover
> > letter example) - load/verifier check of all those programs
> 
> Ah, I see, you are right. And yes, I agree, it would be nice to not
> have to clone the BPF program many times to attach to fentry/fexit, if
> the program itself doesn't really change.
> 
> >
> > it's different for kprobe where you hook single kprobe via multiple
> > kprobe perf events to different kernel function
> >
> > >
> > > Now it also seems weird to me for the kernel API to allow attaching
> > > many-to-many BPF programs-to-attach points. One BPF program-to-many
> > > attach points seems like a more sane and common requirement, no?
> >
> > right, but that's the consequence of what I wrote above
> 
> Well, maybe we should get rid of that limitation first ;)
>

I see this as 2 different things.. even when this would be
possible - attach single program to multiple trampolines,
you still need to install those trampolines via ftrace and
you'll end up in this discussion ;-)

jirka
diff mbox series

Patch

diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
index d27e34133973..21fffff5e237 100644
--- a/tools/lib/bpf/bpf.c
+++ b/tools/lib/bpf/bpf.c
@@ -858,6 +858,18 @@  int bpf_raw_tracepoint_open(const char *name, int prog_fd)
 	return sys_bpf(BPF_RAW_TRACEPOINT_OPEN, &attr, sizeof(attr));
 }
 
+int bpf_trampoline_batch_attach(int *ifds, int *ofds, int count)
+{
+	union bpf_attr attr;
+
+	memset(&attr, 0, sizeof(attr));
+	attr.trampoline_batch.in = ptr_to_u64(ifds);
+	attr.trampoline_batch.out = ptr_to_u64(ofds);
+	attr.trampoline_batch.count = count;
+
+	return sys_bpf(BPF_TRAMPOLINE_BATCH_ATTACH, &attr, sizeof(attr));
+}
+
 int bpf_load_btf(const void *btf, __u32 btf_size, char *log_buf, __u32 log_buf_size,
 		 bool do_log)
 {
diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
index 875dde20d56e..ba3b0b6e3cb0 100644
--- a/tools/lib/bpf/bpf.h
+++ b/tools/lib/bpf/bpf.h
@@ -235,6 +235,7 @@  LIBBPF_API int bpf_prog_query(int target_fd, enum bpf_attach_type type,
 			      __u32 query_flags, __u32 *attach_flags,
 			      __u32 *prog_ids, __u32 *prog_cnt);
 LIBBPF_API int bpf_raw_tracepoint_open(const char *name, int prog_fd);
+LIBBPF_API int bpf_trampoline_batch_attach(int *ifds, int *ofds, int count);
 LIBBPF_API int bpf_load_btf(const void *btf, __u32 btf_size, char *log_buf,
 			    __u32 log_buf_size, bool do_log);
 LIBBPF_API int bpf_task_fd_query(int pid, int fd, __u32 flags, char *buf,
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 313034117070..584da3b401ac 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -421,6 +421,7 @@  struct bpf_object {
 
 	bool loaded;
 	bool has_subcalls;
+	bool trampoline_attach_batch;
 
 	/*
 	 * Information when doing elf related work. Only valid if fd
@@ -6907,6 +6908,9 @@  __bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
 			return ERR_PTR(-ENOMEM);
 	}
 
+	obj->trampoline_attach_batch = OPTS_GET(opts, trampoline_attach_batch,
+						 false);
+
 	err = bpf_object__elf_init(obj);
 	err = err ? : bpf_object__check_endianness(obj);
 	err = err ? : bpf_object__elf_collect(obj);
@@ -10811,9 +10815,75 @@  int bpf_object__load_skeleton(struct bpf_object_skeleton *s)
 	return 0;
 }
 
+static bool is_trampoline(const struct bpf_program *prog)
+{
+	return prog->type == BPF_PROG_TYPE_TRACING &&
+	      (prog->expected_attach_type == BPF_TRACE_FENTRY ||
+	       prog->expected_attach_type == BPF_TRACE_FEXIT);
+}
+
+static int attach_trace_batch(struct bpf_object_skeleton *s)
+{
+	int i, prog_fd, ret = -ENOMEM;
+	int *in_fds, *out_fds, cnt;
+
+	in_fds = calloc(s->prog_cnt, sizeof(in_fds[0]));
+	out_fds = calloc(s->prog_cnt, sizeof(out_fds[0]));
+	if (!in_fds || !out_fds)
+		goto out_clean;
+
+	ret = -EINVAL;
+	for (cnt = 0, i = 0; i < s->prog_cnt; i++) {
+		struct bpf_program *prog = *s->progs[i].prog;
+
+		if (!is_trampoline(prog))
+			continue;
+
+		prog_fd = bpf_program__fd(prog);
+		if (prog_fd < 0) {
+			pr_warn("prog '%s': can't attach before loaded\n", prog->name);
+			goto out_clean;
+		}
+		in_fds[cnt++] = prog_fd;
+	}
+
+	ret = bpf_trampoline_batch_attach(in_fds, out_fds, cnt);
+	if (ret)
+		goto out_clean;
+
+	for (cnt = 0, i = 0; i < s->prog_cnt; i++) {
+		struct bpf_program *prog = *s->progs[i].prog;
+		struct bpf_link **linkp = s->progs[i].link;
+		struct bpf_link *link;
+
+		if (!is_trampoline(prog))
+			continue;
+
+		link = calloc(1, sizeof(*link));
+		if (!link)
+			goto out_clean;
+
+		link->detach = &bpf_link__detach_fd;
+		link->fd = out_fds[cnt++];
+		*linkp = link;
+	}
+
+out_clean:
+	free(in_fds);
+	free(out_fds);
+	return ret;
+}
+
 int bpf_object__attach_skeleton(struct bpf_object_skeleton *s)
 {
-	int i;
+	struct bpf_object *obj = *s->obj;
+	int i, err;
+
+	if (obj->trampoline_attach_batch) {
+		err = attach_trace_batch(s);
+		if (err)
+			return err;
+	}
 
 	for (i = 0; i < s->prog_cnt; i++) {
 		struct bpf_program *prog = *s->progs[i].prog;
@@ -10823,6 +10893,10 @@  int bpf_object__attach_skeleton(struct bpf_object_skeleton *s)
 		if (!prog->load)
 			continue;
 
+		/* Program was attached via batch mode. */
+		if (obj->trampoline_attach_batch && is_trampoline(prog))
+			continue;
+
 		sec_def = find_sec_def(prog->sec_name);
 		if (!sec_def || !sec_def->attach_fn)
 			continue;
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 6909ee81113a..66f8e78aa9f8 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -93,8 +93,11 @@  struct bpf_object_open_opts {
 	 * system Kconfig for CONFIG_xxx externs.
 	 */
 	const char *kconfig;
+	/* Attach trampolines via batch mode.
+	 */
+	bool trampoline_attach_batch;
 };
-#define bpf_object_open_opts__last_field kconfig
+#define bpf_object_open_opts__last_field trampoline_attach_batch
 
 LIBBPF_API struct bpf_object *bpf_object__open(const char *path);
 LIBBPF_API struct bpf_object *
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 4ebfadf45b47..5a5ce921956d 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -336,4 +336,5 @@  LIBBPF_0.2.0 {
 		perf_buffer__epoll_fd;
 		perf_buffer__consume_buffer;
 		xsk_socket__create_shared;
+		bpf_trampoline_batch_attach;
 } LIBBPF_0.1.0;