| Message ID | 20211011082031.4148337-3-davemarchevsky@fb.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | libbpf: deprecate bpf_program__get_prog_info_linear | expand |
On Mon, Oct 11, 2021 at 1:20 AM Dave Marchevsky <davemarchevsky@fb.com> wrote: > > To prepare for impending deprecation of libbpf's > bpf_program__get_prog_info_linear, migrate uses of this function to use > bpf_obj_get_info_by_fd. > > Since the profile_target_name and dump_prog_id_as_func_ptr helpers were > only looking at the first func_info, avoid grabbing the rest to save a > malloc. For do_dump, add a more full-featured helper, but avoid > free/realloc of buffer when possible for multi-prog dumps. > > Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> > --- > tools/bpf/bpftool/btf_dumper.c | 40 +++++---- > tools/bpf/bpftool/prog.c | 154 +++++++++++++++++++++++++-------- > 2 files changed, 144 insertions(+), 50 deletions(-) > > diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c > index 9c25286a5c73..0f85704628bf 100644 > --- a/tools/bpf/bpftool/btf_dumper.c > +++ b/tools/bpf/bpftool/btf_dumper.c > @@ -32,14 +32,16 @@ static int dump_prog_id_as_func_ptr(const struct btf_dumper *d, > const struct btf_type *func_proto, > __u32 prog_id) > { > - struct bpf_prog_info_linear *prog_info = NULL; > const struct btf_type *func_type; > + int prog_fd = -1, func_sig_len; > + struct bpf_prog_info info = {}; > + __u32 info_len = sizeof(info); > const char *prog_name = NULL; > - struct bpf_func_info *finfo; > struct btf *prog_btf = NULL; > - struct bpf_prog_info *info; > - int prog_fd, func_sig_len; > + struct bpf_func_info finfo; > + __u32 finfo_rec_size; > char prog_str[1024]; > + int err; > > /* Get the ptr's func_proto */ > func_sig_len = btf_dump_func(d->btf, prog_str, func_proto, NULL, 0, > @@ -55,22 +57,27 @@ static int dump_prog_id_as_func_ptr(const struct btf_dumper *d, > if (prog_fd == -1) please change this to (prog_fd < 0), see [0] for why we should check all the other places in bpftool to see if there are any patterns like this that would break on libbpf 1.0 (cc Quentin as well) [0] https://github.com/libbpf/libbpf/wiki/Libbpf-1.0-migration-guide#direct-error-code-returning-libbpf_strict_direct_errs > goto print; > > - prog_info = bpf_program__get_prog_info_linear(prog_fd, > - 1UL << BPF_PROG_INFO_FUNC_INFO); > - close(prog_fd); > - if (IS_ERR(prog_info)) { > - prog_info = NULL; > + err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len); > + if (err) > goto print; > - } > - info = &prog_info->info; > > - if (!info->btf_id || !info->nr_func_info) > + if (!info.btf_id || !info.nr_func_info) > + goto print; > + > + finfo_rec_size = info.func_info_rec_size; > + memset(&info, 0, sizeof(info)); > + info.nr_func_info = 1; > + info.func_info_rec_size = finfo_rec_size; > + info.func_info = ptr_to_u64(&finfo); > + > + err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len); > + if (err) > goto print; > - prog_btf = btf__load_from_kernel_by_id(info->btf_id); > + > + prog_btf = btf__load_from_kernel_by_id(info.btf_id); > if (libbpf_get_error(prog_btf)) > goto print; > - finfo = u64_to_ptr(info->func_info); > - func_type = btf__type_by_id(prog_btf, finfo->type_id); > + func_type = btf__type_by_id(prog_btf, finfo.type_id); > if (!func_type || !btf_is_func(func_type)) > goto print; > > @@ -92,7 +99,8 @@ static int dump_prog_id_as_func_ptr(const struct btf_dumper *d, > prog_str[sizeof(prog_str) - 1] = '\0'; > jsonw_string(d->jw, prog_str); > btf__free(prog_btf); > - free(prog_info); > + if (prog_fd != -1) similarly, prog_fd >= 0 here; also, isn't this a fix? Can you add Fixes: tag then? > + close(prog_fd); > return 0; > } > > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c > index a24ea7e26aa4..3b3ccc7b6dd4 100644 > --- a/tools/bpf/bpftool/prog.c > +++ b/tools/bpf/bpftool/prog.c > @@ -98,6 +98,72 @@ static enum bpf_attach_type parse_attach_type(const char *str) > return __MAX_BPF_ATTACH_TYPE; > } > > +#define holder_prep_needed_rec_sz(nr, rec_size)\ > +({ \ > + holder.nr = info->nr; \ > + needed += holder.nr * rec_size; \ > +}) > + > +#define holder_prep_needed(nr, rec_size) \ > +({ \ > + holder.nr = info->nr; \ > + holder.rec_size = info->rec_size; \ > + needed += holder.nr * holder.rec_size; \ > +}) > + > +#define holder_set_ptr(field, nr, rec_size) \ > +({ \ > + holder.field = ptr_to_u64(ptr); \ > + ptr += nr * rec_size; \ > +}) > + > +static int prep_prog_info(struct bpf_prog_info *const info, enum dump_mode mode, > + void **info_data, size_t *const info_data_sz) > +{ > + struct bpf_prog_info holder = {}; > + size_t needed = 0; > + void *ptr; > + > + if (mode == DUMP_JITED) > + holder_prep_needed_rec_sz(jited_prog_len, 1); > + else > + holder_prep_needed_rec_sz(xlated_prog_len, 1); > + > + holder_prep_needed_rec_sz(nr_jited_ksyms, sizeof(__u64)); > + holder_prep_needed_rec_sz(nr_jited_func_lens, sizeof(__u32)); > + holder_prep_needed(nr_func_info, func_info_rec_size); > + holder_prep_needed(nr_line_info, line_info_rec_size); > + holder_prep_needed(nr_jited_line_info, jited_line_info_rec_size); > + > + if (needed > *info_data_sz) { > + *info_data = realloc(*info_data, needed); never do `x = realloc(x);`, if realloc fails, original memory is leaked. Temporary variable is necessary to work with realloc, see a bunch of other places in libbpf where we use realloc for an example. > + if (!*info_data) > + return -1; > + *info_data_sz = needed; > + } > + > + ptr = *info_data; > + > + if (mode == DUMP_JITED) > + holder_set_ptr(jited_prog_insns, holder.jited_prog_len, 1); > + else > + holder_set_ptr(xlated_prog_insns, holder.xlated_prog_len, 1); > + > + holder_set_ptr(jited_ksyms, holder.nr_jited_ksyms, sizeof(__u64)); > + holder_set_ptr(jited_func_lens, holder.nr_jited_func_lens, sizeof(__u32)); > + holder_set_ptr(func_info, holder.nr_func_info, holder.func_info_rec_size); > + holder_set_ptr(line_info, holder.nr_line_info, holder.line_info_rec_size); > + holder_set_ptr(jited_line_info, holder.nr_jited_line_info, > + holder.jited_line_info_rec_size); tbh, I completely lost track of what these holder_xxx() macro actually do here... You saved few lines of code, but I think we lost a lot in readability > + > + *info = holder; instead of copying at the end, why not fill out the passed in bpf_prog_info directly? Of course *info stuff is annoying, but that's solved with a temporary variable, no? > + return 0; > +} > + > +#undef holder_prep_needed > +#undef holder_prep_needed_rec_sz > +#undef holder_set_ptr > + [...]
On Wed, 20 Oct 2021 at 18:37, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Mon, Oct 11, 2021 at 1:20 AM Dave Marchevsky <davemarchevsky@fb.com> wrote: > > > > To prepare for impending deprecation of libbpf's > > bpf_program__get_prog_info_linear, migrate uses of this function to use > > bpf_obj_get_info_by_fd. > > > > Since the profile_target_name and dump_prog_id_as_func_ptr helpers were > > only looking at the first func_info, avoid grabbing the rest to save a > > malloc. For do_dump, add a more full-featured helper, but avoid > > free/realloc of buffer when possible for multi-prog dumps. > > > > Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> > > --- > > tools/bpf/bpftool/btf_dumper.c | 40 +++++---- > > tools/bpf/bpftool/prog.c | 154 +++++++++++++++++++++++++-------- > > 2 files changed, 144 insertions(+), 50 deletions(-) > > > > diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c > > index 9c25286a5c73..0f85704628bf 100644 > > --- a/tools/bpf/bpftool/btf_dumper.c > > +++ b/tools/bpf/bpftool/btf_dumper.c > > @@ -32,14 +32,16 @@ static int dump_prog_id_as_func_ptr(const struct btf_dumper *d, > > const struct btf_type *func_proto, > > __u32 prog_id) > > { > > - struct bpf_prog_info_linear *prog_info = NULL; > > const struct btf_type *func_type; > > + int prog_fd = -1, func_sig_len; > > + struct bpf_prog_info info = {}; > > + __u32 info_len = sizeof(info); > > const char *prog_name = NULL; > > - struct bpf_func_info *finfo; > > struct btf *prog_btf = NULL; > > - struct bpf_prog_info *info; > > - int prog_fd, func_sig_len; > > + struct bpf_func_info finfo; > > + __u32 finfo_rec_size; > > char prog_str[1024]; > > + int err; > > > > /* Get the ptr's func_proto */ > > func_sig_len = btf_dump_func(d->btf, prog_str, func_proto, NULL, 0, > > @@ -55,22 +57,27 @@ static int dump_prog_id_as_func_ptr(const struct btf_dumper *d, > > if (prog_fd == -1) > > please change this to (prog_fd < 0), see [0] for why > > we should check all the other places in bpftool to see if there are > any patterns like this that would break on libbpf 1.0 (cc Quentin as > well) > > [0] https://github.com/libbpf/libbpf/wiki/Libbpf-1.0-migration-guide#direct-error-code-returning-libbpf_strict_direct_errs Hi! Looking at bpftool's code (looking for "-1"), I could find only two occurrences of that pattern, one in btf_dumper.c as noted above, and another one in struct_ops.c: fd = bpf_map_get_fd_by_id(id); if (fd == -1) { [...] } Dave, are you willing to address it as well? I can send a patch later this week otherwise. Thanks, Quentin
diff --git a/tools/bpf/bpftool/btf_dumper.c b/tools/bpf/bpftool/btf_dumper.c index 9c25286a5c73..0f85704628bf 100644 --- a/tools/bpf/bpftool/btf_dumper.c +++ b/tools/bpf/bpftool/btf_dumper.c @@ -32,14 +32,16 @@ static int dump_prog_id_as_func_ptr(const struct btf_dumper *d, const struct btf_type *func_proto, __u32 prog_id) { - struct bpf_prog_info_linear *prog_info = NULL; const struct btf_type *func_type; + int prog_fd = -1, func_sig_len; + struct bpf_prog_info info = {}; + __u32 info_len = sizeof(info); const char *prog_name = NULL; - struct bpf_func_info *finfo; struct btf *prog_btf = NULL; - struct bpf_prog_info *info; - int prog_fd, func_sig_len; + struct bpf_func_info finfo; + __u32 finfo_rec_size; char prog_str[1024]; + int err; /* Get the ptr's func_proto */ func_sig_len = btf_dump_func(d->btf, prog_str, func_proto, NULL, 0, @@ -55,22 +57,27 @@ static int dump_prog_id_as_func_ptr(const struct btf_dumper *d, if (prog_fd == -1) goto print; - prog_info = bpf_program__get_prog_info_linear(prog_fd, - 1UL << BPF_PROG_INFO_FUNC_INFO); - close(prog_fd); - if (IS_ERR(prog_info)) { - prog_info = NULL; + err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len); + if (err) goto print; - } - info = &prog_info->info; - if (!info->btf_id || !info->nr_func_info) + if (!info.btf_id || !info.nr_func_info) + goto print; + + finfo_rec_size = info.func_info_rec_size; + memset(&info, 0, sizeof(info)); + info.nr_func_info = 1; + info.func_info_rec_size = finfo_rec_size; + info.func_info = ptr_to_u64(&finfo); + + err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len); + if (err) goto print; - prog_btf = btf__load_from_kernel_by_id(info->btf_id); + + prog_btf = btf__load_from_kernel_by_id(info.btf_id); if (libbpf_get_error(prog_btf)) goto print; - finfo = u64_to_ptr(info->func_info); - func_type = btf__type_by_id(prog_btf, finfo->type_id); + func_type = btf__type_by_id(prog_btf, finfo.type_id); if (!func_type || !btf_is_func(func_type)) goto print; @@ -92,7 +99,8 @@ static int dump_prog_id_as_func_ptr(const struct btf_dumper *d, prog_str[sizeof(prog_str) - 1] = '\0'; jsonw_string(d->jw, prog_str); btf__free(prog_btf); - free(prog_info); + if (prog_fd != -1) + close(prog_fd); return 0; } diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c index a24ea7e26aa4..3b3ccc7b6dd4 100644 --- a/tools/bpf/bpftool/prog.c +++ b/tools/bpf/bpftool/prog.c @@ -98,6 +98,72 @@ static enum bpf_attach_type parse_attach_type(const char *str) return __MAX_BPF_ATTACH_TYPE; } +#define holder_prep_needed_rec_sz(nr, rec_size)\ +({ \ + holder.nr = info->nr; \ + needed += holder.nr * rec_size; \ +}) + +#define holder_prep_needed(nr, rec_size) \ +({ \ + holder.nr = info->nr; \ + holder.rec_size = info->rec_size; \ + needed += holder.nr * holder.rec_size; \ +}) + +#define holder_set_ptr(field, nr, rec_size) \ +({ \ + holder.field = ptr_to_u64(ptr); \ + ptr += nr * rec_size; \ +}) + +static int prep_prog_info(struct bpf_prog_info *const info, enum dump_mode mode, + void **info_data, size_t *const info_data_sz) +{ + struct bpf_prog_info holder = {}; + size_t needed = 0; + void *ptr; + + if (mode == DUMP_JITED) + holder_prep_needed_rec_sz(jited_prog_len, 1); + else + holder_prep_needed_rec_sz(xlated_prog_len, 1); + + holder_prep_needed_rec_sz(nr_jited_ksyms, sizeof(__u64)); + holder_prep_needed_rec_sz(nr_jited_func_lens, sizeof(__u32)); + holder_prep_needed(nr_func_info, func_info_rec_size); + holder_prep_needed(nr_line_info, line_info_rec_size); + holder_prep_needed(nr_jited_line_info, jited_line_info_rec_size); + + if (needed > *info_data_sz) { + *info_data = realloc(*info_data, needed); + if (!*info_data) + return -1; + *info_data_sz = needed; + } + + ptr = *info_data; + + if (mode == DUMP_JITED) + holder_set_ptr(jited_prog_insns, holder.jited_prog_len, 1); + else + holder_set_ptr(xlated_prog_insns, holder.xlated_prog_len, 1); + + holder_set_ptr(jited_ksyms, holder.nr_jited_ksyms, sizeof(__u64)); + holder_set_ptr(jited_func_lens, holder.nr_jited_func_lens, sizeof(__u32)); + holder_set_ptr(func_info, holder.nr_func_info, holder.func_info_rec_size); + holder_set_ptr(line_info, holder.nr_line_info, holder.line_info_rec_size); + holder_set_ptr(jited_line_info, holder.nr_jited_line_info, + holder.jited_line_info_rec_size); + + *info = holder; + return 0; +} + +#undef holder_prep_needed +#undef holder_prep_needed_rec_sz +#undef holder_set_ptr + static void print_boot_time(__u64 nsecs, char *buf, unsigned int size) { struct timespec real_time_ts, boot_time_ts; @@ -791,16 +857,18 @@ prog_dump(struct bpf_prog_info *info, enum dump_mode mode, static int do_dump(int argc, char **argv) { - struct bpf_prog_info_linear *info_linear; + struct bpf_prog_info info = {}; + __u32 info_len = sizeof(info); + size_t info_data_sz = 0; + void *info_data = NULL; char *filepath = NULL; bool opcodes = false; bool visual = false; enum dump_mode mode; bool linum = false; - int *fds = NULL; int nb_fds, i = 0; + int *fds = NULL; int err = -1; - __u64 arrays; if (is_prefix(*argv, "jited")) { if (disasm_init()) @@ -860,46 +928,46 @@ static int do_dump(int argc, char **argv) goto exit_close; } - if (mode == DUMP_JITED) - arrays = 1UL << BPF_PROG_INFO_JITED_INSNS; - else - arrays = 1UL << BPF_PROG_INFO_XLATED_INSNS; - - arrays |= 1UL << BPF_PROG_INFO_JITED_KSYMS; - arrays |= 1UL << BPF_PROG_INFO_JITED_FUNC_LENS; - arrays |= 1UL << BPF_PROG_INFO_FUNC_INFO; - arrays |= 1UL << BPF_PROG_INFO_LINE_INFO; - arrays |= 1UL << BPF_PROG_INFO_JITED_LINE_INFO; - if (json_output && nb_fds > 1) jsonw_start_array(json_wtr); /* root array */ for (i = 0; i < nb_fds; i++) { - info_linear = bpf_program__get_prog_info_linear(fds[i], arrays); - if (IS_ERR_OR_NULL(info_linear)) { + err = bpf_obj_get_info_by_fd(fds[i], &info, &info_len); + if (err) { + p_err("can't get prog info: %s", strerror(errno)); + break; + } + + err = prep_prog_info(&info, mode, &info_data, &info_data_sz); + if (err) { + p_err("can't grow prog info_data"); + break; + } + + err = bpf_obj_get_info_by_fd(fds[i], &info, &info_len); + if (err) { p_err("can't get prog info: %s", strerror(errno)); break; } if (json_output && nb_fds > 1) { jsonw_start_object(json_wtr); /* prog object */ - print_prog_header_json(&info_linear->info); + print_prog_header_json(&info); jsonw_name(json_wtr, "insns"); } else if (nb_fds > 1) { - print_prog_header_plain(&info_linear->info); + print_prog_header_plain(&info); } - err = prog_dump(&info_linear->info, mode, filepath, opcodes, - visual, linum); + err = prog_dump(&info, mode, filepath, opcodes, visual, linum); if (json_output && nb_fds > 1) jsonw_end_object(json_wtr); /* prog object */ else if (i != nb_fds - 1 && nb_fds > 1) printf("\n"); - free(info_linear); if (err) break; close(fds[i]); + memset(&info, 0, sizeof(info)); } if (json_output && nb_fds > 1) jsonw_end_array(json_wtr); /* root array */ @@ -908,6 +976,7 @@ static int do_dump(int argc, char **argv) for (; i < nb_fds; i++) close(fds[i]); exit_free: + free(info_data); free(fds); return err; } @@ -2004,41 +2073,58 @@ static void profile_print_readings(void) static char *profile_target_name(int tgt_fd) { - struct bpf_prog_info_linear *info_linear; - struct bpf_func_info *func_info; + struct bpf_func_info func_info; + struct bpf_prog_info info = {}; + __u32 info_len = sizeof(info); const struct btf_type *t; + __u32 func_info_rec_size; struct btf *btf = NULL; char *name = NULL; + int err; - info_linear = bpf_program__get_prog_info_linear( - tgt_fd, 1UL << BPF_PROG_INFO_FUNC_INFO); - if (IS_ERR_OR_NULL(info_linear)) { - p_err("failed to get info_linear for prog FD %d", tgt_fd); - return NULL; + err = bpf_obj_get_info_by_fd(tgt_fd, &info, &info_len); + if (err) { + p_err("failed to bpf_obj_get_info_by_fd for prog FD %d", tgt_fd); + goto out; } - if (info_linear->info.btf_id == 0) { + if (info.btf_id == 0) { p_err("prog FD %d doesn't have valid btf", tgt_fd); goto out; } - btf = btf__load_from_kernel_by_id(info_linear->info.btf_id); + func_info_rec_size = info.func_info_rec_size; + if (info.nr_func_info == 0) { + p_err("bpf_obj_get_info_by_fd for prog FD %d found 0 func_info", tgt_fd); + goto out; + } + + memset(&info, 0, sizeof(info)); + info.nr_func_info = 1; + info.func_info_rec_size = func_info_rec_size; + info.func_info = ptr_to_u64(&func_info); + + err = bpf_obj_get_info_by_fd(tgt_fd, &info, &info_len); + if (err) { + p_err("failed to get func_info for prog FD %d", tgt_fd); + goto out; + } + + btf = btf__load_from_kernel_by_id(info.btf_id); if (libbpf_get_error(btf)) { p_err("failed to load btf for prog FD %d", tgt_fd); goto out; } - func_info = u64_to_ptr(info_linear->info.func_info); - t = btf__type_by_id(btf, func_info[0].type_id); + t = btf__type_by_id(btf, func_info.type_id); if (!t) { p_err("btf %d doesn't have type %d", - info_linear->info.btf_id, func_info[0].type_id); + info.btf_id, func_info.type_id); goto out; } name = strdup(btf__name_by_offset(btf, t->name_off)); out: btf__free(btf); - free(info_linear); return name; }
To prepare for impending deprecation of libbpf's bpf_program__get_prog_info_linear, migrate uses of this function to use bpf_obj_get_info_by_fd. Since the profile_target_name and dump_prog_id_as_func_ptr helpers were only looking at the first func_info, avoid grabbing the rest to save a malloc. For do_dump, add a more full-featured helper, but avoid free/realloc of buffer when possible for multi-prog dumps. Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> --- tools/bpf/bpftool/btf_dumper.c | 40 +++++---- tools/bpf/bpftool/prog.c | 154 +++++++++++++++++++++++++-------- 2 files changed, 144 insertions(+), 50 deletions(-)