On Fri 2023-06-09 16:35:16, Song Liu wrote:
>
>
> > On Jun 9, 2023, at 12:09 AM, Petr Mladek <pmladek@xxxxxxxx> wrote:
> >
> > On Fri 2023-06-02 16:24:01, Song Liu wrote:
> >> With CONFIG_LTO_CLANG, kallsyms.c:cleanup_symbol_name() removes symbols
> >> suffixes during comparison. This is problematic for livepatch, as
> >> kallsyms_on_each_match_symbol may find multiple matches for the same
> >> symbol, and fail with:
> >>
> >> livepatch: unresolvable ambiguity for symbol 'xxx' in object 'yyy'
> >>
> >> Fix this by using kallsyms_on_each_symbol instead, and matching symbols
> >> exactly.
> >>
> >> --- a/kernel/livepatch/core.c
> >> +++ b/kernel/livepatch/core.c
> >> @@ -166,7 +159,7 @@ static int klp_find_object_symbol(const char *objname, const char *name,
> >> if (objname)
> >> module_kallsyms_on_each_symbol(objname, klp_find_callback, &args);
> >> else
> >> - kallsyms_on_each_match_symbol(klp_match_callback, name, &args);
> >> + kallsyms_on_each_symbol(klp_find_callback, &args);
> >
> > AFAIK, you have put a lot of effort to optimize the search recently.
> > The speedup was amazing, see commit 4dc533e0f2c04174e1ae
> > ("kallsyms: Add helper kallsyms_on_each_match_symbol()").
> >
>
> That's not me. :) Or maybe you meant Josh?
Ah, I see. I am sorry, I am bad at names.
> > Do we really need to waste this effort completely?
> >
> > What about creating variants:
> >
> > + kallsyms_on_each_match_exact_symbol()
> > + kallsyms_lookup_exact_names()
> > + compare_exact_symbol_name()
> >
> > Where compare_exact_symbol_name() would not try comparing with
> > cleanup_symbol_name()?
>
> The rationale is that livepatch symbol look up is not a hot path,
> and the changes (especially with kallsyms_lookup_exact_names) seem
> an overkill.
I agree that it is a slow path.
Well, Zhen put a lot of effort into the optimization. I am not sure
what was the primary motivation. But it would be harsh to remove it
without asking.
Zhen, what was the motivation for the speedup of kallsyms, please?
> OTOH, this version is simpler and should work just as
> well.
Sure. But we should double check Zhen's motivation.
Anyway, iterating over all symbols costs a lot. See also
the commit f5bdb34bf0c9314548f2d ("livepatch: Avoid CPU hogging
with cond_resched").
Best Regards,
Petr
