On Sat, 14 Nov 2020, Yonghong Song wrote:
>
>
> On 11/14/20 8:04 AM, Alexei Starovoitov wrote:
> > On Fri, Nov 13, 2020 at 10:59 PM Andrii Nakryiko
> > <andrii.nakryiko@xxxxxxxxx> wrote:
> >>
> >> On Fri, Nov 13, 2020 at 10:11 AM Alan Maguire <alan.maguire@xxxxxxxxxx>
> >> wrote:
> >>>
> >>> bpf_snprintf_btf and bpf_seq_printf_btf use a "struct btf_ptr *"
> >>> argument that specifies type information about the type to
> >>> be displayed. Augment this information to include a module
> >>> name, allowing such display to support module types.
> >>>
> >>> Signed-off-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
> >>> ---
> >>> include/linux/btf.h | 8 ++++++++
> >>> include/uapi/linux/bpf.h | 5 ++++-
> >>> kernel/bpf/btf.c | 18 ++++++++++++++++++
> >>> kernel/trace/bpf_trace.c | 42
> >>> ++++++++++++++++++++++++++++++++----------
> >>> tools/include/uapi/linux/bpf.h | 5 ++++-
> >>> 5 files changed, 66 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/include/linux/btf.h b/include/linux/btf.h
> >>> index 2bf6418..d55ca00 100644
> >>> --- a/include/linux/btf.h
> >>> +++ b/include/linux/btf.h
> >>> @@ -209,6 +209,14 @@ static inline const struct btf_var_secinfo
> >>> *btf_type_var_secinfo(
> >>> const struct btf_type *btf_type_by_id(const struct btf *btf, u32
> >>> type_id);
> >>> const char *btf_name_by_offset(const struct btf *btf, u32 offset);
> >>> struct btf *btf_parse_vmlinux(void);
> >>> +#ifdef CONFIG_DEBUG_INFO_BTF_MODULES
> >>> +struct btf *bpf_get_btf_module(const char *name);
> >>> +#else
> >>> +static inline struct btf *bpf_get_btf_module(const char *name)
> >>> +{
> >>> + return ERR_PTR(-ENOTSUPP);
> >>> +}
> >>> +#endif
> >>> struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog);
> >>> #else
> >>> static inline const struct btf_type *btf_type_by_id(const struct btf
> >>> *btf,
> >>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> >>> index 162999b..26978be 100644
> >>> --- a/include/uapi/linux/bpf.h
> >>> +++ b/include/uapi/linux/bpf.h
> >>> @@ -3636,7 +3636,8 @@ struct bpf_stack_build_id {
> >>> * the pointer data is carried out to avoid kernel crashes
> >>> during
> >>> * operation. Smaller types can use string space on the
> >>> stack;
> >>> * larger programs can use map data to store the string
> >>> - * representation.
> >>> + * representation. Module-specific data structures can be
> >>> + * displayed if the module name is supplied.
> >>> *
> >>> * The string can be subsequently shared with userspace via
> >>> * bpf_perf_event_output() or ring buffer interfaces.
> >>> @@ -5076,11 +5077,13 @@ struct bpf_sk_lookup {
> >>> * potentially to specify additional details about the BTF pointer
> >>> * (rather than its mode of display) - is included for future use.
> >>> * Display flags - BTF_F_* - are passed to bpf_snprintf_btf separately.
> >>> + * A module name can be specified for module-specific data.
> >>> */
> >>> struct btf_ptr {
> >>> void *ptr;
> >>> __u32 type_id;
> >>> __u32 flags; /* BTF ptr flags; unused at present. */
> >>> + const char *module; /* optional module name. */
> >>
> >> I think module name is a wrong API here, similarly how type name was
> >> wrong API for specifying the type (and thus we use type_id here).
> >> Using the module's BTF ID seems like a more suitable interface. That's
> >> what I'm going to use for all kinds of existing BPF APIs that expect
> >> BTF type to attach BPF programs.
> >>
> >> Right now, we use only type_id and implicitly know that it's in
> >> vmlinux BTF. With module BTFs, we now need a pair of BTF object ID +
> >> BTF type ID to uniquely identify the type. vmlinux BTF now can be
> >> specified in two different ways: either leaving BTF object ID as zero
> >> (for simplicity and backwards compatibility) or specifying it's actual
> >> BTF obj ID (which pretty much always should be 1, btw). This feels
> >> like a natural extension, WDYT?
> >>
> >> And similar to type_id, no one should expect users to specify these
> >> IDs by hand, Clang built-in and libbpf should work together to figure
> >> this out for the kernel to use.
> >>
> >> BTW, with module names there is an extra problem for end users. Some
> >> types could be either built-in or built as a module (e.g., XFS data
> >> structures). Why would we require BPF users to care which is the case
> >> on any given host?
> >
> > +1.
> > As much as possible libbpf should try to hide the difference between
> > type in a module vs type in the vmlinux, since that difference most of the
> > time is irrelevant from bpf prog pov.
>
All sounds good to me - I've split out the libbpf fix and
put together an updated patchset for the helpers/test which
converts the currently unused __u32 "flags" field in
struct btf_ptr to an "obj_id" field. If obj_id is > 1 it
is presumed to be a module ID. I'd suggest we could move
ahead with those changes, using the more clunky methods
to retrieve the module-specific BTF id, and later fix up
the test to use Yonghong's __builtin_btf_type_id()
modification. Does that sound reasonable?
In connection to this, I wonder if libbpf could
benefit from a simple helper btf__id() (similar
to btf__fd()), allowing easy retrieval of the
object ID associated with module BTF? I suspect
we will always have cases in general-purpose
tracers where we need to look up BTF ids of
objects dynamically, so such a function would
help in that case.
> I just crafted a llvm patch where for __builtin_btf_type_id(), a 64bit value
> is returned instead of a 32bit value. libbpf can use the lower
> 32bit as the btf_type_id and upper 32bit as the kernel module btf id.
>
> https://reviews.llvm.org/D91489
>
> feel free to experiment with it to see whether it helps.
>
>
Great! I'll give it a try, thanks!
Alan
