On Thu, Mar 21, 2024 at 5:02 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Thu, Mar 21, 2024 at 4:55 PM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
> >
> > >> static int kallsyms_cb(unsigned long long sym_addr, char sym_type,
> > >> const char *sym_name, void *ctx)
> > >> {
> > >> + int lto_enabled = check_lto_kernel();
> > >> + char orig_name[PATH_MAX], *res;
> > >> struct bpf_object *obj = ctx;
> > >> const struct btf_type *t;
> > >> struct extern_desc *ext;
> > >>
> > >> - ext = find_extern_by_name(obj, sym_name);
> > >> + /* Only check static variables in data sections */
> > >> + if (sym_type == 'd' && obj->need_kallsyms && lto_enabled == 1) {
> > > why bother grepping config.gz ?
> > > I see no harm in doing below strstr unconditionally.
> >
> > Do you mean we skip condition
> > sym_type == 'd' && obj->need_kallsyms && lto_enabled == 1
> > all together?
> >
> > For condition sym_type == 'd', Andrii suggested (in private discussion)
> > to focus on data first since that is the issue we hitted. Of course
> > we could do all symbols here too.
> >
> > For condition obj->need_kallsyms, I can remove this one.
> >
> > For lto_enabled == 1, the main goal is to avoid extra overhead for
> > not-lto kernels.
> >
> > I guess that the overhead is not that bad since typically symbol name
> > is not long. So removing all conditions seems indeed a viable solution.
>
> I was suggesting to remove the last lto_enabled == 1 check and
> check_lto_kernel() since it will simplify the patch significantly and
> won't cause any slowdown.
+1, grepping Kconfig seems like an overkill
> The first two checks... I'm not sure.
I'd say we shouldn't do this for functions, because if LLVM rewrites
them, then usually that means that function signature is changed,
which seems dangerous to just silently resolve. I'd start with data
symbols for now.
> The less corner cases the better. strstr is fast enough.
yep, I doubt it would be noticeable if we do strstr()
> Ok to remove them all.
