Re: [PATCH v4] kallsyms: add names of built-in modules

Masahiro Yamada
2020-01-20T06:38:52Z

Hi Eugene,

On Thu, Jan 9, 2020 at 3:32 AM Eugene Loh <eugene.loh@oracle.com> wrote:
>
> On 12/18/2019 08:28 PM, Masahiro Yamada wrote:
>
> > On Thu, Dec 19, 2019 at 12:29 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >> Couple of notes:
> >> 1) this affects code that doesn't really have a maintainer. I could
> >> take it in my tree, but I would like to have acks from other
> >> maintainers. Perhaps Jessica Yu (Module maintainer), and probably one
> >> from Linus himself.
> >>
> >> 2) Do not send new versions of a patch as a reply to the old version. I
> >> and many other maintainers sort our inbox by threads, and I look at the
> >> top of the thread for patches. That is, if there's another version of a
> >> patch that is a reply to a previous version, it is basically off my
> >> radar, unless I happen to notice it by chance (which I did with this
> >> email).
> >>
> >> You can send your v4 patch again, but please send it as its own thread,
> >> that way it will be on the radar of other maintainers. Hopefully we can
> >> get some acks on this as well.
>
> Sorry. I misunderstood some process doc. But before I resend...
>
> > I do not like this patch.
> >
> > scripts/Makefile.modbuiltin is really ugly.
> > It traverses all the directories once again.
> >
> > This patch makes it even worse,
> > Kbuild would traverse the
> > whole directories three times.
> >
> > I was thinking to remove scripts/Makefile.modbuiltin
> > and Kconfig's tristate.conf entirely
> > because it is possible to generate modules.builtin more simply.
>
> Sorry about the delayed response, due in part to holidays. Thank you
> for your on-going review and the pointer to
> https://lore.kernel.org/patchwork/project/lkml/list/?series=423205
>
> I agree your proposed patch simplifies some build code, but this is
> long-standing code. Also, the build time -- either that would be saved
> by your patch or that would be incurred by a third traversal -- is
> miniscule.
>
> Further, I do not see how to add object-to-module information to your
> proposed scheme. Can you suggest something? If not, then it seems the
> proposed code simplification is limiting functionality.

The object-to-module information can be retrieved by a similar
way as I did in
https://lore.kernel.org/patchwork/project/lkml/list/?series=423205

But, even if modules_think.builtin is produced in a new way,
there would make no difference in the fact that
the build system needs to generate modules_think.builtin and
.tmp_vmlinux.range, and kallsyms must integrate a big
parser of them.

So, I think this patch lacks the taste as overall.

>
> > As I said, the name of builtin module is not fixed info.
> > And, this makes kallsyms fat just for less important info.
>
> The name of the builtin module can be ambiguous in some cases, but in
> most cases it is not. Indeed, the extra information is typically
> useful, and comments from, e.g., Linus and Steve were positive about
> adding that information to kallsyms. Further, we have even heard
> favorable feedback for adding such built-in-module information to
> available_filter_functions as well.

In my opinion, this should be determined by the balance
between the added value and the ugliness of the code.

(Real) module names are obvious, but as I stated,
built-in module names are somewhat subtle, so I do not
like to extend it too much.

Perhaps, I was the only person who reviewed the code in detail.
After looking at how this feature is integrated,
I do not believe this should go in. Sorry.

Masahiro Yamada

>
> >>> On 12/10/2019 09:48 AM, eugene.loh@oracle.com wrote:
> >>>> From: Eugene Loh <eugene.loh@oracle.com>
> >>>>
> >>>> /proc/kallsyms is very useful for tracers and other tools that need
> >>>> to map kernel symbols to addresses.
> >>>>
> >>>> It would be useful if there were a mapping between kernel symbol and
> >>>> module name that only changed when the kernel source code is changed.
> >>>> This mapping should not vanish simply because a module becomes built
> >>>> into the kernel.

--
Best Regards
Masahiro Yamada