Message ID | 158151067149.71757.2222114135650741733.stgit@xdp-tutorial |
---|---|
State | Changes Requested |
Delegated to: | BPF Maintainers |
Headers | show |
Series | [bpf-next] libbpf: Add support for dynamic program attach target | expand |
Eelco Chaudron <echaudro@redhat.com> writes: > Currently when you want to attach a trace program to a bpf program > the section name needs to match the tracepoint/function semantics. > > However the addition of the bpf_program__set_attach_target() API > allows you to specify the tracepoint/function dynamically. > > The call flow would look something like this: > > xdp_fd = bpf_prog_get_fd_by_id(id); > trace_obj = bpf_object__open_file("func.o", NULL); > prog = bpf_object__find_program_by_title(trace_obj, > "fentry/myfunc"); > bpf_program__set_attach_target(prog, xdp_fd, > "fentry/xdpfilt_blk_all"); I think it would be better to have the attach type as a separate arg instead of encoding it in the function name. I.e., rather: bpf_program__set_attach_target(prog, xdp_fd, "xdpfilt_blk_all", BPF_TRACE_FENTRY); -Toke
On Wed, Feb 12, 2020 at 4:32 AM Eelco Chaudron <echaudro@redhat.com> wrote: > > Currently when you want to attach a trace program to a bpf program > the section name needs to match the tracepoint/function semantics. > > However the addition of the bpf_program__set_attach_target() API > allows you to specify the tracepoint/function dynamically. > > The call flow would look something like this: > > xdp_fd = bpf_prog_get_fd_by_id(id); > trace_obj = bpf_object__open_file("func.o", NULL); > prog = bpf_object__find_program_by_title(trace_obj, > "fentry/myfunc"); > bpf_program__set_attach_target(prog, xdp_fd, > "fentry/xdpfilt_blk_all"); > bpf_object__load(trace_obj) > > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > --- > tools/lib/bpf/libbpf.c | 41 +++++++++++++++++++++++++++++++++++------ > tools/lib/bpf/libbpf.h | 4 ++++ > tools/lib/bpf/libbpf.map | 1 + > 3 files changed, 40 insertions(+), 6 deletions(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 514b1a524abb..2ce879c301bb 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -4933,15 +4933,16 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt, > return ret; > } > > -static int libbpf_find_attach_btf_id(struct bpf_program *prog); > +static int libbpf_find_attach_btf_id(struct bpf_program *prog, > + const char *name); > > int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver) > { > int err = 0, fd, i, btf_id; > > - if (prog->type == BPF_PROG_TYPE_TRACING || > - prog->type == BPF_PROG_TYPE_EXT) { > - btf_id = libbpf_find_attach_btf_id(prog); > + if ((prog->type == BPF_PROG_TYPE_TRACING || > + prog->type == BPF_PROG_TYPE_EXT) && !prog->attach_btf_id) { > + btf_id = libbpf_find_attach_btf_id(prog, NULL); > if (btf_id <= 0) > return btf_id; > prog->attach_btf_id = btf_id; > @@ -6202,6 +6203,31 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog, > prog->expected_attach_type = type; > } > > +int bpf_program__set_attach_target(struct bpf_program *prog, > + int attach_prog_fd, > + const char *attach_func_name) > +{ > + __u32 org_attach_prog_fd; > + int btf_id; > + > + if (!prog || attach_prog_fd < 0 || !attach_func_name) > + return -EINVAL; > + > + org_attach_prog_fd = prog->attach_prog_fd; > + prog->attach_prog_fd = attach_prog_fd; > + > + btf_id = libbpf_find_attach_btf_id(prog, > + attach_func_name); > + > + if (btf_id < 0) { > + prog->attach_prog_fd = org_attach_prog_fd; I don't think there is a need to restore original attach_prog_fd (most probably it's going to be invalid either way). If explicit set_attach_target fails, user application will have to fail or do some other set_attach_target call. > + return btf_id; > + } > + > + prog->attach_btf_id = btf_id; > + return 0; > +} > + > #define BPF_PROG_SEC_IMPL(string, ptype, eatype, is_attachable, btf, atype) \ > { string, sizeof(string) - 1, ptype, eatype, is_attachable, btf, atype } > > @@ -6633,13 +6659,16 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd) > return err; > } > > -static int libbpf_find_attach_btf_id(struct bpf_program *prog) > +static int libbpf_find_attach_btf_id(struct bpf_program *prog, > + const char *name) > { > enum bpf_attach_type attach_type = prog->expected_attach_type; > __u32 attach_prog_fd = prog->attach_prog_fd; > - const char *name = prog->section_name; > int i, err; > > + if (!name) > + name = prog->section_name; > + I second Toke, name should be just a function name, not including "fentry/" (and others) part. If user want to programmatically set/override attach type, we already have bpf_program__set_expected_attach_type() API for that. So this function's logic should do prefix/name extraction from prog->section_name only if name is not explicitly specified. > if (!name) > return -EINVAL; > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > index 3fe12c9d1f92..02fc58a21a7f 100644 > --- a/tools/lib/bpf/libbpf.h > +++ b/tools/lib/bpf/libbpf.h > @@ -334,6 +334,10 @@ LIBBPF_API void > bpf_program__set_expected_attach_type(struct bpf_program *prog, > enum bpf_attach_type type); > > +LIBBPF_API int > +bpf_program__set_attach_target(struct bpf_program *prog, int attach_prog_fd, > + const char *attach_func_name); > + > LIBBPF_API bool bpf_program__is_socket_filter(const struct bpf_program *prog); > LIBBPF_API bool bpf_program__is_tracepoint(const struct bpf_program *prog); > LIBBPF_API bool bpf_program__is_raw_tracepoint(const struct bpf_program *prog); > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map > index b035122142bb..8aba5438a3f0 100644 > --- a/tools/lib/bpf/libbpf.map > +++ b/tools/lib/bpf/libbpf.map > @@ -230,6 +230,7 @@ LIBBPF_0.0.7 { > bpf_program__name; > bpf_program__is_extension; > bpf_program__is_struct_ops; > + bpf_program__set_attach_target; > bpf_program__set_extension; > bpf_program__set_struct_ops; > btf__align_of; >
On Wed, Feb 12, 2020 at 5:05 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Eelco Chaudron <echaudro@redhat.com> writes: > > > Currently when you want to attach a trace program to a bpf program > > the section name needs to match the tracepoint/function semantics. > > > > However the addition of the bpf_program__set_attach_target() API > > allows you to specify the tracepoint/function dynamically. > > > > The call flow would look something like this: > > > > xdp_fd = bpf_prog_get_fd_by_id(id); > > trace_obj = bpf_object__open_file("func.o", NULL); > > prog = bpf_object__find_program_by_title(trace_obj, > > "fentry/myfunc"); > > bpf_program__set_attach_target(prog, xdp_fd, > > "fentry/xdpfilt_blk_all"); > > I think it would be better to have the attach type as a separate arg > instead of encoding it in the function name. I.e., rather: > > bpf_program__set_attach_target(prog, xdp_fd, > "xdpfilt_blk_all", BPF_TRACE_FENTRY); I agree about not specifying section name prefix (e.g., fentry/). But disagree that expected attach type (BPF_TRACE_FENTRY) should be part of this API. We already have bpf_program__set_expected_attach_type() API, no need to duplicate it here. > > -Toke >
> On Feb 12, 2020, at 9:34 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Wed, Feb 12, 2020 at 4:32 AM Eelco Chaudron <echaudro@redhat.com> wrote: >> >> Currently when you want to attach a trace program to a bpf program >> the section name needs to match the tracepoint/function semantics. >> >> However the addition of the bpf_program__set_attach_target() API >> allows you to specify the tracepoint/function dynamically. >> >> The call flow would look something like this: >> >> xdp_fd = bpf_prog_get_fd_by_id(id); >> trace_obj = bpf_object__open_file("func.o", NULL); >> prog = bpf_object__find_program_by_title(trace_obj, >> "fentry/myfunc"); >> bpf_program__set_attach_target(prog, xdp_fd, >> "fentry/xdpfilt_blk_all"); >> bpf_object__load(trace_obj) >> >> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> I am trying to solve the same problem with slightly different approach. It works as the following (with skeleton): obj = myobject_bpf__open_opts(&opts); bpf_object__for_each_program(prog, obj->obj) bpf_program__overwrite_section_name(prog, new_names[id++]); err = myobject_bpf__load(obj); I don't have very strong preference. But I think my approach is simpler? Thanks, Song diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 514b1a524abb..4c29a7181d57 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -238,6 +238,8 @@ struct bpf_program { __u32 line_info_rec_size; __u32 line_info_cnt; __u32 prog_flags; + + char *overwritten_section_name; }; struct bpf_struct_ops { @@ -442,6 +444,7 @@ static void bpf_program__exit(struct bpf_program *prog) zfree(&prog->pin_name); zfree(&prog->insns); zfree(&prog->reloc_desc); + zfree(&prog->overwritten_section_name); prog->nr_reloc = 0; prog->insns_cnt = 0; @@ -6637,7 +6640,7 @@ static int libbpf_find_attach_btf_id(struct bpf_program *prog) { enum bpf_attach_type attach_type = prog->expected_attach_type; __u32 attach_prog_fd = prog->attach_prog_fd; - const char *name = prog->section_name; + const char *name = prog->overwritten_section_name ? : prog->section_name; int i, err; if (!name) @@ -8396,3 +8399,11 @@ void bpf_object__destroy_skeleton(struct bpf_object_skeleton *s) free(s->progs); free(s); } + +char *bpf_program__overwrite_section_name(struct bpf_program *prog, + const char *sec_name) +{ + prog->overwritten_section_name = strdup(sec_name); + + return prog->overwritten_section_name; +} diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index 3fe12c9d1f92..02f0d8b57cc4 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -595,6 +595,10 @@ bpf_program__bpil_addr_to_offs(struct bpf_prog_info_linear *info_linear); LIBBPF_API void bpf_program__bpil_offs_to_addr(struct bpf_prog_info_linear *info_linear); +LIBBPF_API char * +bpf_program__overwrite_section_name(struct bpf_program *prog, + const char *sec_name); + /* * A helper function to get the number of possible CPUs before looking up * per-CPU maps. Negative errno is returned on failure. diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map index b035122142bb..ed26c20729db 100644 --- a/tools/lib/bpf/libbpf.map +++ b/tools/lib/bpf/libbpf.map @@ -235,3 +235,8 @@ LIBBPF_0.0.7 { btf__align_of; libbpf_find_kernel_btf; } LIBBPF_0.0.6; + +LIBBPF_0.0.8 { + global: + bpf_program__overwrite_section_name; +} LIBBPF_0.0.7;
On Wed, Feb 12, 2020 at 10:07 AM Song Liu <songliubraving@fb.com> wrote: > > > > > On Feb 12, 2020, at 9:34 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > On Wed, Feb 12, 2020 at 4:32 AM Eelco Chaudron <echaudro@redhat.com> wrote: > >> > >> Currently when you want to attach a trace program to a bpf program > >> the section name needs to match the tracepoint/function semantics. > >> > >> However the addition of the bpf_program__set_attach_target() API > >> allows you to specify the tracepoint/function dynamically. > >> > >> The call flow would look something like this: > >> > >> xdp_fd = bpf_prog_get_fd_by_id(id); > >> trace_obj = bpf_object__open_file("func.o", NULL); > >> prog = bpf_object__find_program_by_title(trace_obj, > >> "fentry/myfunc"); > >> bpf_program__set_attach_target(prog, xdp_fd, > >> "fentry/xdpfilt_blk_all"); > >> bpf_object__load(trace_obj) > >> > >> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > > > I am trying to solve the same problem with slightly different approach. > > It works as the following (with skeleton): > > obj = myobject_bpf__open_opts(&opts); > bpf_object__for_each_program(prog, obj->obj) > bpf_program__overwrite_section_name(prog, new_names[id++]); > err = myobject_bpf__load(obj); > > I don't have very strong preference. But I think my approach is simpler? I prefer bpf_program__set_attach_target() approach. Section name is a program identifier and a *hint* for libbpf to determine program type, attach type, and whatever else makes sense. But there still should be an API to set all that manually at runtime, thus bpf_program__set_attach_target(). Doing same by overriding section name feels like a hack, plus it doesn't handle overriding attach_program_fd at all. > > Thanks, > Song > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 514b1a524abb..4c29a7181d57 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -238,6 +238,8 @@ struct bpf_program { > __u32 line_info_rec_size; > __u32 line_info_cnt; > __u32 prog_flags; > + > + char *overwritten_section_name; > }; > > struct bpf_struct_ops { > @@ -442,6 +444,7 @@ static void bpf_program__exit(struct bpf_program *prog) > zfree(&prog->pin_name); > zfree(&prog->insns); > zfree(&prog->reloc_desc); > + zfree(&prog->overwritten_section_name); > > prog->nr_reloc = 0; > prog->insns_cnt = 0; > @@ -6637,7 +6640,7 @@ static int libbpf_find_attach_btf_id(struct bpf_program *prog) > { > enum bpf_attach_type attach_type = prog->expected_attach_type; > __u32 attach_prog_fd = prog->attach_prog_fd; > - const char *name = prog->section_name; > + const char *name = prog->overwritten_section_name ? : prog->section_name; > int i, err; > > if (!name) > @@ -8396,3 +8399,11 @@ void bpf_object__destroy_skeleton(struct bpf_object_skeleton *s) > free(s->progs); > free(s); > } > + > +char *bpf_program__overwrite_section_name(struct bpf_program *prog, > + const char *sec_name) > +{ > + prog->overwritten_section_name = strdup(sec_name); > + > + return prog->overwritten_section_name; > +} > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > index 3fe12c9d1f92..02f0d8b57cc4 100644 > --- a/tools/lib/bpf/libbpf.h > +++ b/tools/lib/bpf/libbpf.h > @@ -595,6 +595,10 @@ bpf_program__bpil_addr_to_offs(struct bpf_prog_info_linear *info_linear); > LIBBPF_API void > bpf_program__bpil_offs_to_addr(struct bpf_prog_info_linear *info_linear); > > +LIBBPF_API char * > +bpf_program__overwrite_section_name(struct bpf_program *prog, > + const char *sec_name); > + > /* > * A helper function to get the number of possible CPUs before looking up > * per-CPU maps. Negative errno is returned on failure. > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map > index b035122142bb..ed26c20729db 100644 > --- a/tools/lib/bpf/libbpf.map > +++ b/tools/lib/bpf/libbpf.map > @@ -235,3 +235,8 @@ LIBBPF_0.0.7 { > btf__align_of; > libbpf_find_kernel_btf; > } LIBBPF_0.0.6; > + > +LIBBPF_0.0.8 { > + global: > + bpf_program__overwrite_section_name; > +} LIBBPF_0.0.7;
> On Feb 12, 2020, at 10:14 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Wed, Feb 12, 2020 at 10:07 AM Song Liu <songliubraving@fb.com> wrote: >> >> >> >>> On Feb 12, 2020, at 9:34 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: >>> >>> On Wed, Feb 12, 2020 at 4:32 AM Eelco Chaudron <echaudro@redhat.com> wrote: >>>> >>>> Currently when you want to attach a trace program to a bpf program >>>> the section name needs to match the tracepoint/function semantics. >>>> >>>> However the addition of the bpf_program__set_attach_target() API >>>> allows you to specify the tracepoint/function dynamically. >>>> >>>> The call flow would look something like this: >>>> >>>> xdp_fd = bpf_prog_get_fd_by_id(id); >>>> trace_obj = bpf_object__open_file("func.o", NULL); >>>> prog = bpf_object__find_program_by_title(trace_obj, >>>> "fentry/myfunc"); >>>> bpf_program__set_attach_target(prog, xdp_fd, >>>> "fentry/xdpfilt_blk_all"); >>>> bpf_object__load(trace_obj) >>>> >>>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >> >> >> I am trying to solve the same problem with slightly different approach. >> >> It works as the following (with skeleton): >> >> obj = myobject_bpf__open_opts(&opts); >> bpf_object__for_each_program(prog, obj->obj) >> bpf_program__overwrite_section_name(prog, new_names[id++]); >> err = myobject_bpf__load(obj); >> >> I don't have very strong preference. But I think my approach is simpler? > > I prefer bpf_program__set_attach_target() approach. Section name is a > program identifier and a *hint* for libbpf to determine program type, > attach type, and whatever else makes sense. But there still should be > an API to set all that manually at runtime, thus > bpf_program__set_attach_target(). Doing same by overriding section > name feels like a hack, plus it doesn't handle overriding > attach_program_fd at all. We already have bpf_object_open_opts to handle different attach_program_fd. Can we depreciate bpf_object_open_opts.attach_prog_fd with the bpf_program__set_attach_target() approach? Thanks, Song
On Wed, Feb 12, 2020 at 10:29 AM Song Liu <songliubraving@fb.com> wrote: > > > > > On Feb 12, 2020, at 10:14 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > On Wed, Feb 12, 2020 at 10:07 AM Song Liu <songliubraving@fb.com> wrote: > >> > >> > >> > >>> On Feb 12, 2020, at 9:34 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > >>> > >>> On Wed, Feb 12, 2020 at 4:32 AM Eelco Chaudron <echaudro@redhat.com> wrote: > >>>> > >>>> Currently when you want to attach a trace program to a bpf program > >>>> the section name needs to match the tracepoint/function semantics. > >>>> > >>>> However the addition of the bpf_program__set_attach_target() API > >>>> allows you to specify the tracepoint/function dynamically. > >>>> > >>>> The call flow would look something like this: > >>>> > >>>> xdp_fd = bpf_prog_get_fd_by_id(id); > >>>> trace_obj = bpf_object__open_file("func.o", NULL); > >>>> prog = bpf_object__find_program_by_title(trace_obj, > >>>> "fentry/myfunc"); > >>>> bpf_program__set_attach_target(prog, xdp_fd, > >>>> "fentry/xdpfilt_blk_all"); > >>>> bpf_object__load(trace_obj) > >>>> > >>>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > >> > >> > >> I am trying to solve the same problem with slightly different approach. > >> > >> It works as the following (with skeleton): > >> > >> obj = myobject_bpf__open_opts(&opts); > >> bpf_object__for_each_program(prog, obj->obj) > >> bpf_program__overwrite_section_name(prog, new_names[id++]); > >> err = myobject_bpf__load(obj); > >> > >> I don't have very strong preference. But I think my approach is simpler? > > > > I prefer bpf_program__set_attach_target() approach. Section name is a > > program identifier and a *hint* for libbpf to determine program type, > > attach type, and whatever else makes sense. But there still should be > > an API to set all that manually at runtime, thus > > bpf_program__set_attach_target(). Doing same by overriding section > > name feels like a hack, plus it doesn't handle overriding > > attach_program_fd at all. > > We already have bpf_object_open_opts to handle different attach_program_fd. Not really, because open_opts apply to bpf_object and all its bpf_programs, not to individual bpf_program. So it works only if BPF application has only one BPF program. If you have many, you can only set the same attach_program_fd for all of them. Basically, open_opts' attach_prog_fd should be treated as a default or fallback attach_prog_fd. > Can we depreciate bpf_object_open_opts.attach_prog_fd with the > bpf_program__set_attach_target() approach? bpf_program__set_attach_target() overrides attach_prog_fd, yes. But we can't just deprecate that option because it's part of an API already, even though adding it to open opts was probably a mistake. But for simple BPF apps with single BPF program it does work fine, so... > > Thanks, > Song >
> On Feb 12, 2020, at 10:34 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Wed, Feb 12, 2020 at 10:29 AM Song Liu <songliubraving@fb.com> wrote: >> >> >> >>> On Feb 12, 2020, at 10:14 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: >>> >>> On Wed, Feb 12, 2020 at 10:07 AM Song Liu <songliubraving@fb.com> wrote: >>>> >>>> >>>> >>>>> On Feb 12, 2020, at 9:34 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: >>>>> >>>>> On Wed, Feb 12, 2020 at 4:32 AM Eelco Chaudron <echaudro@redhat.com> wrote: >>>>>> >>>>>> Currently when you want to attach a trace program to a bpf program >>>>>> the section name needs to match the tracepoint/function semantics. >>>>>> >>>>>> However the addition of the bpf_program__set_attach_target() API >>>>>> allows you to specify the tracepoint/function dynamically. >>>>>> >>>>>> The call flow would look something like this: >>>>>> >>>>>> xdp_fd = bpf_prog_get_fd_by_id(id); >>>>>> trace_obj = bpf_object__open_file("func.o", NULL); >>>>>> prog = bpf_object__find_program_by_title(trace_obj, >>>>>> "fentry/myfunc"); >>>>>> bpf_program__set_attach_target(prog, xdp_fd, >>>>>> "fentry/xdpfilt_blk_all"); >>>>>> bpf_object__load(trace_obj) >>>>>> >>>>>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >>>> >>>> >>>> I am trying to solve the same problem with slightly different approach. >>>> >>>> It works as the following (with skeleton): >>>> >>>> obj = myobject_bpf__open_opts(&opts); >>>> bpf_object__for_each_program(prog, obj->obj) >>>> bpf_program__overwrite_section_name(prog, new_names[id++]); >>>> err = myobject_bpf__load(obj); >>>> >>>> I don't have very strong preference. But I think my approach is simpler? >>> >>> I prefer bpf_program__set_attach_target() approach. Section name is a >>> program identifier and a *hint* for libbpf to determine program type, >>> attach type, and whatever else makes sense. But there still should be >>> an API to set all that manually at runtime, thus >>> bpf_program__set_attach_target(). Doing same by overriding section >>> name feels like a hack, plus it doesn't handle overriding >>> attach_program_fd at all. >> >> We already have bpf_object_open_opts to handle different attach_program_fd. > > Not really, because open_opts apply to bpf_object and all its > bpf_programs, not to individual bpf_program. So it works only if BPF > application has only one BPF program. If you have many, you can only > set the same attach_program_fd for all of them. Basically, open_opts' > attach_prog_fd should be treated as a default or fallback > attach_prog_fd. Fair enough. I will use set_attach_target in my code. > >> Can we depreciate bpf_object_open_opts.attach_prog_fd with the >> bpf_program__set_attach_target() approach? > > bpf_program__set_attach_target() overrides attach_prog_fd, yes. But we > can't just deprecate that option because it's part of an API already, > even though adding it to open opts was probably a mistake. But for > simple BPF apps with single BPF program it does work fine, so... Maybe add a warning saying "attach_prog_fd is deprecated, xxx"? Thanks, Song
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Wed, Feb 12, 2020 at 5:05 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> Eelco Chaudron <echaudro@redhat.com> writes: >> >> > Currently when you want to attach a trace program to a bpf program >> > the section name needs to match the tracepoint/function semantics. >> > >> > However the addition of the bpf_program__set_attach_target() API >> > allows you to specify the tracepoint/function dynamically. >> > >> > The call flow would look something like this: >> > >> > xdp_fd = bpf_prog_get_fd_by_id(id); >> > trace_obj = bpf_object__open_file("func.o", NULL); >> > prog = bpf_object__find_program_by_title(trace_obj, >> > "fentry/myfunc"); >> > bpf_program__set_attach_target(prog, xdp_fd, >> > "fentry/xdpfilt_blk_all"); >> >> I think it would be better to have the attach type as a separate arg >> instead of encoding it in the function name. I.e., rather: >> >> bpf_program__set_attach_target(prog, xdp_fd, >> "xdpfilt_blk_all", BPF_TRACE_FENTRY); > > I agree about not specifying section name prefix (e.g., fentry/). But > disagree that expected attach type (BPF_TRACE_FENTRY) should be part > of this API. We already have bpf_program__set_expected_attach_type() > API, no need to duplicate it here. Ah yes, forgot about that; just keeping that and making this function name only is fine with me :) -Toke
On 12 Feb 2020, at 22:52, Toke Høiland-Jørgensen wrote: > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > >> On Wed, Feb 12, 2020 at 5:05 AM Toke Høiland-Jørgensen >> <toke@redhat.com> wrote: >>> >>> Eelco Chaudron <echaudro@redhat.com> writes: >>> >>>> Currently when you want to attach a trace program to a bpf program >>>> the section name needs to match the tracepoint/function semantics. >>>> >>>> However the addition of the bpf_program__set_attach_target() API >>>> allows you to specify the tracepoint/function dynamically. >>>> >>>> The call flow would look something like this: >>>> >>>> xdp_fd = bpf_prog_get_fd_by_id(id); >>>> trace_obj = bpf_object__open_file("func.o", NULL); >>>> prog = bpf_object__find_program_by_title(trace_obj, >>>> "fentry/myfunc"); >>>> bpf_program__set_attach_target(prog, xdp_fd, >>>> "fentry/xdpfilt_blk_all"); >>> >>> I think it would be better to have the attach type as a separate arg >>> instead of encoding it in the function name. I.e., rather: >>> >>> bpf_program__set_attach_target(prog, xdp_fd, >>> "xdpfilt_blk_all", >>> BPF_TRACE_FENTRY); >> >> I agree about not specifying section name prefix (e.g., fentry/). But >> disagree that expected attach type (BPF_TRACE_FENTRY) should be part >> of this API. We already have bpf_program__set_expected_attach_type() >> API, no need to duplicate it here. > > Ah yes, forgot about that; just keeping that and making this function > name only is fine with me :) Toke/Andrii, Thanks for the feedback, will send out a v2 soon. //Eelco
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 514b1a524abb..2ce879c301bb 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -4933,15 +4933,16 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt, return ret; } -static int libbpf_find_attach_btf_id(struct bpf_program *prog); +static int libbpf_find_attach_btf_id(struct bpf_program *prog, + const char *name); int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver) { int err = 0, fd, i, btf_id; - if (prog->type == BPF_PROG_TYPE_TRACING || - prog->type == BPF_PROG_TYPE_EXT) { - btf_id = libbpf_find_attach_btf_id(prog); + if ((prog->type == BPF_PROG_TYPE_TRACING || + prog->type == BPF_PROG_TYPE_EXT) && !prog->attach_btf_id) { + btf_id = libbpf_find_attach_btf_id(prog, NULL); if (btf_id <= 0) return btf_id; prog->attach_btf_id = btf_id; @@ -6202,6 +6203,31 @@ void bpf_program__set_expected_attach_type(struct bpf_program *prog, prog->expected_attach_type = type; } +int bpf_program__set_attach_target(struct bpf_program *prog, + int attach_prog_fd, + const char *attach_func_name) +{ + __u32 org_attach_prog_fd; + int btf_id; + + if (!prog || attach_prog_fd < 0 || !attach_func_name) + return -EINVAL; + + org_attach_prog_fd = prog->attach_prog_fd; + prog->attach_prog_fd = attach_prog_fd; + + btf_id = libbpf_find_attach_btf_id(prog, + attach_func_name); + + if (btf_id < 0) { + prog->attach_prog_fd = org_attach_prog_fd; + return btf_id; + } + + prog->attach_btf_id = btf_id; + return 0; +} + #define BPF_PROG_SEC_IMPL(string, ptype, eatype, is_attachable, btf, atype) \ { string, sizeof(string) - 1, ptype, eatype, is_attachable, btf, atype } @@ -6633,13 +6659,16 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd) return err; } -static int libbpf_find_attach_btf_id(struct bpf_program *prog) +static int libbpf_find_attach_btf_id(struct bpf_program *prog, + const char *name) { enum bpf_attach_type attach_type = prog->expected_attach_type; __u32 attach_prog_fd = prog->attach_prog_fd; - const char *name = prog->section_name; int i, err; + if (!name) + name = prog->section_name; + if (!name) return -EINVAL; diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index 3fe12c9d1f92..02fc58a21a7f 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -334,6 +334,10 @@ LIBBPF_API void bpf_program__set_expected_attach_type(struct bpf_program *prog, enum bpf_attach_type type); +LIBBPF_API int +bpf_program__set_attach_target(struct bpf_program *prog, int attach_prog_fd, + const char *attach_func_name); + LIBBPF_API bool bpf_program__is_socket_filter(const struct bpf_program *prog); LIBBPF_API bool bpf_program__is_tracepoint(const struct bpf_program *prog); LIBBPF_API bool bpf_program__is_raw_tracepoint(const struct bpf_program *prog); diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map index b035122142bb..8aba5438a3f0 100644 --- a/tools/lib/bpf/libbpf.map +++ b/tools/lib/bpf/libbpf.map @@ -230,6 +230,7 @@ LIBBPF_0.0.7 { bpf_program__name; bpf_program__is_extension; bpf_program__is_struct_ops; + bpf_program__set_attach_target; bpf_program__set_extension; bpf_program__set_struct_ops; btf__align_of;
Currently when you want to attach a trace program to a bpf program the section name needs to match the tracepoint/function semantics. However the addition of the bpf_program__set_attach_target() API allows you to specify the tracepoint/function dynamically. The call flow would look something like this: xdp_fd = bpf_prog_get_fd_by_id(id); trace_obj = bpf_object__open_file("func.o", NULL); prog = bpf_object__find_program_by_title(trace_obj, "fentry/myfunc"); bpf_program__set_attach_target(prog, xdp_fd, "fentry/xdpfilt_blk_all"); bpf_object__load(trace_obj) Signed-off-by: Eelco Chaudron <echaudro@redhat.com> --- tools/lib/bpf/libbpf.c | 41 +++++++++++++++++++++++++++++++++++------ tools/lib/bpf/libbpf.h | 4 ++++ tools/lib/bpf/libbpf.map | 1 + 3 files changed, 40 insertions(+), 6 deletions(-)