On Mon, Jul 26, 2021 at 6:12 PM Hengqi Chen <hengqi.chen@xxxxxxxxx> wrote: > > > > On 7/27/21 6:49 AM, Andrii Nakryiko wrote: > > On Fri, Jul 23, 2021 at 10:13 PM Hengqi Chen <hengqi.chen@xxxxxxxxx> wrote: > >> > >> Add libbpf_load_vmlinux_btf/libbpf_load_module_btf APIs. > >> This is part of the libbpf v1.0. [1] > >> > >> [1] https://github.com/libbpf/libbpf/issues/280 > > > > Saying it's part of libbpf 1.0 effort and given a link to Github PR is > > not really a sufficient commit message. Please expand on what you are > > doing in the patch and why. > > > > Will do. > > >> > >> Signed-off-by: Hengqi Chen <hengqi.chen@xxxxxxxxx> > >> --- > >> tools/lib/bpf/btf.c | 24 +++++++++++++++++++++++- > >> tools/lib/bpf/btf.h | 2 ++ > >> tools/lib/bpf/libbpf.c | 8 ++++---- > >> tools/lib/bpf/libbpf.map | 2 ++ > >> 4 files changed, 31 insertions(+), 5 deletions(-) > >> > >> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c > >> index b46760b93bb4..414e1c5635ef 100644 > >> --- a/tools/lib/bpf/btf.c > >> +++ b/tools/lib/bpf/btf.c > >> @@ -4021,7 +4021,7 @@ static void btf_dedup_merge_hypot_map(struct btf_dedup *d) > >> */ > >> if (d->hypot_adjust_canon) > >> continue; > >> - > >> + > >> if (t_kind == BTF_KIND_FWD && c_kind != BTF_KIND_FWD) > >> d->map[t_id] = c_id; > >> > >> @@ -4395,6 +4395,11 @@ static int btf_dedup_remap_types(struct btf_dedup *d) > >> * data out of it to use for target BTF. > >> */ > >> struct btf *libbpf_find_kernel_btf(void) > >> +{ > >> + return libbpf_load_vmlinux_btf(); > >> +} > >> + > >> +struct btf *libbpf_load_vmlinux_btf(void) > >> { > >> struct { > >> const char *path_fmt; > >> @@ -4440,6 +4445,23 @@ struct btf *libbpf_find_kernel_btf(void) > >> return libbpf_err_ptr(-ESRCH); > >> } > >> > >> +struct btf *libbpf_load_module_btf(const char *mod) > > > > So we probably need to allow user to pre-load and re-use vmlinux BTF > > for efficiency, especially if they have some use-case to load a lot of > > BTFs. > > > > Should the API change to this ? > > struct btf *libbpf_load_module_btf(struct btf *base, const char *mod) > > It seems better for the use-case you mentioned. Something like this, yeah. Let's put struct btf * as the last argument and module name as a first one, to follow the btf__parse_split() convention of having base_btf the last. And maybe let's call base a "vmlinux_btf", as in this case it's pretty specific that it's supposed to be the vmlinux BTF, not just any random BTF. > > >> +{ > >> + char path[80]; > >> + struct btf *base; > >> + int err; > >> + > >> + base = libbpf_load_vmlinux_btf(); > >> + err = libbpf_get_error(base); > >> + if (err) { > >> + pr_warn("Error loading vmlinux BTF: %d\n", err); > >> + return base; > > > > libbpf_err_ptr() needs to be used here, pr_warn() could have destroyed > > errno already > > > > OK. > > >> + } > >> + > >> + snprintf(path, sizeof(path), "/sys/kernel/btf/%s", mod); > >> + return btf__parse_split(path, base); > > > > so who's freeing base BTF in this case? > > > > Sorry, missed that. > But if we change the signature, then leave this to user. Right, that's what I was getting at. If we make vmlinux BTF non-optional, then user will have to own freeing it, just like we do that for other APIs w/ base BTF. > > >> +} > >> + > >> int btf_type_visit_type_ids(struct btf_type *t, type_id_visit_fn visit, void *ctx) > >> { > >> int i, n, err; > > > > [...] > >
