| Message ID | 20211101224357.2651181-1-davemarchevsky@fb.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | libbpf: deprecate bpf_program__get_prog_info_linear | expand |
On Mon, Nov 1, 2021 at 3:46 PM Dave Marchevsky <davemarchevsky@fb.com> wrote: > > bpf_program__get_prog_info_linear is a helper which wraps the > bpf_obj_get_info_by_fd BPF syscall with some niceties that put > all dynamic-length bpf_prog_info in one buffer contiguous with struct > bpf_prog_info, and simplify the selection of which dynamic data to grab. > > The resultant combined struct, bpf_prog_info_linear, is persisted to > file by 'perf' to enable later annotation of BPF prog data. libbpf > includes some vaddr <-> offset conversion helpers for > struct bpf_prog_info_linear to simplify this. > > This functionality is heavily tailored to perf's usecase, so its use as > a general prog info API should be deemphasized in favor of just calling > bpf_obj_get_info_by_fd, which can be more easily fit to purpose. Some > examples from caller migrations in this series: > > * Some callers weren't requesting or using dynamic-sized prog info and > are well served by a simple get_info_by_fd call (e.g. > dump_prog_id_as_func_ptr in bpftool) > * Some callers were requesting all of a specific dynamic info type but > only using the first record, so can avoid unnecessary malloc by > only requesting 1 (e.g. profile_target_name in bpftool) > * bpftool's do_dump saves some malloc/free by growing and reusing its > dynamic prog_info buf as it loops over progs to grab info and dump. > > Perf does need the full functionality of > bpf_program__get_prog_info_linear and its accompanying structs + > helpers, so copy the code to its codebase, migrate all other uses in the > tree, and deprecate the helper in libbpf. > > Since the deprecated symbols continue to be included in perf some > renaming was necessary in perf's copy, otherwise functionality is > unchanged. > > This work was previously discussed in libbpf's issue tracker [0]. > > [0]: https://github.com/libbpf/libbpf/issues/313 > > v2->v3: > * Remove v2's patch 1 ("libbpf: Migrate internal use of > bpf_program__get_prog_info_linear"), which was applied [Andrii] > * Add new patch 1 migrating error checking of libbpf calls to > new scheme [Andrii, Quentin] > * In patch 2, fix != -1 error check of libbpf call, improper realloc > handling, and get rid of confusing macros [Andrii] > * In patch 4, deprecate starting from 0.6 instead of 0.7 [Andrii] LGTM. Quentin, can you please take a look and ack as well? Thanks! > > v1->v2: fix bpftool do_dump changes to clear bpf_prog_info after use and > correctly pass realloc'd ptr back (patch 2) > > Dave Marchevsky (4): > bpftool: Migrate -1 err checks of libbpf fn calls > bpftool: use bpf_obj_get_info_by_fd directly > perf: pull in bpf_program__get_prog_info_linear > libbpf: deprecate bpf_program__get_prog_info_linear > > tools/bpf/bpftool/btf_dumper.c | 42 +-- > tools/bpf/bpftool/prog.c | 158 ++++++++--- > tools/bpf/bpftool/struct_ops.c | 2 +- > tools/lib/bpf/libbpf.h | 3 + > .../Documentation/perf.data-file-format.txt | 2 +- > tools/perf/util/Build | 1 + > tools/perf/util/annotate.c | 3 +- > tools/perf/util/bpf-event.c | 41 ++- > tools/perf/util/bpf-event.h | 2 +- > tools/perf/util/bpf-utils.c | 261 ++++++++++++++++++ > tools/perf/util/bpf-utils.h | 76 +++++ > tools/perf/util/bpf_counter.c | 6 +- > tools/perf/util/dso.c | 1 + > tools/perf/util/env.c | 1 + > tools/perf/util/header.c | 13 +- > 15 files changed, 527 insertions(+), 85 deletions(-) > create mode 100644 tools/perf/util/bpf-utils.c > create mode 100644 tools/perf/util/bpf-utils.h > > -- > 2.30.2 >
2021-11-02 16:06 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com> > On Mon, Nov 1, 2021 at 3:46 PM Dave Marchevsky <davemarchevsky@fb.com> wrote: >> >> bpf_program__get_prog_info_linear is a helper which wraps the >> bpf_obj_get_info_by_fd BPF syscall with some niceties that put >> all dynamic-length bpf_prog_info in one buffer contiguous with struct >> bpf_prog_info, and simplify the selection of which dynamic data to grab. >> >> The resultant combined struct, bpf_prog_info_linear, is persisted to >> file by 'perf' to enable later annotation of BPF prog data. libbpf >> includes some vaddr <-> offset conversion helpers for >> struct bpf_prog_info_linear to simplify this. >> >> This functionality is heavily tailored to perf's usecase, so its use as >> a general prog info API should be deemphasized in favor of just calling >> bpf_obj_get_info_by_fd, which can be more easily fit to purpose. Some >> examples from caller migrations in this series: >> >> * Some callers weren't requesting or using dynamic-sized prog info and >> are well served by a simple get_info_by_fd call (e.g. >> dump_prog_id_as_func_ptr in bpftool) >> * Some callers were requesting all of a specific dynamic info type but >> only using the first record, so can avoid unnecessary malloc by >> only requesting 1 (e.g. profile_target_name in bpftool) >> * bpftool's do_dump saves some malloc/free by growing and reusing its >> dynamic prog_info buf as it loops over progs to grab info and dump. >> >> Perf does need the full functionality of >> bpf_program__get_prog_info_linear and its accompanying structs + >> helpers, so copy the code to its codebase, migrate all other uses in the >> tree, and deprecate the helper in libbpf. >> >> Since the deprecated symbols continue to be included in perf some >> renaming was necessary in perf's copy, otherwise functionality is >> unchanged. >> >> This work was previously discussed in libbpf's issue tracker [0]. >> >> [0]: https://github.com/libbpf/libbpf/issues/313 >> >> v2->v3: >> * Remove v2's patch 1 ("libbpf: Migrate internal use of >> bpf_program__get_prog_info_linear"), which was applied [Andrii] >> * Add new patch 1 migrating error checking of libbpf calls to >> new scheme [Andrii, Quentin] >> * In patch 2, fix != -1 error check of libbpf call, improper realloc >> handling, and get rid of confusing macros [Andrii] >> * In patch 4, deprecate starting from 0.6 instead of 0.7 [Andrii] > > LGTM. Quentin, can you please take a look and ack as well? Thanks! Thanks Andrii for the Cc! I realised yesterday morning that I'd been hit by the unsubscription incident and missed v3 of this set. The changes look good to me, and you can add my tag to the first three patches: Acked-by: Quentin Monnet <quentin@isovalent.com> Regarding patch 4, looking at the latest deprecations in libbpf, I would have expected the functions to be deprecated starting in v0.7, and not v0.6? Other than that, on patch 2 (apologies for not answering inline), it would feel more natural, in do_dump()'s "for" loop in prog.c, to have the memset() above the call to bpf_obj_get_info_by_fd() (and to skip the zero-initialisation of "info") instead of at the end of the loop, which means a useless memset() just before we exit the loop. But probably not worth a respin just for that. Thanks, Quentin
On Wed, Nov 3, 2021 at 4:26 AM Quentin Monnet <quentin@isovalent.com> wrote: > > 2021-11-02 16:06 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com> > > On Mon, Nov 1, 2021 at 3:46 PM Dave Marchevsky <davemarchevsky@fb.com> wrote: > >> > >> bpf_program__get_prog_info_linear is a helper which wraps the > >> bpf_obj_get_info_by_fd BPF syscall with some niceties that put > >> all dynamic-length bpf_prog_info in one buffer contiguous with struct > >> bpf_prog_info, and simplify the selection of which dynamic data to grab. > >> > >> The resultant combined struct, bpf_prog_info_linear, is persisted to > >> file by 'perf' to enable later annotation of BPF prog data. libbpf > >> includes some vaddr <-> offset conversion helpers for > >> struct bpf_prog_info_linear to simplify this. > >> > >> This functionality is heavily tailored to perf's usecase, so its use as > >> a general prog info API should be deemphasized in favor of just calling > >> bpf_obj_get_info_by_fd, which can be more easily fit to purpose. Some > >> examples from caller migrations in this series: > >> > >> * Some callers weren't requesting or using dynamic-sized prog info and > >> are well served by a simple get_info_by_fd call (e.g. > >> dump_prog_id_as_func_ptr in bpftool) > >> * Some callers were requesting all of a specific dynamic info type but > >> only using the first record, so can avoid unnecessary malloc by > >> only requesting 1 (e.g. profile_target_name in bpftool) > >> * bpftool's do_dump saves some malloc/free by growing and reusing its > >> dynamic prog_info buf as it loops over progs to grab info and dump. > >> > >> Perf does need the full functionality of > >> bpf_program__get_prog_info_linear and its accompanying structs + > >> helpers, so copy the code to its codebase, migrate all other uses in the > >> tree, and deprecate the helper in libbpf. > >> > >> Since the deprecated symbols continue to be included in perf some > >> renaming was necessary in perf's copy, otherwise functionality is > >> unchanged. > >> > >> This work was previously discussed in libbpf's issue tracker [0]. > >> > >> [0]: https://github.com/libbpf/libbpf/issues/313 > >> > >> v2->v3: > >> * Remove v2's patch 1 ("libbpf: Migrate internal use of > >> bpf_program__get_prog_info_linear"), which was applied [Andrii] > >> * Add new patch 1 migrating error checking of libbpf calls to > >> new scheme [Andrii, Quentin] > >> * In patch 2, fix != -1 error check of libbpf call, improper realloc > >> handling, and get rid of confusing macros [Andrii] > >> * In patch 4, deprecate starting from 0.6 instead of 0.7 [Andrii] > > > > LGTM. Quentin, can you please take a look and ack as well? Thanks! > > Thanks Andrii for the Cc! I realised yesterday morning that I'd been hit > by the unsubscription incident and missed v3 of this set. Yeah, super unfortunate this unsubscription. Had to go through that as well. > > The changes look good to me, and you can add my tag to the first three > patches: > > Acked-by: Quentin Monnet <quentin@isovalent.com> Thanks, I will. > > Regarding patch 4, looking at the latest deprecations in libbpf, I would > have expected the functions to be deprecated starting in v0.7, and not v0.6? The reason to do it in upcoming v0.6 is because there are no replacement APIs that we are going to wait for. No point in delaying the inevitable just for the sake of delaying it. > > Other than that, on patch 2 (apologies for not answering inline), it > would feel more natural, in do_dump()'s "for" loop in prog.c, to have > the memset() above the call to bpf_obj_get_info_by_fd() (and to skip the > zero-initialisation of "info") instead of at the end of the loop, which > means a useless memset() just before we exit the loop. But probably not > worth a respin just for that. Yeah, that bothered me a bit as well, but if you are also mentioning that I'll just move it up as you suggested, while applying. Thanks. > > Thanks, > Quentin
On Wed, Nov 3, 2021 at 11:21 AM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Wed, Nov 3, 2021 at 4:26 AM Quentin Monnet <quentin@isovalent.com> wrote: > > > > 2021-11-02 16:06 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@gmail.com> > > > On Mon, Nov 1, 2021 at 3:46 PM Dave Marchevsky <davemarchevsky@fb.com> wrote: > > >> > > >> bpf_program__get_prog_info_linear is a helper which wraps the > > >> bpf_obj_get_info_by_fd BPF syscall with some niceties that put > > >> all dynamic-length bpf_prog_info in one buffer contiguous with struct > > >> bpf_prog_info, and simplify the selection of which dynamic data to grab. > > >> > > >> The resultant combined struct, bpf_prog_info_linear, is persisted to > > >> file by 'perf' to enable later annotation of BPF prog data. libbpf > > >> includes some vaddr <-> offset conversion helpers for > > >> struct bpf_prog_info_linear to simplify this. > > >> > > >> This functionality is heavily tailored to perf's usecase, so its use as > > >> a general prog info API should be deemphasized in favor of just calling > > >> bpf_obj_get_info_by_fd, which can be more easily fit to purpose. Some > > >> examples from caller migrations in this series: > > >> > > >> * Some callers weren't requesting or using dynamic-sized prog info and > > >> are well served by a simple get_info_by_fd call (e.g. > > >> dump_prog_id_as_func_ptr in bpftool) > > >> * Some callers were requesting all of a specific dynamic info type but > > >> only using the first record, so can avoid unnecessary malloc by > > >> only requesting 1 (e.g. profile_target_name in bpftool) > > >> * bpftool's do_dump saves some malloc/free by growing and reusing its > > >> dynamic prog_info buf as it loops over progs to grab info and dump. > > >> > > >> Perf does need the full functionality of > > >> bpf_program__get_prog_info_linear and its accompanying structs + > > >> helpers, so copy the code to its codebase, migrate all other uses in the > > >> tree, and deprecate the helper in libbpf. > > >> > > >> Since the deprecated symbols continue to be included in perf some > > >> renaming was necessary in perf's copy, otherwise functionality is > > >> unchanged. > > >> > > >> This work was previously discussed in libbpf's issue tracker [0]. > > >> > > >> [0]: https://github.com/libbpf/libbpf/issues/313 > > >> > > >> v2->v3: > > >> * Remove v2's patch 1 ("libbpf: Migrate internal use of > > >> bpf_program__get_prog_info_linear"), which was applied [Andrii] > > >> * Add new patch 1 migrating error checking of libbpf calls to > > >> new scheme [Andrii, Quentin] > > >> * In patch 2, fix != -1 error check of libbpf call, improper realloc > > >> handling, and get rid of confusing macros [Andrii] > > >> * In patch 4, deprecate starting from 0.6 instead of 0.7 [Andrii] > > > > > > LGTM. Quentin, can you please take a look and ack as well? Thanks! > > > > Thanks Andrii for the Cc! I realised yesterday morning that I'd been hit > > by the unsubscription incident and missed v3 of this set. > > Yeah, super unfortunate this unsubscription. Had to go through that as well. > > > > > The changes look good to me, and you can add my tag to the first three > > patches: > > > > Acked-by: Quentin Monnet <quentin@isovalent.com> > > Thanks, I will. > > > > > Regarding patch 4, looking at the latest deprecations in libbpf, I would > > have expected the functions to be deprecated starting in v0.7, and not v0.6? > > The reason to do it in upcoming v0.6 is because there are no > replacement APIs that we are going to wait for. No point in delaying > the inevitable just for the sake of delaying it. > > > > > Other than that, on patch 2 (apologies for not answering inline), it > > would feel more natural, in do_dump()'s "for" loop in prog.c, to have > > the memset() above the call to bpf_obj_get_info_by_fd() (and to skip the > > zero-initialisation of "info") instead of at the end of the loop, which > > means a useless memset() just before we exit the loop. But probably not > > worth a respin just for that. > > Yeah, that bothered me a bit as well, but if you are also mentioning > that I'll just move it up as you suggested, while applying. Thanks. > Applied to bpf-next, thanks. > > > > Thanks, > > Quentin
bpf_program__get_prog_info_linear is a helper which wraps the bpf_obj_get_info_by_fd BPF syscall with some niceties that put all dynamic-length bpf_prog_info in one buffer contiguous with struct bpf_prog_info, and simplify the selection of which dynamic data to grab. The resultant combined struct, bpf_prog_info_linear, is persisted to file by 'perf' to enable later annotation of BPF prog data. libbpf includes some vaddr <-> offset conversion helpers for struct bpf_prog_info_linear to simplify this. This functionality is heavily tailored to perf's usecase, so its use as a general prog info API should be deemphasized in favor of just calling bpf_obj_get_info_by_fd, which can be more easily fit to purpose. Some examples from caller migrations in this series: * Some callers weren't requesting or using dynamic-sized prog info and are well served by a simple get_info_by_fd call (e.g. dump_prog_id_as_func_ptr in bpftool) * Some callers were requesting all of a specific dynamic info type but only using the first record, so can avoid unnecessary malloc by only requesting 1 (e.g. profile_target_name in bpftool) * bpftool's do_dump saves some malloc/free by growing and reusing its dynamic prog_info buf as it loops over progs to grab info and dump. Perf does need the full functionality of bpf_program__get_prog_info_linear and its accompanying structs + helpers, so copy the code to its codebase, migrate all other uses in the tree, and deprecate the helper in libbpf. Since the deprecated symbols continue to be included in perf some renaming was necessary in perf's copy, otherwise functionality is unchanged. This work was previously discussed in libbpf's issue tracker [0]. [0]: https://github.com/libbpf/libbpf/issues/313 v2->v3: * Remove v2's patch 1 ("libbpf: Migrate internal use of bpf_program__get_prog_info_linear"), which was applied [Andrii] * Add new patch 1 migrating error checking of libbpf calls to new scheme [Andrii, Quentin] * In patch 2, fix != -1 error check of libbpf call, improper realloc handling, and get rid of confusing macros [Andrii] * In patch 4, deprecate starting from 0.6 instead of 0.7 [Andrii] v1->v2: fix bpftool do_dump changes to clear bpf_prog_info after use and correctly pass realloc'd ptr back (patch 2) Dave Marchevsky (4): bpftool: Migrate -1 err checks of libbpf fn calls bpftool: use bpf_obj_get_info_by_fd directly perf: pull in bpf_program__get_prog_info_linear libbpf: deprecate bpf_program__get_prog_info_linear tools/bpf/bpftool/btf_dumper.c | 42 +-- tools/bpf/bpftool/prog.c | 158 ++++++++--- tools/bpf/bpftool/struct_ops.c | 2 +- tools/lib/bpf/libbpf.h | 3 + .../Documentation/perf.data-file-format.txt | 2 +- tools/perf/util/Build | 1 + tools/perf/util/annotate.c | 3 +- tools/perf/util/bpf-event.c | 41 ++- tools/perf/util/bpf-event.h | 2 +- tools/perf/util/bpf-utils.c | 261 ++++++++++++++++++ tools/perf/util/bpf-utils.h | 76 +++++ tools/perf/util/bpf_counter.c | 6 +- tools/perf/util/dso.c | 1 + tools/perf/util/env.c | 1 + tools/perf/util/header.c | 13 +- 15 files changed, 527 insertions(+), 85 deletions(-) create mode 100644 tools/perf/util/bpf-utils.c create mode 100644 tools/perf/util/bpf-utils.h