On Wed, Jun 7, 2023 at 3:05 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > On Wed, 2023-06-07 at 14:47 -0700, Andrii Nakryiko wrote: > > On Wed, Jun 7, 2023 at 9:14 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > > > > > On Wed, 2023-06-07 at 08:29 -0700, Yonghong Song wrote: > > > > > > > > On 6/7/23 4:55 AM, Eduard Zingerman wrote: > > > > > On Tue, 2023-06-06 at 13:30 +0200, Toke Høiland-Jørgensen wrote: > > > > > [...] > > > > > > > > > > > > As for bumping the version number, I don't think it's a good idea to > > > > > > deliberately break compatibility this way unless it's absolutely > > > > > > necessary. With "absolutely necessary" meaning "things will break in > > > > > > subtle ways in any case, so it's better to make the breakage obvious". > > > > > > But it libbpf is not checking the version field anyway, that becomes > > > > > > kind of a moot point, as bumping it doesn't really gain us anything, > > > > > > then... > > > > > > > > > > It seems to me that in terms of backward compatibility, the ability to > > > > > specify the size for each kind entry is more valuable than the > > > > > capability to add new BTF kinds: > > > > > - The former allows for extending kind records in > > > > > a backward-compatible manner, such as adding a function address to > > > > > BTF_KIND_FUNC. > > > > > > > > Eduard, the new proposal is to add new kind, e.g., BTF_KIND_KFUNC, which > > > > will have an 'address' field. BTF_KIND_KFUNC is for kernel functions. > > > > So we will not have size compatibility issue for BTF_KIND_FUNC. > > > > > > Well, actually this might be a way to avoid BTF_KIND_KFUNC :) > > > What I wanted to say is that any use of this feature leads to > > > incompatibility with current BTF parsers, as either size of existing > > > kinds would be changed or a new kind with unknown size would be added. > > > It seems to me that this warrants version bump (or some other way to > > > signal existing parsers that format is incompatible). > > > > It is probably too late to have existing KINDs changing their size. If > > this layout metadata was mandatory from the very beginning, then we > > could have relied on it for determining new extra fields for > > BTF_KIND_FUNC. > > > > As things stand right now, new BTF_KIND_KFUNC is both a signal of > > newer format (for kernel-side BTF; nothing changes for BPF object file > > BTFs, which is great side-effect making backwards compat pain > > smaller), and is a simpler and safer way to add extra information. > > > > > > > > > > > > > > - The latter is much more fragile. Types refer to each other, > > > > > compatibility is already lost once a new "unknown" tag is introduced > > > > > in a type chain. > > > > > > > > > > However, changing the size of existing BTF kinds is itself a > > > > > backward-incompatible change. Therefore, a version bump may be > > > > > warranted in this regard. > > > > > > > See above and previous emails. Not having to bump version means we can > > start emitting this layout info from Clang and pahole with no extra > > opt-in flags, and not worry about breaking existing tools and apps. > > This is great, so let's not ruin that property :) > > I'm not sure I understand how this would help: > - If no new kinds are added, absence or presence of metadata section > does not matter. Old parsers would ignore it, new parsers would work > as old parsers, so there is no added value in generating metadata. > - As soon as new kind is added old parsers are broken. > > What am I missing? I was arguing both against changing BTF_KIND_FUNC (not adding new fields to id, not changing its size based on klag, etc) and against bumping BTF_VERSION to 2. For kernel-side BTF breakage is unavoidable, unfortunately, either if we extend BTF_KIND_FUNC with addr or add new kind BTF_KIND_KFUNC. Any application that would want to open such new kernel BTF would need to upgrade to latest libbpf to be able to do it. What I'm trying to avoid is also breaking (unnecessarily) BPF object file-side BTF generated by Clang. BPF object BTF also has BTF_KIND_FUNC generated for each entry program and subprogram. So if we change anything about BTF_KIND_FUNC, we break existing tools, so we need to be careful about that. If we are talking about this btf_layout information separately from extending kernel-side function info. By adding just that, we can keep both kernel and BPF object BTFs backwards compatible with existing tooling (unless someone decided to be very strict about checking BTF_VERSION or btf_header bytes after last known field). So tl;dr: - btf_layout is useful and can be done in a backwards compatible way, if we don't bump BTF_VERSION; - we can start emitting it from Clang and pahole unconditionally, if done this way; - adding addrs to either new BTF_KIND_KFUNC or extending existing BTF_KIND_FUNC is separate from btf_layout (it just prompted btf_layout prioritization to help avoid unnecessary tooling breakages in the future), and I'm leaning towards new BTF_KIND_KFUNC instead of trying to extend existing BTF_KIND_FUNC.
