On Fri, Sep 24, 2021 at 4:54 PM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> On Wed, Sep 22, 2021 at 04:11:13AM IST, Andrii Nakryiko wrote:
> > On Mon, Sep 20, 2021 at 7:15 AM Kumar Kartikeya Dwivedi
> > <memxor@xxxxxxxxx> wrote:
> > > [...]
> > > + return -E2BIG;
> > > + }
> > > + ext->ksym.offset = index;
> >
> > > + } else {
> > > + ext->ksym.offset = 0;
> > > }
> >
> > I think it will be cleaner if you move the entire offset determination
> > logic after all the other checks are performed and ext is mostly
> > populated. That will also make the logic shorter and simpler because
> > if ayou find kern_btf_fd match, you can exit early (or probably rather
>
> Ack to everything else (including the other mail), but...
>
> > goto to report the match and exit). Otherwise
> >
>
> This sentence got eaten up.
No idea what I was going to say here, sorry... Sometimes Gmail UI
glitches with undo/redo, maybe that's what happened here. Doesn't
matter, ignore the "Otherwise" part.
>
> > >
> > > kern_func = btf__type_by_id(kern_btf, kfunc_id);
> >
> > this is actually extremely wasteful for module BTFs. Let's add
> > internal (at least for now) helper that will search only for "own" BTF
> > types in the BTF, skipping types in base BTF. Something like
> > btf_type_by_id_own()?
> >
>
> Just to make sure I am not misunderstanding: I don't see where this is wasteful.
> btf_type_by_id seems to not be searching anything, but just returns pointer in
> base BTF if kfunc_id < btf->start_id, otherwise in module BTF.
>
Hm, sorry... Right sentiment and thought, but wrong piece of code to
quote it on.
I had in mind the btf__find_by_name_kind() use in find_ksym_btf_id().
Once we start going over each module, we shouldn't be re-checking
vmlinux BTF when doing btf__find_by_name_kind. It should only check
the types that each module BTF adds on top of vmlinux BTF. That's what
would be good to optimize, especially as more complicated BPF programs
will start using more ksym vars and funcs.
> What am I missing? I guess the 'kern_btf' name was the source of confusion? If
> so, I'll rename it.
>
> Thanks.
>
> --
> Kartikeya
