On Fri, Jun 2, 2023 at 1:34 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> >
> > > > > >> +
> > > > > >> +struct btf_kind_meta {
> > > > > >> + __u32 name_off; /* kind name string offset */
> > >
> > > I'm not sure why we'd need to record this for every KIND? The tool
> > > that doesn't know about this new kind can't do much about it anyways,
> > > so whether it knows that this is "KIND_NEW_FANCY" or just its ID #123
> > > doesn't make much difference?
> >
> > The name is certainly more meaningful than 123.
> > bpftool output is consumed by humans who will be able to tell the difference.
> > I'd keep the name here.
>
> Ok, it's fine. When I was originally proposing this compact metadata,
> I was trying to make it as minimal as possible, so adding 80 bytes
> just for string offset fields (plus a bunch of strings) felt like an
> unnecessary overhead. But it's not a big deal.
Exactly. It's just 4 * num_kinds bytes in and ~20 * num_kinds for
names, but it implements 'self description'.
Otherwise the names become an external knowledge and BTF is not self described.
> >
> > > > > > and would bump the BTF_VERSION to 2 to make it a 'milestone'.
> > >
> > > Bumping BTF_VERSION to 2 automatically makes BTF incompatible with all
> > > existing kernels (and potentially many tools that parse BTF). Given we
> > > can actually extend BTF in backwards compatible way by just adding an
> > > optional two fields to btf_header + extra bytes for metadata sections,
> > > why making our lives harder by bumping this version?
> >
> > I fail to see how bumping the version makes it harder.
> > libbpf needs to sanitize meta* fields in the struct btf_header on
> > older kernels anway. At the same time sanitizing the version from 2 to
> > 1
> > in the same header is one extra line of code in libbpf.
> > What am I missing?
>
> So I checked libbpf code, and libbpf doesn't really check the version
> field. So for the most part this BTF_VERSION bump wouldn't matter for
> any tool that's based on libbpf's struct btf API. But if libbpf did
> check version (as it probably should have), then by upgrading to newer
> Clang that would emit BTF with this metadata (but no new fancy
> BTF_KIND_KERNEL_FUNC or anything like that), we automatically make
> such BTF incompatible with all those tools.
>
> Kernel is a bit different because it's extremely strict about BTF. I'm
> more worried about tools like bpftool (but we don't check BTF_VERSION
> there due to libbpf), llvm-objdump (when it supports BTF), etc.
>
> On the other hand, what do we gain by bumping this BTF_VERSION?
The version bump will be an indication that
v2 of BTF has enough info in the format for any tool/kernel to consume it.
With v2 we should make BTF_KIND_META description mandatory.
If we keep it as v1 then the presence of BTF_KIND_META would be
an indication of 'self described' format.
Which is also ok-ish, but seems less clean.
zero vs not-zero of meta_off in btf_header is pretty much v1 vs v2.
>
> We don't have variable-sized KINDs, unless you are proposing to use
> vlen as "number of bytes" of ID payload.
Exactly. I'm proposing BTF_KIND_CHECKSUM and use vlen for size.
> And another KIND is
> automatically breaking backwards compat for all existing tools.
No. That's the whole point of 'self described'.
New kinds will not be breaking.
> For no
> good reason. This whole metadata is completely optional right now for
> anything that's using libbpf for BTF parsing. But adding KIND_ID makes
> it not optional at all.
and that's the crux of our disagreement.
If BTF_KIND_META are optional it's just a glorified comment inside BTF and
not a new 'self described' format.
If it's just a comment I'd rather not add it to BTF.
Such debug info can go to BTF.ext or don't do it at all.
The self described BTF would mean that struct btf_kind_meta-s contain
enough info for tools to parse from now on.
Imagine we didn't need CRC right now.
The self described format lands and now we want to add CRC.
If we're saying: "let's add a few hard coded fields to struct btf_header
or struct btf_metadata" then we failed.
It's not a self described format if we still need to extend hard coded
structs.
The idea of self description is that struct btf_kind_meta-s describe
absolutely everything about BTF from now on.
Meaning that not only things like ENUM64 and FUNC with addresses
become new KINDs, but crc-s and everything else is a new kind too,
because that's the only thing that btf_kind_meta-s can describe.
> Not sure what new KIND gives us in this case. This ID (whether it's
> "crc:abcdef" or just "661866dbea52bfac7420cd35d0e502d4ccc11bb6" or
> whatever) can be used by all application as is for comparison, you
> don't need to understand how it is generated at all.
That's fine. tools don't need to parse it.
With BTF_KIND_CHECKSUM the tools will just compare vlen sized binary data.
> >
> > > This also has a good property that 0 means "no ID", which helps with
> > > the base BTF case. Current "__u32 crc;" doesn't have this property and
> > > requires a flag.
> >
> > imo this crc addition is a litmus test for this self-described format.
> > If we cannot encode it as a new KIND* it means this self-described
> > idea is broken.
>
> We can, but this can be a straightforward and simple *opaque* string
> ID, so the new kind just seems unnecessary.
BTF_KIND_CHECKSUM can have a string in vlen part of it, but
it feels wrong to encode binary data as a string while everything else
in BTF is binary.
