Thanks Andrii for taking a thorough look!
On Wed, 2022-03-02 at 17:46 -0800, Andrii Nakryiko wrote:
> There is a need (I needed it in libusdt, for example). Let's add it
> from the very beginning, especially that it can be done in *exactly*
> the same form as for skeletons.
Absolutely, I'll get that into the next version!
> seems like subskel's datasec codegen is considerably different (and
> simpler, really) than skeleton's, I'd add a separate function for it
> instead of all this if (subskel) special-casing. Main concern for
> skeleton is generating correct memory layout, while for subskel we
> just need to generate a list of pointers without any regard to memory
> layout.
I'm not 100% convinced given how much code would end up being duplicated but I
can go in that direction, if you'd prefer it.
>
> it's unfortunate to have to modify original BTF just to have this '*'
> added. If I remember correctly, C syntax for pointers has special
> case only for arrays and func prototypes, so it should work with this
> logic (not tested, obviously)
>
> 1. if top variable type is array or func_proto, set var_ident to "(*<variable>)"
> 2. otherwise set var_ident to "*<variable>"
>
> we'd need to test it for array and func_proto, but it definitely
> should work for all other cases
A couple of thoughts here:
1. We are not modifing the original BTF, we are layering in-memory-only types on
top of it. This ends up working transparently through btf_dump code, which is
the source of truth of what "correct" is. I think this is strictly better than
the alternative textual modifications to var_ident.
2. I guess we see the change differently - to me it's not just about adding an
asterisk but instead working with derivative types. This might come in handy in
other contexts that we haven't envisioned yet and I feel is a direction worth
supporting overall.
3. We have a full type system with layering and mixed file- and memory-based
storage. Why limit ourselves to templating instead of using it in the codegen?
If I were writing this from scratch, much of codegen_datasecs would instead
create in-memory BTF types and have btf_dump emit them (but that's not the
bikeshed I want to paint here!).
> > + char header_guard[MAX_OBJ_NAME_LEN + sizeof("__SKEL_H__")];
>
> use __SUBSKEL_H__ here?
Sure, I can introduce the .subskel.h suffix everywhere.
>
> > + strncpy(obj_name, *argv, MAX_OBJ_NAME_LEN - 1);
> > + obj_name[MAX_OBJ_NAME_LEN - 1] = '\0';
>
> we should probably force obj_name to be an empty string, so that all
> map names match their proper section names
Ah, maybe this is why bpf_map__name was not doing the right thing before. I
don't really like that we're relying on side effects of the empty obj_name but
I'll try it and see if anything breaks (the templating for one will need it
anyway).
>
> > + if (verifier_logs)
> > + /* log_level1 + log_level2 + stats, but not stable UAPI */
> > + opts.kernel_log_level = 1 + 2 + 4;
>
> hm.. we shouldn't need this, we are not loading anything into kernel
> and don't use light skeleton
You're right, will remove.
>
> > + obj = bpf_object__open_mem(obj_data, file_sz, &opts);
> > + err = libbpf_get_error(obj);
>
> no need for libbpf_get_error() anymore, bpftool is in libbpf 1.0 mode,
> so it will get NULL on error and error itself will be in errno
Ah, yes, I won't add new callsites.
>
> >
> > + map_type_id = btf__find_by_name_kind(btf, bpf_map__section_name(map), BTF_KIND_DATASEC);
>
> if we set obj_name to "", bpf_map__name() should return ELF section
> name here, so no need to expose this as an API
>
>
> oh, but also bpf_map__btf_value_type_id() should give you this ID directly
TIL, that's not obvious at all. There's a few places in gen.c that could be
simplified then - find_type_for_map goes through slicing the complete name and
walking over every BTF type to match on the slice. Is there some compatibility
reason to do that or is btf_value_type_id always there?
>
> > + for (i = 0, var = btf_var_secinfos(map_type), len = btf_vlen(map_type);
> > + i < len;
> > + i++, var++) {
>
> nit: move those long one-time assignments out of the for() loop and
> keep it single-line?
Yeah, I hate that structure too, I'll clean it up.
>
> >
> > + if (!subskel) { \n\
> > + errno = ENOMEM; \n\
> > + return NULL; \n\
>
> leaking obj here
Yeah, I noticed that I didn't use __destroy in the subskel, will fix for v2.
> > + /* walk through each symbol and emit the runtime representation
> > + */
>
> nit: keep this comment single-lined?
I did initially and checkpatch scolded me :)
>
> > + bpf_object__for_each_map(map, obj) {
> > + if (!bpf_map__is_internal(map))
> > + continue;
> > +
> > + if (!(bpf_map__map_flags(map) & BPF_F_MMAPABLE))
> > + continue;
> > +
> > + if (!get_map_ident(map, ident, sizeof(ident)))
> > + continue;
>
> this sequence of ifs seems so frequently repeated that it's probably
> time to add a helper function?
It is and it's annoying me too. I'll look at the whole iteration pattern more
closely.
>
>
> > + codegen("\
> > + \n\
> > + syms[%4$d].name = \"%1$s\"; \n\
> > + syms[%4$d].section = \"%3$s\"; \n\
> > + syms[%4$d].addr = (void**) &obj->%2$s.%1$s; \n\
> > + ", var_ident, ident, bpf_map__section_name(map), sym_idx);
> > + }
> > + }
>
> why not assign subskel->sym_cnt here using sym_idx and avoid that
> extra loop over all variables above?
Good call, will do.
>
> Quentin will remind you that you should also update the man page and
> bash completion script :)
Ah, yes, glad to see it's rst and I don't have to suffer groff/mdoc flashbacks!
Thanks,
Delyan
