RFC - KBUILD_MODNAME is misleading in builtins, as seen in /proc/dynamic_debug/control

RFC - KBUILD_MODNAME is misleading in builtins, as seen in /proc/dynamic_debug/control

[jim.cromie]

dynamic-debug METADATA uses KBUILD_MODNAME as:

#define DEFINE_DYNAMIC_DEBUG_METADATA_CLS(name, cls, fmt)       \
        static struct _ddebug  __aligned(8)                     \
        __section("__dyndbg") name = {                          \
                .modname = KBUILD_MODNAME,                      \

This is going amiss for some builtins, ie those enabled here, by:

    echo module main +pmf > /proc/dynamic_debug_control
    grep =pmf /proc/dynamic_debug/control

init/main.c:1187 [main]initcall_blacklist =pmf "blacklisting initcall %s\n"
init/main.c:1226 [main]initcall_blacklisted =pmf "initcall %s blacklisted\n"
init/main.c:1432 [main]run_init_process =pmf "  with arguments:\n"
init/main.c:1434 [main]run_init_process =pmf "    %s\n"
init/main.c:1435 [main]run_init_process =pmf "  with environment:\n"
init/main.c:1437 [main]run_init_process =pmf "    %s\n"
kernel/module/main.c:336 [main]find_symbol =pmf "Failed to find symbol %s\n"
kernel/module/main.c:567 [main]already_uses =pmf "%s uses %s!\n"
kernel/module/main.c:571 [main]already_uses =pmf "%s does not use %s!\n"
kernel/module/main.c:586 [main]add_module_usage =pmf "Allocating new
usage for %s.\n"
kernel/module/main.c:627 [main]module_unload_free =pmf "%s unusing %s\n"
kernel/module/main.c:733 [main]__do_sys_delete_module =pmf "%s already dying\n"
kernel/module/main.c:1345 [main]simplify_symbols =pmf "Common symbol: %s\n"
kernel/module/main.c:1353 [main]simplify_symbols =pmf "Absolute
symbol: 0x%08lx\n"
kernel/module/main.c:1508 [main]__layout_sections =pmf "\t%s\n"
kernel/module/main.c:1526 [main]layout_sections =pmf "Core section
allocation order:\n"
kernel/module/main.c:1529 [main]layout_sections =pmf "Init section
allocation order:\n"
kernel/module/main.c:2168 [main]move_module =pmf "final section addresses:\n"
kernel/module/main.c:2183 [main]move_module =pmf "\t0x%lx %s\n"
kernel/module/main.c:2921 [main]__do_sys_init_module =pmf
"init_module: umod=%p, len=%lu, uargs=%p\n"
kernel/module/main.c:2942 [main]__do_sys_finit_module =pmf
"finit_module: fd=%d, uargs=%p, flags=%i\n"
drivers/base/power/main.c:135 [main]device_pm_add =pmf "Adding info for %s:%s\n"
drivers/base/power/main.c:156 [main]device_pm_remove =pmf "Removing
info for %s:%s\n"
drivers/base/power/main.c:175 [main]device_pm_move_before =pmf "Moving
%s:%s before %s:%s\n"
drivers/base/power/main.c:189 [main]device_pm_move_after =pmf "Moving
%s:%s after %s:%s\n"
drivers/base/power/main.c:202 [main]device_pm_move_last =pmf "Moving
%s:%s to end of list\n"
drivers/base/power/main.c:441 [main]pm_dev_dbg =pmf "PM: %s%s%s driver
flags: %x\n"
drivers/base/power/main.c:467 [main]dpm_show_time =pmf "%s%s%s of
devices %s after %ld.%03ld msecs\n"
bash-5.2#

Basically, KBUILD_MODNAME appears to get basename,
not something specific, set in some config (such as pm, or module)
or cleverly picked out of the path (power is possible)

this compromises the clarity of  dyndbg's module keyword

is some heuristic possible to improve this,
with a manual setting to fix when heuristic fails ?

Re: RFC - KBUILD_MODNAME is misleading in builtins, as seen in /proc/dynamic_debug/control

[jim.cromie]

On Mon, Mar 20, 2023 at 12:35 PM Jason Baron <jbaron@akamai.com> wrote:
>
>
>
> On 3/20/23 1:05 AM, jim.cromie@gmail.com wrote:
> > dynamic-debug METADATA uses KBUILD_MODNAME as:
> >
> > #define DEFINE_DYNAMIC_DEBUG_METADATA_CLS(name, cls, fmt)       \
> >          static struct _ddebug  __aligned(8)                     \
> >          __section("__dyndbg") name = {                          \
> >                  .modname = KBUILD_MODNAME,                      \
> >
> > This is going amiss for some builtins, ie those enabled here, by:
> >
> >      echo module main +pmf > /proc/dynamic_debug_control
> >      grep =pmf /proc/dynamic_debug/control
> >
> > init/main.c:1187 [main]initcall_blacklist =pmf "blacklisting initcall %s\n"
> > init/main.c:1226 [main]initcall_blacklisted =pmf "initcall %s blacklisted\n"
> > init/main.c:1432 [main]run_init_process =pmf "  with arguments:\n"
> > init/main.c:1434 [main]run_init_process =pmf "    %s\n"
> > init/main.c:1435 [main]run_init_process =pmf "  with environment:\n"
> > init/main.c:1437 [main]run_init_process =pmf "    %s\n"
>
>
> Hi Jim,
>
> So if I'm following correctly, this is not a new issue, the 'module'
> name for dynamic debug has always been this way for builtin.

It is not a new issue - both PM and init-main have been in [main] for some time.

I believe that with
cfc1d277891e module: Move all into module/

module's module-name joined them, changing from [module] to [main]


We could do
> something simple and just normalize it when we initially create the
> table, but setting the 'module name' to 'core' or 'builtin' or something
> for all these?

core and builtin would both lump all those separate modules together,
making it less meaningful.

having stable names independent of M vs Y config choices is imperative, ISTM.

Also, I dont think "only builtins are affected" captures the whole problem.
I dont recall amdgpu or other modules changing when built with =y

Theres some subtlety in how KBUILD_MODNAME is set,
and probably many current users who like its current behavior.
A new var ?

1st, I think that anything tristate gets a sensible value,
but at least some of the builtin-only "modules" get basenames, by default.

arch/x86/events/amd/ibs.c:1398 [ibs]force_ibs_eilvt_setup =_ "No EILVT
entry available\n"
arch/x86/events/intel/pt.c:797 [pt]pt_topa_dump =_ "# table @%p, off
%llx size %zx\n"w=%16llx\n"

kvm gets a solid name, because tristate ?

arch/x86/kvm/mmu/mmu.c:6661 [kvm]kvm_mmu_invalidate_mmio_sptes =_
"kvm: kvm [%i]: zapping shadow pages for mmio generation wraparound\n"
arch/x86/kvm/hyperv.c:1402 [kvm]kvm_hv_set_msr_pw =_ "kvm [%i]: vcpu%i
hv crash (0x%llx 0x%llx 0x%llx 0x%llx 0x%llx)\n"

kvm-intel and kvm-amd get their names elsewhere.

arch/x86/kvm/vmx/nested.c:207 [kvm_intel]nested_vmx_abort =_
"kvm_intel: nested vmx abort, indicator %d\n"
arch/x86/kvm/vmx/nested.c:913 [kvm_intel]nested_vmx_load_msr =_
"kvm_intel: %s cannot read MSR entry (%u, 0x%08llx)\n"

arch/x86/kvm/svm/avic.c:860 [kvm_amd]get_pi_vcpu_info =_ "SVM: %s: use
GA mode for irq %u\n"
arch/x86/kvm/svm/avic.c:889 [kvm_amd]avic_pi_update_irte =_ "SVM: %s:
host_irq=%#x, guest_irq=%#x, set=%#x\n"

iow, I dont know..

>
> Thanks,
>
> -Jason

Re: RFC - KBUILD_MODNAME is misleading in builtins, as seen in /proc/dynamic_debug/control

[Luis Chamberlain]

On Mon, Mar 20, 2023 at 01:59:28PM -0600, jim.cromie@gmail.com wrote:
> On Mon, Mar 20, 2023 at 12:35 PM Jason Baron <jbaron@akamai.com> wrote:
> >
> >
> >
> > On 3/20/23 1:05 AM, jim.cromie@gmail.com wrote:
> > > dynamic-debug METADATA uses KBUILD_MODNAME as:
> > >
> > > #define DEFINE_DYNAMIC_DEBUG_METADATA_CLS(name, cls, fmt)       \
> > >          static struct _ddebug  __aligned(8)                     \
> > >          __section("__dyndbg") name = {                          \
> > >                  .modname = KBUILD_MODNAME,                      \
> > >
> > > This is going amiss for some builtins, ie those enabled here, by:
> > >
> > >      echo module main +pmf > /proc/dynamic_debug_control
> > >      grep =pmf /proc/dynamic_debug/control
> > >
> > > init/main.c:1187 [main]initcall_blacklist =pmf "blacklisting initcall %s\n"
> > > init/main.c:1226 [main]initcall_blacklisted =pmf "initcall %s blacklisted\n"
> > > init/main.c:1432 [main]run_init_process =pmf "  with arguments:\n"
> > > init/main.c:1434 [main]run_init_process =pmf "    %s\n"
> > > init/main.c:1435 [main]run_init_process =pmf "  with environment:\n"
> > > init/main.c:1437 [main]run_init_process =pmf "    %s\n"
> >
> >
> > Hi Jim,
> >
> > So if I'm following correctly, this is not a new issue, the 'module'
> > name for dynamic debug has always been this way for builtin.
> 
> It is not a new issue - both PM and init-main have been in [main] for some time.
> 
> I believe that with
> cfc1d277891e module: Move all into module/
> 
> module's module-name joined them, changing from [module] to [main]

If there was a regression due to this, we'd be very interested in
hearing about it. Aaron he did the work to move the code to its own directory. 

> We could do
> > something simple and just normalize it when we initially create the
> > table, but setting the 'module name' to 'core' or 'builtin' or something
> > for all these?
> 
> core and builtin would both lump all those separate modules together,
> making it less meaningful.
> 
> having stable names independent of M vs Y config choices is imperative, ISTM.
> 
> Also, I dont think "only builtins are affected" captures the whole problem.
> I dont recall amdgpu or other modules changing when built with =y
> 
> Theres some subtlety in how KBUILD_MODNAME is set,
> and probably many current users who like its current behavior.
> A new var ?
> 
> 1st, I think that anything tristate gets a sensible value,
> but at least some of the builtin-only "modules" get basenames, by default.

In general we could all benefit from an enhancement for a shortname for
things which could be modules being built-in. We're now seeing requests
for dynamic debug, but it could also be usefulf for Nick's future work
to help userspace tools / tracing map kallsysms to specific modules when
built-in.

To that end I had suggested the current state of affairs & current difficulty
in trying to get us a name for this here:

https://lore.kernel.org/all/Y/kXDqW+7d71C4wz@bombadil.infradead.org/

I ended up suggesting perhaps we need a -DPOSSIBLE_MODULE then if we
could *somehow* pull that off perhaps then we could instead use
-DPOSSIBLE_KBUILD_MODNAME which would ensure a consistent symbol when
a module is built-in as well.

That still leaves the difficulty in trying to gather possible-obj-m as
a future challenge.

  Luis

Re: RFC - KBUILD_MODNAME is misleading in builtins, as seen in /proc/dynamic_debug/control

[Masahiro Yamada]

On Tue, Mar 21, 2023 at 6:04 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Mon, Mar 20, 2023 at 01:59:28PM -0600, jim.cromie@gmail.com wrote:
> > On Mon, Mar 20, 2023 at 12:35 PM Jason Baron <jbaron@akamai.com> wrote:
> > >
> > >
> > >
> > > On 3/20/23 1:05 AM, jim.cromie@gmail.com wrote:
> > > > dynamic-debug METADATA uses KBUILD_MODNAME as:
> > > >
> > > > #define DEFINE_DYNAMIC_DEBUG_METADATA_CLS(name, cls, fmt)       \
> > > >          static struct _ddebug  __aligned(8)                     \
> > > >          __section("__dyndbg") name = {                          \
> > > >                  .modname = KBUILD_MODNAME,                      \
> > > >
> > > > This is going amiss for some builtins, ie those enabled here, by:
> > > >
> > > >      echo module main +pmf > /proc/dynamic_debug_control
> > > >      grep =pmf /proc/dynamic_debug/control
> > > >
> > > > init/main.c:1187 [main]initcall_blacklist =pmf "blacklisting initcall %s\n"
> > > > init/main.c:1226 [main]initcall_blacklisted =pmf "initcall %s blacklisted\n"
> > > > init/main.c:1432 [main]run_init_process =pmf "  with arguments:\n"
> > > > init/main.c:1434 [main]run_init_process =pmf "    %s\n"
> > > > init/main.c:1435 [main]run_init_process =pmf "  with environment:\n"
> > > > init/main.c:1437 [main]run_init_process =pmf "    %s\n"
> > >
> > >
> > > Hi Jim,
> > >
> > > So if I'm following correctly, this is not a new issue, the 'module'
> > > name for dynamic debug has always been this way for builtin.
> >
> > It is not a new issue - both PM and init-main have been in [main] for some time.
> >
> > I believe that with
> > cfc1d277891e module: Move all into module/
> >
> > module's module-name joined them, changing from [module] to [main]
>
> If there was a regression due to this, we'd be very interested in
> hearing about it. Aaron he did the work to move the code to its own directory.
>
> > We could do
> > > something simple and just normalize it when we initially create the
> > > table, but setting the 'module name' to 'core' or 'builtin' or something
> > > for all these?
> >
> > core and builtin would both lump all those separate modules together,
> > making it less meaningful.
> >
> > having stable names independent of M vs Y config choices is imperative, ISTM.
> >
> > Also, I dont think "only builtins are affected" captures the whole problem.
> > I dont recall amdgpu or other modules changing when built with =y
> >
> > Theres some subtlety in how KBUILD_MODNAME is set,
> > and probably many current users who like its current behavior.
> > A new var ?
> >
> > 1st, I think that anything tristate gets a sensible value,
> > but at least some of the builtin-only "modules" get basenames, by default.
>
> In general we could all benefit from an enhancement for a shortname for
> things which could be modules being built-in. We're now seeing requests
> for dynamic debug, but it could also be usefulf for Nick's future work
> to help userspace tools / tracing map kallsysms to specific modules when
> built-in.



I think I rejected it some years ago.
He comes back again and again with almost the same approaches,
until he finds a "sponsor" (it's you) who will get it in.

Recently, I rejected the Kbuild changes again.




> To that end I had suggested the current state of affairs & current difficulty
> in trying to get us a name for this here:
>
> https://lore.kernel.org/all/Y/kXDqW+7d71C4wz@bombadil.infradead.org/
>
> I ended up suggesting perhaps we need a -DPOSSIBLE_MODULE then if we
> could *somehow* pull that off perhaps then we could instead use
> -DPOSSIBLE_KBUILD_MODNAME which would ensure a consistent symbol when
> a module is built-in as well.
>
> That still leaves the difficulty in trying to gather possible-obj-m as
> a future challenge.


I do not understand your point.

Why is it important to achieve "precisely-exactly-possible-obj-m" instead of
"perhaps-possible-obj-m"?


When "modprobe foo" succeeds, the user is sure that the kernel
provides the feature "foo" (but he does not care if
"foo" is built-in or modular).

I think that is the point for kmod check also module.builtin
before saying no.


When CONFIG_FOO=y, "modprobe foo" succeeds because "foo" is available
as built-in.
When CONFIG_FOO=n, "modprobe foo" fails because "foo" is not available anywhere.
I do not see anything wrong here.

Why do we need to make "modprobe foo" fail, where the feature "foo" is
still available
but just because we cannot compile it as a module?

He spams with MODULE_LICENSE removal with no justification.

-- 
Best Regards
Masahiro Yamada

Re: RFC - KBUILD_MODNAME is misleading in builtins, as seen in /proc/dynamic_debug/control

[Nick Alcock]

On 21 Mar 2023, Masahiro Yamada stated:
> He spams with MODULE_LICENSE removal with no justification.

Luis a) asked me to do it b) asked me to split it up like that (believe
me, it was extra work). A good few maintainers subsequently protested
that it wasn't split up even more finely.

I consider doing what Luis asks to constitute justification enough to
give it a try in this area -- or at least I'm not going to say no
without a damn good reason!

Re: RFC - KBUILD_MODNAME is misleading in builtins, as seen in /proc/dynamic_debug/control

[Luis Chamberlain]

On Tue, Mar 21, 2023 at 06:03:01PM +0900, Masahiro Yamada wrote:
> On Tue, Mar 21, 2023 at 6:04 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > On Mon, Mar 20, 2023 at 01:59:28PM -0600, jim.cromie@gmail.com wrote:
> > > On Mon, Mar 20, 2023 at 12:35 PM Jason Baron <jbaron@akamai.com> wrote:
> > > >
> > > >
> > > >
> > > > On 3/20/23 1:05 AM, jim.cromie@gmail.com wrote:
> > > > > dynamic-debug METADATA uses KBUILD_MODNAME as:
> > > > >
> > > > > #define DEFINE_DYNAMIC_DEBUG_METADATA_CLS(name, cls, fmt)       \
> > > > >          static struct _ddebug  __aligned(8)                     \
> > > > >          __section("__dyndbg") name = {                          \
> > > > >                  .modname = KBUILD_MODNAME,                      \
> > > > >
> > > > > This is going amiss for some builtins, ie those enabled here, by:
> > > > >
> > > > >      echo module main +pmf > /proc/dynamic_debug_control
> > > > >      grep =pmf /proc/dynamic_debug/control
> > > > >
> > > > > init/main.c:1187 [main]initcall_blacklist =pmf "blacklisting initcall %s\n"
> > > > > init/main.c:1226 [main]initcall_blacklisted =pmf "initcall %s blacklisted\n"
> > > > > init/main.c:1432 [main]run_init_process =pmf "  with arguments:\n"
> > > > > init/main.c:1434 [main]run_init_process =pmf "    %s\n"
> > > > > init/main.c:1435 [main]run_init_process =pmf "  with environment:\n"
> > > > > init/main.c:1437 [main]run_init_process =pmf "    %s\n"
> > > >
> > > >
> > > > Hi Jim,
> > > >
> > > > So if I'm following correctly, this is not a new issue, the 'module'
> > > > name for dynamic debug has always been this way for builtin.
> > >
> > > It is not a new issue - both PM and init-main have been in [main] for some time.
> > >
> > > I believe that with
> > > cfc1d277891e module: Move all into module/
> > >
> > > module's module-name joined them, changing from [module] to [main]
> >
> > If there was a regression due to this, we'd be very interested in
> > hearing about it. Aaron he did the work to move the code to its own directory.
> >
> > > We could do
> > > > something simple and just normalize it when we initially create the
> > > > table, but setting the 'module name' to 'core' or 'builtin' or something
> > > > for all these?
> > >
> > > core and builtin would both lump all those separate modules together,
> > > making it less meaningful.
> > >
> > > having stable names independent of M vs Y config choices is imperative, ISTM.
> > >
> > > Also, I dont think "only builtins are affected" captures the whole problem.
> > > I dont recall amdgpu or other modules changing when built with =y
> > >
> > > Theres some subtlety in how KBUILD_MODNAME is set,
> > > and probably many current users who like its current behavior.
> > > A new var ?
> > >
> > > 1st, I think that anything tristate gets a sensible value,
> > > but at least some of the builtin-only "modules" get basenames, by default.
> >
> > In general we could all benefit from an enhancement for a shortname for
> > things which could be modules being built-in. We're now seeing requests
> > for dynamic debug, but it could also be usefulf for Nick's future work
> > to help userspace tools / tracing map kallsysms to specific modules when
> > built-in.
> 
> 
> 
> I think I rejected it some years ago.

Good to know.

> He comes back again and again with almost the same approaches,
> until he finds a "sponsor" (it's you) who will get it in.

Actually I also rejected the same approach too.

> Recently, I rejected the Kbuild changes again.

Yes I saw.

> > To that end I had suggested the current state of affairs & current difficulty
> > in trying to get us a name for this here:
> >
> > https://lore.kernel.org/all/Y/kXDqW+7d71C4wz@bombadil.infradead.org/
> >
> > I ended up suggesting perhaps we need a -DPOSSIBLE_MODULE then if we
> > could *somehow* pull that off perhaps then we could instead use
> > -DPOSSIBLE_KBUILD_MODNAME which would ensure a consistent symbol when
> > a module is built-in as well.
> >
> > That still leaves the difficulty in trying to gather possible-obj-m as
> > a future challenge.
> 
> I do not understand your point.
> 
> Why is it important to achieve "precisely-exactly-possible-obj-m" instead of
> "perhaps-possible-obj-m"?
> 
> When "modprobe foo" succeeds, the user is sure that the kernel
> provides the feature "foo" (but he does not care if
> "foo" is built-in or modular).

You are thinking about the modprobe situation. That is in no way shape
or form the end goal. Nick suggests he has tooling enhancements which
allow userspace to disambiguate symbols from kallsyms which are built-in
to come things which are modules. His hacks bring back the tristate crap
which both you and I have rejected. The alternative is to live with what
we *do* have. What do *do* have is to rely on the module license tag to
see if something could possibly be a module.

The module license cleanup is motivated by the fact that relying on the
fact that a module license tag does not always mean something can be a
module. Having something which could never be a module still use
MODULE_LICENSE() and have modprobe succeed is not fatal. However, if
future tooling *is* going to be relied upon to display to tracer /
debuggers where symbols come from, it becomes more useful if that
information is a bit more deterministic.

We eventually want to actually see if we can just infer the
MODULE_LICENSE() from the SPDX tag, how we go about that remains
to be seen, but if we *at least* had MODULE_LICENSE only in places
that *were really* modules this reduces the scope.

While we don't *need* to "fix" code which is using MODULE_LICENSE()
which can *never* be modules, removing that cruft *can* still be useful
from a driver maintainer perspective.

> He spams with MODULE_LICENSE removal with no justification.

If we don't want Nick to rely on the old tristate crap then the only
tooling available he has today at his disposal to map symbols to
possible modules is the MODULE_LICENSE tag.

Part of the issue with Nick's patches all along has been proper
justification / documentation. Try to take a step back and think about
the possible *value-add* of userspace having more concrete information
for traces / debugging. Again, value of his patches are separate, but
*if* someone wanted to bring such tooling which *did* want to help map
symbols more closely to possible modules *relying* on module-license is
one of the cheap ways to do it today without incurring a build system
slowdown.

  Luis

Re: RFC - KBUILD_MODNAME is misleading in builtins, as seen in /proc/dynamic_debug/control

[Nick Alcock]

On 21 Mar 2023, Luis Chamberlain spake thusly:

> On Tue, Mar 21, 2023 at 06:03:01PM +0900, Masahiro Yamada wrote:
>> On Tue, Mar 21, 2023 at 6:04 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
>> > In general we could all benefit from an enhancement for a shortname for
>> > things which could be modules being built-in. We're now seeing requests
>> > for dynamic debug, but it could also be usefulf for Nick's future work
>> > to help userspace tools / tracing map kallsysms to specific modules when
>> > built-in.
>> 
>> I think I rejected it some years ago.
>
> Good to know.

Indeed! It was rejected because it used too much space in the kernel at
runtime (it really did, it was like a megabyte, it was ridiculous).

The new approach consumes only about 12K even in a make allyesconfig
kernel. Any realistic kernel has much lower overhead, usually about 4KiB
total and often even less, but once we're below a page I doubt anyone
cares much any more. Load two modules and you've lost more RAM on pure
page-granularity slack than that.

(This is true to the point where I stopped trying to optimize it further
because all the data to determine which module and object file every
symbol comes from, to sufficient accuracy to make it possible for every
symbol to have a unique (name, module, objname) triplet, was becoming
smaller than the size of the code needed to read it!)

>> He comes back again and again with almost the same approaches,
>> until he finds a "sponsor" (it's you) who will get it in.
>
> Actually I also rejected the same approach too.

... and that approach is indeed gone, surviving only in a patch series
serving as a *validator* to ensure that Luis's approach for tracking
built-in modules gives the right result in every case.

The validator patch series hasn't been upstreamed. I've given up on that
because whenever I tried to upstream it people seem to have assumed that
it was incurring costs at build time or in some way being used by the
kallsyms code at runtime. It doesn't, it isn't, it's purely a validator,
it isn't even executed unless you do make tristatecheck. But not
upstreaming it isn't too problematic: even out of tree it still serves
its purpose. It's only a developer tool after all.

>> When "modprobe foo" succeeds, the user is sure that the kernel
>> provides the feature "foo" (but he does not care if
>> "foo" is built-in or modular).
>
> You are thinking about the modprobe situation. That is in no way shape
> or form the end goal. Nick suggests he has tooling enhancements which
> allow userspace to disambiguate symbols from kallsyms which are built-in
> to come things which are modules.

DTrace has had this for about a decade now, using a much older and less
capable version of the kallmodsyms patch series. I'm working on adding
the same sort of thing to perf and ftrace right now. systemtap can
already do it, using hacks that would make your hair turn white:
hopefully it can move to using /proc/kallmodsyms in time :)

>                                   His hacks bring back the tristate crap
> which both you and I have rejected.

Nah, those are gone and will not come back (see above). In hindsight I
actually have a good reason for rejecting them -- they relied on
uppercasing CONFIG_ symbols to a y/n value, but a lot of the makefiles
contain code that relies on those symbols' values being lowercase! So
doing the modules.builtin construction that way leads to wrong results
unless apply rather a large pile of tweaks to ugly hacks in makefiles
across the tree, and the errors are silent (as witness the fact that
nobody had ever fixed it in over a decade). The new approach avoids
those errors and gives results precisely matching the tristate values in
Kconfig, *as long as* .modinfo is present only in things that can be
built as modules. Hence the validator.

(Also the makefile trickery in the old scripts/Makefile.modbuiltin is
just *unreadable*.)

>         Having something which could never be a module still use
> MODULE_LICENSE() and have modprobe succeed is not fatal.

Actually I have since found code in dracut that relies on modprobe
failing only if a module it tries to load genuinely does not exist, and
succeeding if it's built-in.

There is probably more. It seems like something scripts loading modules
would be likely to care about.

>                                                          However, if
> future tooling *is* going to be relied upon to display to tracer /
> debuggers where symbols come from, it becomes more useful if that
> information is a bit more deterministic.

I'd say it's even more valuable to use it for tracers/debuggers to
arrange to only *trace* things that you want to: so this is input side,
deciding what to trace, as much as it is output side in trace/stackwalk
output etc.

Things like locking primitives are often traceable, but there are many
duplicates all with the same names because they're static functions in
headers, and you really don't want to trace all the duplicates, only the
specific instances you're interested in. (If you trace all of them you
likely cause an overwhelming flood of output and possibly crash the
kernel due to tracing some bit of locking that it was critical nobody
ever trace, which was almost certainly not the specific instance you
were concerned with). Function specialization and hot/cold partitioning
makes this even worse.

> Part of the issue with Nick's patches all along has been proper
> justification / documentation. Try to take a step back and think about
> the possible *value-add* of userspace having more concrete information
> for traces / debugging.

... I hope what I wrote above helps a bit?

I'm about to take another look at the cover letter and commit logs for
the whole kallmodsyms side of things and try (again) to improve on them.

Re: RFC - KBUILD_MODNAME is misleading in builtins, as seen in /proc/dynamic_debug/control

[Masahiro Yamada]

On Tue, Mar 21, 2023 at 5:00 AM <jim.cromie@gmail.com> wrote:
>
> On Mon, Mar 20, 2023 at 12:35 PM Jason Baron <jbaron@akamai.com> wrote:
> >
> >
> >
> > On 3/20/23 1:05 AM, jim.cromie@gmail.com wrote:
> > > dynamic-debug METADATA uses KBUILD_MODNAME as:
> > >
> > > #define DEFINE_DYNAMIC_DEBUG_METADATA_CLS(name, cls, fmt)       \
> > >          static struct _ddebug  __aligned(8)                     \
> > >          __section("__dyndbg") name = {                          \
> > >                  .modname = KBUILD_MODNAME,                      \
> > >
> > > This is going amiss for some builtins, ie those enabled here, by:
> > >
> > >      echo module main +pmf > /proc/dynamic_debug_control
> > >      grep =pmf /proc/dynamic_debug/control
> > >
> > > init/main.c:1187 [main]initcall_blacklist =pmf "blacklisting initcall %s\n"
> > > init/main.c:1226 [main]initcall_blacklisted =pmf "initcall %s blacklisted\n"
> > > init/main.c:1432 [main]run_init_process =pmf "  with arguments:\n"
> > > init/main.c:1434 [main]run_init_process =pmf "    %s\n"
> > > init/main.c:1435 [main]run_init_process =pmf "  with environment:\n"
> > > init/main.c:1437 [main]run_init_process =pmf "    %s\n"
> >
> >
> > Hi Jim,
> >
> > So if I'm following correctly, this is not a new issue, the 'module'
> > name for dynamic debug has always been this way for builtin.
>
> It is not a new issue - both PM and init-main have been in [main] for some time.
>
> I believe that with
> cfc1d277891e module: Move all into module/
>
> module's module-name joined them, changing from [module] to [main]

Maybe more.

We have almost 100 'main.c' files.

$ find . -name main.c | wc
     97      97    3473





> We could do
> > something simple and just normalize it when we initially create the
> > table, but setting the 'module name' to 'core' or 'builtin' or something
> > for all these?
>
> core and builtin would both lump all those separate modules together,
> making it less meaningful.
>
> having stable names independent of M vs Y config choices is imperative, ISTM.


I do not understand what you mean.


KBUILD_MODNAME is not affected by the y/m configuration.




If an object is a member of a composite object, which
does not necessarily be a real module, KBUILD_MODNAME
refers to the name of the composite.
Otherwise, the basename of the source file.


Examples:


obj-y += alias-name.o
alias-name-objs := foo.o

  -->  KBUILD_MODNAME is "alias-name"



obj-y += foo.o

  -->  KBUILD_MODNAME is "foo"



This is about how you write Makefile code.
CONFIG options are unrelated.







> Also, I dont think "only builtins are affected" captures the whole problem.
> I dont recall amdgpu or other modules changing when built with =y
>
> Theres some subtlety in how KBUILD_MODNAME is set,
> and probably many current users who like its current behavior.
> A new var ?
>
> 1st, I think that anything tristate gets a sensible value,
> but at least some of the builtin-only "modules" get basenames, by default.
>
> arch/x86/events/amd/ibs.c:1398 [ibs]force_ibs_eilvt_setup =_ "No EILVT
> entry available\n"
> arch/x86/events/intel/pt.c:797 [pt]pt_topa_dump =_ "# table @%p, off
> %llx size %zx\n"w=%16llx\n"
>
> kvm gets a solid name, because tristate ?
>
> arch/x86/kvm/mmu/mmu.c:6661 [kvm]kvm_mmu_invalidate_mmio_sptes =_
> "kvm: kvm [%i]: zapping shadow pages for mmio generation wraparound\n"
> arch/x86/kvm/hyperv.c:1402 [kvm]kvm_hv_set_msr_pw =_ "kvm [%i]: vcpu%i
> hv crash (0x%llx 0x%llx 0x%llx 0x%llx 0x%llx)\n"
>
> kvm-intel and kvm-amd get their names elsewhere.
>
> arch/x86/kvm/vmx/nested.c:207 [kvm_intel]nested_vmx_abort =_
> "kvm_intel: nested vmx abort, indicator %d\n"
> arch/x86/kvm/vmx/nested.c:913 [kvm_intel]nested_vmx_load_msr =_
> "kvm_intel: %s cannot read MSR entry (%u, 0x%08llx)\n"
>
> arch/x86/kvm/svm/avic.c:860 [kvm_amd]get_pi_vcpu_info =_ "SVM: %s: use
> GA mode for irq %u\n"
> arch/x86/kvm/svm/avic.c:889 [kvm_amd]avic_pi_update_irte =_ "SVM: %s:
> host_irq=%#x, guest_irq=%#x, set=%#x\n"
>
> iow, I dont know..
>
> >
> > Thanks,
> >
> > -Jason



-- 
Best Regards
Masahiro Yamada

Re: RFC - KBUILD_MODNAME is misleading in builtins, as seen in /proc/dynamic_debug/control

[jim.cromie]

On Tue, Mar 21, 2023 at 12:45 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Tue, Mar 21, 2023 at 5:00 AM <jim.cromie@gmail.com> wrote:
> >
> > On Mon, Mar 20, 2023 at 12:35 PM Jason Baron <jbaron@akamai.com> wrote:
> > >
> > >
> > >
> > > On 3/20/23 1:05 AM, jim.cromie@gmail.com wrote:
> > > > dynamic-debug METADATA uses KBUILD_MODNAME as:
> > > >
> > > > #define DEFINE_DYNAMIC_DEBUG_METADATA_CLS(name, cls, fmt)       \
> > > >          static struct _ddebug  __aligned(8)                     \
> > > >          __section("__dyndbg") name = {                          \
> > > >                  .modname = KBUILD_MODNAME,                      \
> > > >
> > > > This is going amiss for some builtins, ie those enabled here, by:
> > > >
> > > >      echo module main +pmf > /proc/dynamic_debug_control
> > > >      grep =pmf /proc/dynamic_debug/control
> > > >
> > > > init/main.c:1187 [main]initcall_blacklist =pmf "blacklisting initcall %s\n"
> > > > init/main.c:1226 [main]initcall_blacklisted =pmf "initcall %s blacklisted\n"
> > > > init/main.c:1432 [main]run_init_process =pmf "  with arguments:\n"
> > > > init/main.c:1434 [main]run_init_process =pmf "    %s\n"
> > > > init/main.c:1435 [main]run_init_process =pmf "  with environment:\n"
> > > > init/main.c:1437 [main]run_init_process =pmf "    %s\n"
> > >
> > >
> > > Hi Jim,
> > >
> > > So if I'm following correctly, this is not a new issue, the 'module'
> > > name for dynamic debug has always been this way for builtin.
> >
> > It is not a new issue - both PM and init-main have been in [main] for some time.
> >
> > I believe that with
> > cfc1d277891e module: Move all into module/
> >
> > module's module-name joined them, changing from [module] to [main]
>
> Maybe more.
>
> We have almost 100 'main.c' files.
>
> $ find . -name main.c | wc
>      97      97    3473
>

yes.  I just picked [main] as the example cuz it was the biggest
bucket of unrelateds.

there are other oddities:

"power" module ( subsystem ?) has 2 names matching/picked-by basename

drivers/base/power/main.c:467 [main]dpm_show_time =_ "%s%s%s of
devices %s after %ld.%03ld msecs\n"
drivers/base/power/domain.c:582 [domain]_genpd_power_on =_ "%s:
Power-%s latency exceeded, new value %lld ns\n"

"power" also has 4 other "mod-names", all matching basename

kernel/power/suspend.c:580 [suspend]enter_state =_ "Preparing system
for sleep (%s)\n"
kernel/power/hibernate.c:691 [hibernate]load_image_and_restore =_
"Loading hibernation image.\n"
kernel/power/snapshot.c:1083 [snapshot]mark_nosave_pages =p "Marking
nosave pages: [mem %#010llx-%#010llx]\n"
kernel/power/swap.c:1509 [swap]swsusp_read =p "Error %d resuming\n"

others have distinct [modnames], where/how do they get set ?

drivers/base/firmware_loader/main.c:1442
[firmware_class]device_uncache_fw_images =_ "%s\n"
drivers/media/rc/rc-main.c:230 [rc_core]ir_create_table =_ "Allocated
space for %u keycode entries (%u bytes)\n"



>
>
>
>
> > We could do
> > > something simple and just normalize it when we initially create the
> > > table, but setting the 'module name' to 'core' or 'builtin' or something
> > > for all these?
> >
> > core and builtin would both lump all those separate modules together,
> > making it less meaningful.
> >
> > having stable names independent of M vs Y config choices is imperative, ISTM.
>
>
> I do not understand what you mean.
>

stable names   ===    modprobe foo working whether module is builtin or loadable

>
> KBUILD_MODNAME is not affected by the y/m configuration.
>
>
>
>
> If an object is a member of a composite object, which
> does not necessarily be a real module, KBUILD_MODNAME
> refers to the name of the composite.
> Otherwise, the basename of the source file.
>
>
> Examples:
>
>
> obj-y += alias-name.o
> alias-name-objs := foo.o
>
>   -->  KBUILD_MODNAME is "alias-name"
>
>
>
> obj-y += foo.o
>
>   -->  KBUILD_MODNAME is "foo"
>

taken-from-basename   correctly characterizes that.


>
>
> This is about how you write Makefile code.
> CONFIG options are unrelated.
>

in kernel/power/Makefile, there is:

     obj-$(CONFIG_HIBERNATION) += hibernate.o snapshot.o swap.o

and those 3 objects each get their own [mod-name]

>
>
>
>
>
>
> > Also, I dont think "only builtins are affected" captures the whole problem.
> > I dont recall amdgpu or other modules changing when built with =y
> >
> > Theres some subtlety in how KBUILD_MODNAME is set,
> > and probably many current users who like its current behavior.
> > A new var ?
> >
> > 1st, I think that anything tristate gets a sensible value,
> > but at least some of the builtin-only "modules" get basenames, by default.
> >
> > arch/x86/events/amd/ibs.c:1398 [ibs]force_ibs_eilvt_setup =_ "No EILVT
> > entry available\n"
> > arch/x86/events/intel/pt.c:797 [pt]pt_topa_dump =_ "# table @%p, off
> > %llx size %zx\n"w=%16llx\n"
> >
> > kvm gets a solid name, because tristate ?
> >
> > arch/x86/kvm/mmu/mmu.c:6661 [kvm]kvm_mmu_invalidate_mmio_sptes =_
> > "kvm: kvm [%i]: zapping shadow pages for mmio generation wraparound\n"
> > arch/x86/kvm/hyperv.c:1402 [kvm]kvm_hv_set_msr_pw =_ "kvm [%i]: vcpu%i
> > hv crash (0x%llx 0x%llx 0x%llx 0x%llx 0x%llx)\n"
> >
> > kvm-intel and kvm-amd get their names elsewhere.
> >
> > arch/x86/kvm/vmx/nested.c:207 [kvm_intel]nested_vmx_abort =_
> > "kvm_intel: nested vmx abort, indicator %d\n"
> > arch/x86/kvm/vmx/nested.c:913 [kvm_intel]nested_vmx_load_msr =_
> > "kvm_intel: %s cannot read MSR entry (%u, 0x%08llx)\n"
> >
> > arch/x86/kvm/svm/avic.c:860 [kvm_amd]get_pi_vcpu_info =_ "SVM: %s: use
> > GA mode for irq %u\n"
> > arch/x86/kvm/svm/avic.c:889 [kvm_amd]avic_pi_update_irte =_ "SVM: %s:
> > host_irq=%#x, guest_irq=%#x, set=%#x\n"
> >
> > iow, I dont know..
> >
> > >
> > > Thanks,
> > >
> > > -Jason
>
>
>
> --
> Best Regards
> Masahiro Yamada