On Wed, Nov 3, 2021 at 11:21 AM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Wed, Nov 3, 2021 at 4:26 AM Quentin Monnet <quentin@xxxxxxxxxxxxx> wrote: > > > > 2021-11-02 16:06 UTC-0700 ~ Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> > > > On Mon, Nov 1, 2021 at 3:46 PM Dave Marchevsky <davemarchevsky@xxxxxx> 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@xxxxxxxxxxxxx> > > 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
