On Thu, May 25, 2023 at 3:20 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote: > > On 25/05/2023 09:52, Jiri Olsa wrote: > > On Mon, May 22, 2023 at 02:31:01PM -0700, Andrii Nakryiko wrote: > >> On Thu, May 18, 2023 at 5:26 PM Alexei Starovoitov > >> <alexei.starovoitov@xxxxxxxxx> wrote: > >>> > >>> On Thu, May 18, 2023 at 11:26 AM Yonghong Song <yhs@xxxxxxxx> wrote: > >>>>> I wonder now when the address will be stored as number (not string) we > >>>>> could somehow generate relocation records and have the module loader > >>>>> do the relocation automatically > >>>>> > >>>>> not sure how that works for vmlinux when it's loaded/relocated on boot > >>>> > >>>> Right, actual module address will mostly not match the one in dwarf. > >>>> Some during module btf load, we should modify btf address as well > >>>> for later use? Yes, may need to reuse some routines used in initial > >>>> module relocation. > >>> > >>> > >>> Few thoughts: > >>> > >>> Initially I felt that single FUNC with multiple DECL_TAG(addr) > >>> is better, since BTF for all funcs is the same and it's likely > >>> one static inline function that the compiler decided not to inline > >>> (like cpumask_weight), so when libbpf wants to attach prog to it > >>> the kernel should automatically attach in all places. > >>> But then noticed that actually different functions with > >>> the same name and proto will be deduplicated into one. > >>> Their bodies at different locations will be different. > >>> Example: seq_show. > >>> In this case it's better to let libbpf pick the exact one to attach. > >>> Then realized that even the same function like cpumask_weight() > >>> might have different body at different locations due to optimizations. > >>> I don't think dwarf contains enough info to distinguish all the combinations. > >>> > >>> Considering all that it's better to keep one BTF kind_func -> one addr. > >>> If it's extended the way Alan is proposing with kind_flag > >>> the dedup logic will not combine them due to different addresses. > >> > >> I've discussed this w/ Alexei and Yonghong offline, so will summarize > >> what I said here. I don't think that we should go the route of adding > >> kflag to BTF_KIND_FUNC. As Yonghong pointed out, previously only vlen > >> and kind determined byte size of the type, and so adding a third > >> variable (kflag), which would apply only to BTF_KIND_FUNC, seems like > >> an unnecessary new complication. > >> > >> I propose to go with an entirely new kind instead, we have plenty of > >> them left. This new kind will be pretty kernel-specific, so could be > >> targeted for kernel use cases better without adding unnecessary > >> complications to Clang. BTF_KIND_FUNCs generated by Clang for .bpf.o > >> files don't need addr, they are meaningless and Clang doesn't know > >> anything about addresses anyways. So we can keep Clang unchanged and > >> more backwards compatible. > >> > >> But now that this new kind (BTF_KIND_KERNEL_FUNC? KFUNC would be > >> misleading, unfortunately) is kernel-specific and generated by pahole > >> only, besides addr we can add some flags field and use them to mark > >> function as defined as kfunc or not, or (as a hypothetical example) > >> traceable or not, or maybe we even have inline flag some day, etc. > >> Something that makes sense mostly for kernel functions. > >> > >> Having said all that, given we are going to break all existing > >> BTF-aware tools again with a new kind, we should really couple all > >> this work with making BTF self-describing as discussed in [0], so that > >> future changes like this won't break older bpftool and other similar > >> tools, unnecessarily. > > > > nice, would be great to have this and eventually got rid of new pahole > > enable/disable options, makes sense to do this before adding new type > > > > jirka > > > > agreed; I'd been thinking the same and I've been working on a proof-of- > concept of this based on our previous discussions, I'll send it out as > soon as I've got it roughly working. nice! it would be great to not have every older tool break whenever we add a tiny extension to BTF > > With respect to the question of having a new kind, I'm not sure I agree > with the above. We've already broken the "vlen == number of objects > following" for BTF_KIND_FUNC, where vlen is used to represent linkage > information instead. I'd say BTF_KIND_FUNC and its abuse of vlen is just an example of what we shouldn't do going forward. But even that still allows us to compactly describe each btf_type's size as a pair of (fixed_sz, vlen_sz), where the total size will be fixed_sz + vlen * vlen_sz. For BTF_KIND_FUNC vlen_sz will be zero, even if vlen is non-zero. If we allow klag to change bytes size of a type, we'd need another sizing dimension, which is what I try to avoid here. Look at ENUM and ENUM64. We chose to add a new kind for ENUM64 because of different byte sizing needs, instead of abusing kflag for this, and I think this was the right decision. Let's do that here as well. > > To me, it feels more natural to have continuity across different object > types (kernel versus BPF program) with BTF_KIND_FUNC: the fact that > it's hard to come up with an alternate name is perhaps a reflection of > this. Most characteristics (aside from "is a kfunc") seem to be shared > across kernel and BPF program functions, but the best way to judge > is probably to come up with as complete a list as is possible I suppose. Even the address that you'll add to BTF_KIND_FUNC doesn't apply to BTF emitted for BPF object files. So while I can sympathize (conceptually) with the desire to have one kind for "all thing function", it will cause more problems. Again, look at ENUM and ENUM64 case. Sure, we had to teach BTF dedup and BPF CO-RE about both kinds to represent the same enum concept, but we'd have to do the same even if just added a kflag for 64-bit enum. So my point is that tools will still need to be extended to take advantage of new information, but reusing the same kind is causing more problems and is not solving any. > > In order to accommodate a metadata description using existing > BTF_KIND_FUNC, we can have a metadata flag that can say > "KFLAG set means singular object following of object_size" that is > set for BTF_KIND_FUNC. We can mark it as discouraged for future > use. > > One argument I definitely see for a new kind representing kernel > functions is if it were the case that we might need N elements > _and_ a singular object following the btf_type to represent it. > I don't currently see any use for such a model for function > representation, but if that is anticipated somehow, it might be > worth having a new kind to support that sort of representation. We can never foresee stuff like this, but it's best to not design ourselves into the corner. We have enough space for BTF_KIND_xxx, which gives you all the same benefits. Let's keep it simple and straightforward. > > Alan > > >> > >> Which, btw, is another reason to not use kflag to determine the size > >> of btf_type. Proposed solution in [0] assumes that kind + vlen defines > >> the size. We should probably have dedicated discussion for > >> self-describing BTF, but I think both changes have to be done in the > >> same release window. > >> > >> [0] https://lore.kernel.org/bpf/CAEf4BzYjWHRdNNw4B=eOXOs_ONrDwrgX4bn=Nuc1g8JPFC34MA@xxxxxxxxxxxxxx/#t > >> > >>> > >>> Also turned out that the kernel doesn't validate decl_tag string. > >>> The following code loads without error: > >>> __attribute__((btf_decl_tag("\x10\xf0"))); > >>> > >>> I'm not sure whether we want to tighten decl_tag validation and how. > >>> If we keep it as-is we can use func+decl_tag approach > >>> to add 4 bytes of addr in the binary format (if 1st byte is not zero). > >>> But it feels like a hack, since the kernel needs to be changed > >>> anyway to adjust the addresses after module loading and kernel relocation. > >>> So func with kind_flag seems like the best approach. > >>> > >>> Regarding relocation of address in the kernel and modules... > >>> We just need to add base_addr to all addrs-es recorded in BTF. > >>> Both for kernel and for module BTFs. > >>> Shouldn't be too complicated. > >> > >> yep, KASLR seems simple enough to handle by the kernel itself at boot time. > >
