Re: [PATCH v8 2/3] modpost: Produce extended MODVERSIONS information

[Matthew Maurer <mmaurer@google.com>]

[-- Attachment #1: Type: text/plain, Size: 3649 bytes --]

On Thu, Nov 7, 2024 at 2:38 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Wed, Nov 06, 2024 at 02:19:38PM -0800, Matthew Maurer wrote:
> > >
> > > > If booted against an old kernel, it will
> > > > behave as though there is no modversions information.
> > >
> > > Huh? This I don't get. If you have the new libkmod and boot
> > > an old kernel, that should just not break becauase well, long
> > > symbols were not ever supported properly anyway, so no regression.
> >
> > Specifically, if you set NO_BASIC_MODVERSIONS, build a module, and
> > then load said module with a kernel *before* EXTENDED_MODVERSIONS
> > existed, it will see no modversion info on the module to check. This
> > will be true regardless of symbol length.
>
> Isn't that just the same as disabling modverisons?
>
> If you select modversions, you get the options to choose:
>
>   - old modversions
>   - old modversions + extended modversions
>   - extended modversions only

Yes, what I'm pointing out is that kernels before the introduction of
extended modversions will not know how to read extended modversions,
and so they will treat modules with *only* extended modversions as
though they have no modversions.

>
> > > I'm not quite sure I understood your last comment here though,
> > > can you clarify what you meant?
> > >
> > > Anyway, so now that this is all cleared up, the next question I have
> > > is, let's compare a NO_BASIC_MODVERSIONS world now, given that the
> > > userspace requirements aren't large at all, what actual benefits does
> > > using this new extended mod versions have? Why wouldn't a distro end
> > > up preferring this for say a future release for all modules?
> >
> > I think a distro will end up preferring using this for all modules,
> > but was intending to put both in for a transitional period until the
> > new format was more accepted.
>
> The only thing left I think to test is the impact at runtime, and the
> only thing I can think of is first we use find_symbol() on resolve_symbol()
> which it took me a while to review and realize that this just uses a
> completely different ELF section, the the ksymtab sections which are split up
> between the old and the gpl section. But after that we use check_version().
> I suspect the major overhead here is in find_symbol() and that's in no way shape
> or form affected by your changes, and I also suspect that since the
> way you implemented for_each_modversion_info_ext() is just *one* search
> there shouldn't be any penalty here at all. Given it took *me* a while
> to review all this, I think it would be good for you to also expand your
> cover letter to be crystal clear on these expectations to users and
> developers and if anything expand on the Kconfig / and add documentation
> if we don't document any of this.

I can add a commit extending modules.rst, but it's not clear to me
what piece was surprising here - the existing MODVERSIONS format is
*also* in a separate section. Nothing written in the "Module
Versioning" section has been invalidated that I can see.

Things I could think to add:

* Summary of the internal data format (seems odd, since the previous
one isn't here, and I'd think that an implementation detail anyways)
* A warning about the effects of NO_BASIC_MODVERSIONS (probably better
in Kconfig, isn't in the current changeset because the flag isn't
there)

>
> I'd still like to see you guys test all this with the new TEST_KALLSYMS.

I've attached the results of running TEST_KALLSYMS - it appears to be
irrelevant to performance, as you expected.

>
>   Luis

[-- Attachment #2: extended-log --]
[-- Type: application/octet-stream, Size: 1306 bytes --]

TAP version 13
1..1
# timeout set to 45
# selftests: module: find_symbol.sh
#
#  Performance counter stats for '/sbin/modprobe test_kallsyms_b':
#
#           28898032 ns   duration_time
#            2366000 ns   user_time
#                207      page-faults
#
#        0.020085991 seconds time elapsed
#
#        0.002366000 seconds user
#        0.000000000 seconds sys
#
#
#
#  Performance counter stats for '/sbin/modprobe test_kallsyms_b':
#
#           25333640 ns   duration_time
#              32000 ns   user_time
#            2357000 ns   system_time
#                207      page-faults
#
#        0.024083957 seconds time elapsed
#
#        0.000032000 seconds user
#        0.002357000 seconds sys
#
#
#
#  Performance counter stats for '/sbin/modprobe test_kallsyms_b':
#
#           25398600 ns   duration_time
#              30000 ns   user_time
#            2269000 ns   system_time
#                208      page-faults
#
#        0.024095204 seconds time elapsed
#
#        0.000030000 seconds user
#        0.002269000 seconds sys
#
#
ok 1 selftests: module: find_symbol.sh
/selftests # zcat /proc/config.gz | grep EXTENDED
# CONFIG_X86_EXTENDED_PLATFORM is not set
CONFIG_EXTENDED_MODVERSIONS=y
# CONFIG_NETCONSOLE_EXTENDED_LOG is not set
CONFIG_SERIAL_8250_EXTENDED=y
/selftests #

[-- Attachment #3: noextend-log --]
[-- Type: application/octet-stream, Size: 1316 bytes --]

TAP version 13
1..1
# timeout set to 45
# selftests: module: find_symbol.sh
#
#  Performance counter stats for '/sbin/modprobe test_kallsyms_b':
#
#           30477348 ns   duration_time
#            2494000 ns   user_time
#                206      page-faults
#
#        0.024078527 seconds time elapsed
#
#        0.002494000 seconds user
#        0.000000000 seconds sys
#
#
#
#  Performance counter stats for '/sbin/modprobe test_kallsyms_b':
#
#           25264385 ns   duration_time
#              34000 ns   user_time
#            2351000 ns   system_time
#                207      page-faults
#
#        0.024081980 seconds time elapsed
#
#        0.000034000 seconds user
#        0.002351000 seconds sys
#
#
#
#  Performance counter stats for '/sbin/modprobe test_kallsyms_b':
#
#           25300644 ns   duration_time
#              33000 ns   user_time
#            2307000 ns   system_time
#                207      page-faults
#
#        0.024109409 seconds time elapsed
#
#        0.000033000 seconds user
#        0.002307000 seconds sys
#
#
ok 1 selftests: module: find_symbol.sh
/selftests # zcat /proc/config.gz | grep EXTEND
# CONFIG_X86_EXTENDED_PLATFORM is not set
# CONFIG_EXTENDED_MODVERSIONS is not set
# CONFIG_NETCONSOLE_EXTENDED_LOG is not set
CONFIG_SERIAL_8250_EXTENDED=y
/selftests #