| Message ID | 20190524103648.15669-3-quentin.monnet@netronome.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | tools: bpftool: add an option for debug output from libbpf and verifier | expand |
On Fri, 24 May 2019 11:36:47 +0100 Quentin Monnet <quentin.monnet@netronome.com> wrote: > libbpf was recently made aware of the log_level attribute for programs, > used to specify the level of information expected to be dumped by the > verifier. Function bpf_prog_load_xattr() got support for this log_level > parameter. > > But some applications using libbpf rely on another function to load > programs, bpf_object__load(), which does accept any parameter for log > level. Create an API function based on bpf_object__load(), but accepting > an "attr" object as a parameter. Then add a log_level field to that > object, so that applications calling the new bpf_object__load_xattr() > can pick the desired log level. Does this allow us to extend struct bpf_object_load_attr later? > v3: > - Rewrite commit log. > > v2: > - We are in a new cycle, bump libbpf extraversion number. > > Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com> > Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com> > --- > tools/lib/bpf/Makefile | 2 +- > tools/lib/bpf/libbpf.c | 20 +++++++++++++++++--- > tools/lib/bpf/libbpf.h | 6 ++++++ > tools/lib/bpf/libbpf.map | 5 +++++ > 4 files changed, 29 insertions(+), 4 deletions(-) > > diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile > index a2aceadf68db..9312066a1ae3 100644 > --- a/tools/lib/bpf/Makefile > +++ b/tools/lib/bpf/Makefile > @@ -3,7 +3,7 @@ > > BPF_VERSION = 0 > BPF_PATCHLEVEL = 0 > -BPF_EXTRAVERSION = 3 > +BPF_EXTRAVERSION = 4 > > MAKEFLAGS += --no-print-directory > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 197b574406b3..1c6fb7a3201e 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -2222,7 +2222,7 @@ static bool bpf_program__is_function_storage(struct bpf_program *prog, > } > > static int > -bpf_object__load_progs(struct bpf_object *obj) > +bpf_object__load_progs(struct bpf_object *obj, int log_level) > { > size_t i; > int err; > @@ -2230,6 +2230,7 @@ bpf_object__load_progs(struct bpf_object *obj) > for (i = 0; i < obj->nr_programs; i++) { > if (bpf_program__is_function_storage(&obj->programs[i], obj)) > continue; > + obj->programs[i].log_level = log_level; > err = bpf_program__load(&obj->programs[i], > obj->license, > obj->kern_version); > @@ -2381,10 +2382,14 @@ int bpf_object__unload(struct bpf_object *obj) > return 0; > } > > -int bpf_object__load(struct bpf_object *obj) > +int bpf_object__load_xattr(struct bpf_object_load_attr *attr) > { > + struct bpf_object *obj; > int err; > > + if (!attr) > + return -EINVAL; > + obj = attr->obj; > if (!obj) > return -EINVAL; > > @@ -2397,7 +2402,7 @@ int bpf_object__load(struct bpf_object *obj) > > CHECK_ERR(bpf_object__create_maps(obj), err, out); > CHECK_ERR(bpf_object__relocate(obj), err, out); > - CHECK_ERR(bpf_object__load_progs(obj), err, out); > + CHECK_ERR(bpf_object__load_progs(obj, attr->log_level), err, out); > > return 0; > out: > @@ -2406,6 +2411,15 @@ int bpf_object__load(struct bpf_object *obj) > return err; > } > > +int bpf_object__load(struct bpf_object *obj) > +{ > + struct bpf_object_load_attr attr = { > + .obj = obj, > + }; > + > + return bpf_object__load_xattr(&attr); > +} > + > static int check_path(const char *path) > { > char *cp, errmsg[STRERR_BUFSIZE]; > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > index c5ff00515ce7..e1c748db44f6 100644 > --- a/tools/lib/bpf/libbpf.h > +++ b/tools/lib/bpf/libbpf.h > @@ -89,8 +89,14 @@ LIBBPF_API int bpf_object__unpin_programs(struct bpf_object *obj, > LIBBPF_API int bpf_object__pin(struct bpf_object *object, const char *path); > LIBBPF_API void bpf_object__close(struct bpf_object *object); > > +struct bpf_object_load_attr { > + struct bpf_object *obj; > + int log_level; > +}; Can this be extended later? > /* Load/unload object into/from kernel */ > LIBBPF_API int bpf_object__load(struct bpf_object *obj); > +LIBBPF_API int bpf_object__load_xattr(struct bpf_object_load_attr *attr); > LIBBPF_API int bpf_object__unload(struct bpf_object *obj); > LIBBPF_API const char *bpf_object__name(struct bpf_object *obj); > LIBBPF_API unsigned int bpf_object__kversion(struct bpf_object *obj); > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map > index 673001787cba..6ce61fa0baf3 100644 > --- a/tools/lib/bpf/libbpf.map > +++ b/tools/lib/bpf/libbpf.map > @@ -164,3 +164,8 @@ LIBBPF_0.0.3 { > bpf_map_freeze; > btf__finalize_data; > } LIBBPF_0.0.2; > + > +LIBBPF_0.0.4 { > + global: > + bpf_object__load_xattr; > +} LIBBPF_0.0.3;
2019-05-24 13:22 UTC+0200 ~ Jesper Dangaard Brouer <brouer@redhat.com> > On Fri, 24 May 2019 11:36:47 +0100 > Quentin Monnet <quentin.monnet@netronome.com> wrote: > >> libbpf was recently made aware of the log_level attribute for programs, >> used to specify the level of information expected to be dumped by the >> verifier. Function bpf_prog_load_xattr() got support for this log_level >> parameter. >> >> But some applications using libbpf rely on another function to load >> programs, bpf_object__load(), which does accept any parameter for log >> level. Create an API function based on bpf_object__load(), but accepting >> an "attr" object as a parameter. Then add a log_level field to that >> object, so that applications calling the new bpf_object__load_xattr() >> can pick the desired log level. > > Does this allow us to extend struct bpf_object_load_attr later? I see no reason why it could not. Having the _xattr() version of the function is precisely a way to have something extensible in the future, without having to create additional API functions each time we want to pass a new parameter. And e.g. struct bpf_prog_load_attr (used with bpf_prog_load_xattr()) has already been extended in the past. So, yeah, we can add to it in the future. Do you have something in mind? Quentin
On Fri, 24 May 2019 12:51:14 +0100 Quentin Monnet <quentin.monnet@netronome.com> wrote: > 2019-05-24 13:22 UTC+0200 ~ Jesper Dangaard Brouer <brouer@redhat.com> > > On Fri, 24 May 2019 11:36:47 +0100 > > Quentin Monnet <quentin.monnet@netronome.com> wrote: > > > >> libbpf was recently made aware of the log_level attribute for programs, > >> used to specify the level of information expected to be dumped by the > >> verifier. Function bpf_prog_load_xattr() got support for this log_level > >> parameter. > >> > >> But some applications using libbpf rely on another function to load > >> programs, bpf_object__load(), which does accept any parameter for log > >> level. Create an API function based on bpf_object__load(), but accepting > >> an "attr" object as a parameter. Then add a log_level field to that > >> object, so that applications calling the new bpf_object__load_xattr() > >> can pick the desired log level. > > > > Does this allow us to extend struct bpf_object_load_attr later? > > I see no reason why it could not. Having the _xattr() version of the > function is precisely a way to have something extensible in the future, > without having to create additional API functions each time we want to > pass a new parameter. And e.g. struct bpf_prog_load_attr (used with > bpf_prog_load_xattr()) has already been extended in the past. So, yeah, > we can add to it in the future. Great. I just don't know/understand how user-space handle this. If a binary is compiled with libbpf as dynamic loadable lib, then it e.g. saw libbpf.so.2 when it was compiled, then can't it choose to use libbpf.so.3 then? (e.g. when libbpf.so.2 is not on the system). (I would actually like to learn/understand this, so links are welcome). > Do you have something in mind? I was playing with extending bpf_prog_load_attr, but instead I created a bpf_prog_load_attr_maps instead and a new function bpf_prog_load_xattr_maps(), e.g. see: https://github.com/xdp-project/xdp-tutorial/blob/master/common/common_libbpf.h https://github.com/xdp-project/xdp-tutorial/blob/master/common/common_libbpf.c I guess, I could just extend bpf_prog_load_attr instead, right?
2019-05-24 14:49 UTC+0200 ~ Jesper Dangaard Brouer <brouer@redhat.com> > On Fri, 24 May 2019 12:51:14 +0100 > Quentin Monnet <quentin.monnet@netronome.com> wrote: > >> 2019-05-24 13:22 UTC+0200 ~ Jesper Dangaard Brouer <brouer@redhat.com> >>> On Fri, 24 May 2019 11:36:47 +0100 >>> Quentin Monnet <quentin.monnet@netronome.com> wrote: >>> >>>> libbpf was recently made aware of the log_level attribute for programs, >>>> used to specify the level of information expected to be dumped by the >>>> verifier. Function bpf_prog_load_xattr() got support for this log_level >>>> parameter. >>>> >>>> But some applications using libbpf rely on another function to load >>>> programs, bpf_object__load(), which does accept any parameter for log >>>> level. Create an API function based on bpf_object__load(), but accepting >>>> an "attr" object as a parameter. Then add a log_level field to that >>>> object, so that applications calling the new bpf_object__load_xattr() >>>> can pick the desired log level. >>> >>> Does this allow us to extend struct bpf_object_load_attr later? >> >> I see no reason why it could not. Having the _xattr() version of the >> function is precisely a way to have something extensible in the future, >> without having to create additional API functions each time we want to >> pass a new parameter. And e.g. struct bpf_prog_load_attr (used with >> bpf_prog_load_xattr()) has already been extended in the past. So, yeah, >> we can add to it in the future. > > Great. I just don't know/understand how user-space handle this. If a > binary is compiled with libbpf as dynamic loadable lib, then it e.g. saw > libbpf.so.2 when it was compiled, then can't it choose to use libbpf.so.3 > then? (e.g. when libbpf.so.2 is not on the system). (I would actually > like to learn/understand this, so links are welcome). Well I'm no library expert, so don't take my word for it. As far as I understand, the soname of the library is selected at link time. So if your app is linked again libbpf.so.2, you will need version 2.* of the library to be installed on your system, because increasing the version number usually implies ABI breakage. You can usually check which version of the libraries is needed with ldd ("ldd bpftool", except that you won't see libbpf because it's statically linked for bpftool). This being said, for now the version number for libbpf has not been incremented and is still at 0, we only had the extraversion increasing. Since it's not part of the soname ("-Wl,-soname,libbpf.so.$(VERSION)" in libbpf Makefile), it is not taken into account when searching for the lib on the system. What I mean is that if the program is linked against libbpf.so.0, it could pick libbpf.so.0.0.2 or libbpf.so.0.0.3 indifferently depending on what it finds on the system (I assume it takes the newest?). There should not be any ABI breakage between the two, so programs compiled against an older patchlevel or extraversion of the library should still be able to use a newer one. There is some documentation on libraries here (I should take some time to finish reading it myself!): http://tldp.org/HOWTO/Program-Library-HOWTO/ There are also interesting elements in the documentation that was cited when Andrey introduced the LIBPPF_API macros in libbpf: https://www.akkadia.org/drepper/dsohowto.pdf > >> Do you have something in mind? > > I was playing with extending bpf_prog_load_attr, but instead I created a > bpf_prog_load_attr_maps instead and a new function > bpf_prog_load_xattr_maps(), e.g. see: > > https://github.com/xdp-project/xdp-tutorial/blob/master/common/common_libbpf.h > https://github.com/xdp-project/xdp-tutorial/blob/master/common/common_libbpf.c > > I guess, I could just extend bpf_prog_load_attr instead, right? > I believe so. Best, Quentin
On Fri, May 24, 2019 at 3:36 AM Quentin Monnet <quentin.monnet@netronome.com> wrote: > > libbpf was recently made aware of the log_level attribute for programs, > used to specify the level of information expected to be dumped by the > verifier. Function bpf_prog_load_xattr() got support for this log_level > parameter. > > But some applications using libbpf rely on another function to load > programs, bpf_object__load(), which does accept any parameter for log > level. Create an API function based on bpf_object__load(), but accepting > an "attr" object as a parameter. Then add a log_level field to that > object, so that applications calling the new bpf_object__load_xattr() > can pick the desired log level. > > v3: > - Rewrite commit log. > > v2: > - We are in a new cycle, bump libbpf extraversion number. > > Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com> > Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com> > --- > tools/lib/bpf/Makefile | 2 +- > tools/lib/bpf/libbpf.c | 20 +++++++++++++++++--- > tools/lib/bpf/libbpf.h | 6 ++++++ > tools/lib/bpf/libbpf.map | 5 +++++ > 4 files changed, 29 insertions(+), 4 deletions(-) This commit broke ./test_progs -s prog_tests/bpf_verif_scale.c no longer passes log_level. Could you please take a look?
2019-05-28 17:35 UTC-0700 ~ Alexei Starovoitov <alexei.starovoitov@gmail.com> > On Fri, May 24, 2019 at 3:36 AM Quentin Monnet > <quentin.monnet@netronome.com> wrote: >> >> libbpf was recently made aware of the log_level attribute for programs, >> used to specify the level of information expected to be dumped by the >> verifier. Function bpf_prog_load_xattr() got support for this log_level >> parameter. >> >> But some applications using libbpf rely on another function to load >> programs, bpf_object__load(), which does accept any parameter for log >> level. Create an API function based on bpf_object__load(), but accepting >> an "attr" object as a parameter. Then add a log_level field to that >> object, so that applications calling the new bpf_object__load_xattr() >> can pick the desired log level. >> >> v3: >> - Rewrite commit log. >> >> v2: >> - We are in a new cycle, bump libbpf extraversion number. >> >> Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com> >> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com> >> --- >> tools/lib/bpf/Makefile | 2 +- >> tools/lib/bpf/libbpf.c | 20 +++++++++++++++++--- >> tools/lib/bpf/libbpf.h | 6 ++++++ >> tools/lib/bpf/libbpf.map | 5 +++++ >> 4 files changed, 29 insertions(+), 4 deletions(-) > > This commit broke ./test_progs -s > prog_tests/bpf_verif_scale.c no longer passes log_level. > Could you please take a look? > Indeed, I forgot that bpf_load_prog_xattr() would eventually call bpf_object__load_progs() as well, where the log_level is now overwritten. Fix incoming, sorry about that. Quentin
diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile index a2aceadf68db..9312066a1ae3 100644 --- a/tools/lib/bpf/Makefile +++ b/tools/lib/bpf/Makefile @@ -3,7 +3,7 @@ BPF_VERSION = 0 BPF_PATCHLEVEL = 0 -BPF_EXTRAVERSION = 3 +BPF_EXTRAVERSION = 4 MAKEFLAGS += --no-print-directory diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 197b574406b3..1c6fb7a3201e 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -2222,7 +2222,7 @@ static bool bpf_program__is_function_storage(struct bpf_program *prog, } static int -bpf_object__load_progs(struct bpf_object *obj) +bpf_object__load_progs(struct bpf_object *obj, int log_level) { size_t i; int err; @@ -2230,6 +2230,7 @@ bpf_object__load_progs(struct bpf_object *obj) for (i = 0; i < obj->nr_programs; i++) { if (bpf_program__is_function_storage(&obj->programs[i], obj)) continue; + obj->programs[i].log_level = log_level; err = bpf_program__load(&obj->programs[i], obj->license, obj->kern_version); @@ -2381,10 +2382,14 @@ int bpf_object__unload(struct bpf_object *obj) return 0; } -int bpf_object__load(struct bpf_object *obj) +int bpf_object__load_xattr(struct bpf_object_load_attr *attr) { + struct bpf_object *obj; int err; + if (!attr) + return -EINVAL; + obj = attr->obj; if (!obj) return -EINVAL; @@ -2397,7 +2402,7 @@ int bpf_object__load(struct bpf_object *obj) CHECK_ERR(bpf_object__create_maps(obj), err, out); CHECK_ERR(bpf_object__relocate(obj), err, out); - CHECK_ERR(bpf_object__load_progs(obj), err, out); + CHECK_ERR(bpf_object__load_progs(obj, attr->log_level), err, out); return 0; out: @@ -2406,6 +2411,15 @@ int bpf_object__load(struct bpf_object *obj) return err; } +int bpf_object__load(struct bpf_object *obj) +{ + struct bpf_object_load_attr attr = { + .obj = obj, + }; + + return bpf_object__load_xattr(&attr); +} + static int check_path(const char *path) { char *cp, errmsg[STRERR_BUFSIZE]; diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index c5ff00515ce7..e1c748db44f6 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -89,8 +89,14 @@ LIBBPF_API int bpf_object__unpin_programs(struct bpf_object *obj, LIBBPF_API int bpf_object__pin(struct bpf_object *object, const char *path); LIBBPF_API void bpf_object__close(struct bpf_object *object); +struct bpf_object_load_attr { + struct bpf_object *obj; + int log_level; +}; + /* Load/unload object into/from kernel */ LIBBPF_API int bpf_object__load(struct bpf_object *obj); +LIBBPF_API int bpf_object__load_xattr(struct bpf_object_load_attr *attr); LIBBPF_API int bpf_object__unload(struct bpf_object *obj); LIBBPF_API const char *bpf_object__name(struct bpf_object *obj); LIBBPF_API unsigned int bpf_object__kversion(struct bpf_object *obj); diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map index 673001787cba..6ce61fa0baf3 100644 --- a/tools/lib/bpf/libbpf.map +++ b/tools/lib/bpf/libbpf.map @@ -164,3 +164,8 @@ LIBBPF_0.0.3 { bpf_map_freeze; btf__finalize_data; } LIBBPF_0.0.2; + +LIBBPF_0.0.4 { + global: + bpf_object__load_xattr; +} LIBBPF_0.0.3;