Messages in this thread | | | Date | Mon, 16 May 2022 13:03:48 -0700 | Subject | Re: [PATCH bpf-next v3] bpftool: Use sysfs vmlinux when dumping BTF by ID | From | Yonghong Song <> |
| |
On 5/13/22 5:17 AM, Larysa Zaremba wrote: > Currently, dumping almost all BTFs specified by id requires > using the -B option to pass the base BTF. For kernel module > BTFs the vmlinux BTF sysfs path should work.
This is not precise. It should be that dumping all module BTFs specified by id requires using the -B option. In current situation, to dump a btf associated with a bpf program, -B option is not needed.
> > This patch simplifies dumping by ID usage by loading > vmlinux BTF from sysfs as base, if base BTF was not specified > and the ID corresponds to a kernel module BTF. > > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > Reviewed-by: Alexander Lobakin <alexandr.lobakin@intel.com> > --- > From v2[0]: > - instead of using vmlinux as base only after the first unsuccessful > attempt, set base to vmlinux before loading in applicable cases, precisely > if no base was provided by user and id belongs to a kernel module BTF. > > From v1[1]: > - base BTF is assumed to be vmlinux only for kernel BTFs. > > [0] https://lore.kernel.org/bpf/20220505130507.130670-1-larysa.zaremba@intel.com/ > [1] https://lore.kernel.org/bpf/20220428111442.111805-1-larysa.zaremba@intel.com/ > --- > tools/bpf/bpftool/btf.c | 65 +++++++++++++++++++++++++++++++++++------ > 1 file changed, 56 insertions(+), 9 deletions(-) > > diff --git a/tools/bpf/bpftool/btf.c b/tools/bpf/bpftool/btf.c > index a2c665beda87..0eb105c416fc 100644 > --- a/tools/bpf/bpftool/btf.c > +++ b/tools/bpf/bpftool/btf.c > @@ -459,6 +459,54 @@ static int dump_btf_c(const struct btf *btf, > return err; > } > > +static const char sysfs_vmlinux[] = "/sys/kernel/btf/vmlinux"; > + > +static struct btf *get_vmlinux_btf_from_sysfs(void) > +{ > + struct btf *base; > + > + base = btf__parse(sysfs_vmlinux, NULL); > + if (libbpf_get_error(base)) { > + p_err("failed to parse vmlinux BTF at '%s': %ld\n", > + sysfs_vmlinux, libbpf_get_error(base));
nit: libbpf_get_error() is called twice. Can be consolidated into one call.
> + base = NULL; > + } > + > + return base; > +} > + > +#define BTF_NAME_BUFF_LEN 64 > + > +static bool btf_is_kernel_module(__u32 btf_id) > +{ > + struct bpf_btf_info btf_info = {}; > + char btf_name[BTF_NAME_BUFF_LEN]; > + int btf_fd; > + __u32 len; > + int err; > + > + btf_fd = bpf_btf_get_fd_by_id(btf_id); > + if (btf_fd < 0) { > + p_err("can't get BTF object by id (%u): %s", > + btf_id, strerror(errno));
I am not sure whether we want p_err here or not. The function is to test whether btf is a kernel_module. If anything wrong happens here. The original logic follows and an error will be printed out any way.
> + return false; > + } > + > + len = sizeof(btf_info); > + btf_info.name = ptr_to_u64(btf_name); > + btf_info.name_len = sizeof(btf_name); > + err = bpf_obj_get_info_by_fd(btf_fd, &btf_info, &len); > + close(btf_fd); > + > + if (err) { > + p_err("can't get BTF (ID %u) object info: %s", > + btf_id, strerror(errno));
The same as above, probably we don't need p_err here.
> + return false; > + } > + > + return strncmp(btf_name, "vmlinux", sizeof(btf_name)) && btf_info.kernel_btf;
This should work considering current module names but the code itself doesn't sound right. The characters after "vmlinux" could be arbitrary. strncmp(btf_name, "vmlinux", strlen("vmlinux") + 1) is better.
> +} > + > static int do_dump(int argc, char **argv) > { > struct btf *btf = NULL, *base = NULL; > @@ -536,18 +584,11 @@ static int do_dump(int argc, char **argv) > NEXT_ARG(); > } else if (is_prefix(src, "file")) { > const char sysfs_prefix[] = "/sys/kernel/btf/"; > - const char sysfs_vmlinux[] = "/sys/kernel/btf/vmlinux"; > > if (!base_btf && > strncmp(*argv, sysfs_prefix, sizeof(sysfs_prefix) - 1) == 0 && > - strcmp(*argv, sysfs_vmlinux) != 0) { > - base = btf__parse(sysfs_vmlinux, NULL); > - if (libbpf_get_error(base)) { > - p_err("failed to parse vmlinux BTF at '%s': %ld\n", > - sysfs_vmlinux, libbpf_get_error(base)); > - base = NULL; > - } > - } > + strcmp(*argv, sysfs_vmlinux)) > + base = get_vmlinux_btf_from_sysfs(); > > btf = btf__parse_split(*argv, base ?: base_btf); > err = libbpf_get_error(btf); > @@ -591,6 +632,12 @@ static int do_dump(int argc, char **argv) > } > > if (!btf) { > + if (!base_btf && btf_is_kernel_module(btf_id)) { > + p_info("Warning: valid base BTF was not specified with -B option, falling back on standard base BTF (%s)", > + sysfs_vmlinux);
Having 'Warning' for p_info seems not appropriate. Similar to other p_info, you can just remove 'Warning: ".
> + base_btf = get_vmlinux_btf_from_sysfs(); > + } > + > btf = btf__load_from_kernel_by_id_split(btf_id, base_btf); > err = libbpf_get_error(btf); > if (err) {
| |