diff mbox series

[07/20] kernel: copy all built kernel modules to root filesystem image

Message ID ee3fb185b494b4de05ca65725e0a96cffa307acb.1700010291.git.ehem+openwrt@m5p.com
State Not Applicable
Delegated to: Petr Štetiar
Headers show
Series Kernel build fixups, split object directory off (partial WIP) | expand

Commit Message

Elliott Mitchell Nov. 11, 2023, 12:21 a.m. UTC
This removes the requirement for to create a package for all modules.
Now devices can simply specify in-tree drivers/other to be built as
modules and they will be present in the resultant image.

Signed-off-by: Elliott Mitchell <ehem+openwrt@m5p.com>
---
I'm pretty sure this is the right general approach.  Why build
modules and simply dump them in the garbage.  I have an odd suspicion
a particular sort of adjustment will be requested.  Issue there is
the OpenWRT use of Make is quite something, I'm unsure about which
timestamp files to use.
---
 include/kernel-defaults.mk | 2 ++
 include/kernel.mk          | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

Comments

Felix Fietkau Nov. 17, 2023, 4:20 p.m. UTC | #1
On 11.11.23 01:21, Elliott Mitchell wrote:
> This removes the requirement for to create a package for all modules.
> Now devices can simply specify in-tree drivers/other to be built as
> modules and they will be present in the resultant image.
> 
> Signed-off-by: Elliott Mitchell <ehem+openwrt@m5p.com>

It seems to me that this completely ignores the use case of having 
release builds that ship a lot of kernel modules as installable 
packages. Did I misread something?
If not, this gets a strong NACK from me.

- Felix
Elliott Mitchell Nov. 17, 2023, 9:31 p.m. UTC | #2
On Fri, Nov 17, 2023 at 05:20:33PM +0100, Felix Fietkau wrote:
> On 11.11.23 01:21, Elliott Mitchell wrote:
> > This removes the requirement for to create a package for all modules.
> > Now devices can simply specify in-tree drivers/other to be built as
> > modules and they will be present in the resultant image.
> > 
> > Signed-off-by: Elliott Mitchell <ehem+openwrt@m5p.com>
> 
> It seems to me that this completely ignores the use case of having 
> release builds that ship a lot of kernel modules as installable 
> packages. Did I misread something?
> If not, this gets a strong NACK from me.

Should be completely orthogonal, though it could have bugs.

Using `cp -l` has two valuable effects:  First, it reduces storage space
usage.  Second, it serves to mark module files as belonging to a package.

My goal is previously setting a kernel option to "m" in a configuration
file, but not having a package causes it to be built, then ignored.  I
want this to do something sensible, not simply waste electricity
building a module and then tossing it in the garbage.

Hmm, come to think of it, that should be $(XARGS) (fix on commit?).
Felix Fietkau Nov. 18, 2023, 6:57 a.m. UTC | #3
On 17.11.23 22:31, Elliott Mitchell wrote:
> On Fri, Nov 17, 2023 at 05:20:33PM +0100, Felix Fietkau wrote:
>> On 11.11.23 01:21, Elliott Mitchell wrote:
>> > This removes the requirement for to create a package for all modules.
>> > Now devices can simply specify in-tree drivers/other to be built as
>> > modules and they will be present in the resultant image.
>> > 
>> > Signed-off-by: Elliott Mitchell <ehem+openwrt@m5p.com>
>> 
>> It seems to me that this completely ignores the use case of having 
>> release builds that ship a lot of kernel modules as installable 
>> packages. Did I misread something?
>> If not, this gets a strong NACK from me.
> 
> Should be completely orthogonal, though it could have bugs.
> 
> Using `cp -l` has two valuable effects:  First, it reduces storage space
> usage.  Second, it serves to mark module files as belonging to a package.
> 
> My goal is previously setting a kernel option to "m" in a configuration
> file, but not having a package causes it to be built, then ignored.  I
> want this to do something sensible, not simply waste electricity
> building a module and then tossing it in the garbage.
> 
> Hmm, come to think of it, that should be $(XARGS) (fix on commit?).

Thanks for the explanation, it makes more sense to me now.
That said, I see a few pitfalls here:

1. If you select kernel modules that depend on other modules selected 
via kmod packages, you end up with non-functional modules with missing 
dependencies in the rootfs.
2. If the kmod package selection accidentally ends up selecting extra 
modules that aren't stored in the package, you end up with rootfs bloat.

I'm fine with the cp -l change, but I think adding all remaining modules 
to the rootfs is not something we should do by default (maybe opt-in?)

By the way, I still get bounces when replying to you, because your mail 
server is refusing my mail.

- Felix
Elliott Mitchell Nov. 20, 2023, 1:36 a.m. UTC | #4
On Sat, Nov 18, 2023 at 07:57:35AM +0100, Felix Fietkau wrote:
> On 17.11.23 22:31, Elliott Mitchell wrote:
> > On Fri, Nov 17, 2023 at 05:20:33PM +0100, Felix Fietkau wrote:
> >> On 11.11.23 01:21, Elliott Mitchell wrote:
> >> > This removes the requirement for to create a package for all modules.
> >> > Now devices can simply specify in-tree drivers/other to be built as
> >> > modules and they will be present in the resultant image.
> >> > 
> >> > Signed-off-by: Elliott Mitchell <ehem+openwrt@m5p.com>
> >> 
> >> It seems to me that this completely ignores the use case of having 
> >> release builds that ship a lot of kernel modules as installable 
> >> packages. Did I misread something?
> >> If not, this gets a strong NACK from me.
> > 
> > Should be completely orthogonal, though it could have bugs.
> > 
> > Using `cp -l` has two valuable effects:  First, it reduces storage space
> > usage.  Second, it serves to mark module files as belonging to a package.
> > 
> > My goal is previously setting a kernel option to "m" in a configuration
> > file, but not having a package causes it to be built, then ignored.  I
> > want this to do something sensible, not simply waste electricity
> > building a module and then tossing it in the garbage.
> > 
> > Hmm, come to think of it, that should be $(XARGS) (fix on commit?).
> 
> Thanks for the explanation, it makes more sense to me now.
> That said, I see a few pitfalls here:
> 
> 1. If you select kernel modules that depend on other modules selected 
> via kmod packages, you end up with non-functional modules with missing 
> dependencies in the rootfs.

Is this actually that much of a problem?  From what I've seen most kmod
packages handled during image build get preinstalled onto the root fs
image.  As such these would nominally function as long as the packages
weren't removed.

Wouldn't this also indicate breakage in the module package anyway?

> 2. If the kmod package selection accidentally ends up selecting extra 
> modules that aren't stored in the package, you end up with rootfs bloat.

Eww, that would be gross.  As with the above, wouldn't this be indicating
breakage in the module packaging anyway?

> I'm fine with the cp -l change, but I think adding all remaining modules 
> to the rootfs is not something we should do by default (maybe opt-in?)

Perhaps.  This could also be handled by the approach the series as a
whole is aiming for.  If kernel module building used a separate object
directory from the kernel build, then modules selected in the device
configuration could be isolated.

> By the way, I still get bounces when replying to you, because your mail 
> server is refusing my mail.

Ick.  Welcome to the world of 2023 and what spam has done to e-mail.
Alas, this is due to extreme measures being taken by the person who
handles this system.  We're at a point where small mailservers cannot
exist due to the shear amount of spam being generated.

I suppose OpenWRT might help by having a default rule to filter out port
25.  There is also that yet to be written kernel patch which disables
the interfering 2.45GHz channels.  There is also the default egress
filtering...   All those things I could get done if I ruled the world.
Jonas Gorski Nov. 24, 2023, 12:53 p.m. UTC | #5
Hi,

On Mon, 20 Nov 2023 at 02:37, Elliott Mitchell <ehem+openwrt@m5p.com> wrote:
>
> On Sat, Nov 18, 2023 at 07:57:35AM +0100, Felix Fietkau wrote:
> > On 17.11.23 22:31, Elliott Mitchell wrote:
> > > On Fri, Nov 17, 2023 at 05:20:33PM +0100, Felix Fietkau wrote:
> > >> On 11.11.23 01:21, Elliott Mitchell wrote:
> > >> > This removes the requirement for to create a package for all modules.
> > >> > Now devices can simply specify in-tree drivers/other to be built as
> > >> > modules and they will be present in the resultant image.
> > >> >
> > >> > Signed-off-by: Elliott Mitchell <ehem+openwrt@m5p.com>
> > >>
> > >> It seems to me that this completely ignores the use case of having
> > >> release builds that ship a lot of kernel modules as installable
> > >> packages. Did I misread something?
> > >> If not, this gets a strong NACK from me.
> > >
> > > Should be completely orthogonal, though it could have bugs.
> > >
> > > Using `cp -l` has two valuable effects:  First, it reduces storage space
> > > usage.  Second, it serves to mark module files as belonging to a package.
> > >
> > > My goal is previously setting a kernel option to "m" in a configuration
> > > file, but not having a package causes it to be built, then ignored.  I
> > > want this to do something sensible, not simply waste electricity
> > > building a module and then tossing it in the garbage.
> > >
> > > Hmm, come to think of it, that should be $(XARGS) (fix on commit?).
> >
> > Thanks for the explanation, it makes more sense to me now.
> > That said, I see a few pitfalls here:
> >
> > 1. If you select kernel modules that depend on other modules selected
> > via kmod packages, you end up with non-functional modules with missing
> > dependencies in the rootfs.
>
> Is this actually that much of a problem?  From what I've seen most kmod
> packages handled during image build get preinstalled onto the root fs
> image.  As such these would nominally function as long as the packages
> weren't removed.

If you do a build with ALL_KMODS=y because you don't know which kmods
you may need later on, and want to avoid having to reflash in this
case, there will be plenty of kmod packages build but not installed
into the rootfs.

>
> Wouldn't this also indicate breakage in the module package anyway?

No, not necessarily. Let's say there is a kmod-foo that packages FOO
(foo.ko). This is selected as =m.

Then you run kernel_*config and select BAR=m (bar.ko), since there is
no kmod defined for it. bar.ko has a dependency on foo.ko

With your patch (AFAIU) the bar.ko would be then installed in the
rootfs, but since foo.ko is packaged separately as a m-package, it
won't be in the rootfs => bar.ko is missing its dependencies in the
rootfs.

> > 2. If the kmod package selection accidentally ends up selecting extra
> > modules that aren't stored in the package, you end up with rootfs bloat.
>
> Eww, that would be gross.  As with the above, wouldn't this be indicating
> breakage in the module packaging anyway?

Maybe, but sometimes modules/drivers in the kernel default to =m for
silly reasons. And again as the above example, if the triggering kmod
package is selected as =m, then the additional modules land in the
rootfs without their dependencies.

> > I'm fine with the cp -l change, but I think adding all remaining modules
> > to the rootfs is not something we should do by default (maybe opt-in?)
>
> Perhaps.  This could also be handled by the approach the series as a
> whole is aiming for.  If kernel module building used a separate object
> directory from the kernel build, then modules selected in the device
> configuration could be isolated.

Maybe instead of putting them into the rootfs, we could wrap them in a
special package? Then you can select it if you want to include them in
your image or not. No idea what to name it, kmod-remaining?
kmod-unaccounted-for? kmod-not-appearing-in-the-definitions?

We could also try to wrap any unexpected .kos into autogenerated kmod
packages based in their names (e.g. if we find a foo.ko, we
autogenerate a kmod-foo.ipk for it), but these wouldn't be selectable
then in menuconfig. Also not sure how well the build system would
handle dynamic package generation.

Also going the other way around, maybe the build system should
warn/complain about any .ko found that isn't wrapped in a kmod-*
package.

WARNING: unpackaged kernel module found: foo/bar.ko

etc.

Regards,
Jonas
Elliott Mitchell Nov. 25, 2023, 2:28 a.m. UTC | #6
On Fri, Nov 24, 2023 at 01:53:01PM +0100, Jonas Gorski wrote:
> 
> On Mon, 20 Nov 2023 at 02:37, Elliott Mitchell <ehem+openwrt@m5p.com> wrote:
> >
> > On Sat, Nov 18, 2023 at 07:57:35AM +0100, Felix Fietkau wrote:
> > > On 17.11.23 22:31, Elliott Mitchell wrote:
> > > > On Fri, Nov 17, 2023 at 05:20:33PM +0100, Felix Fietkau wrote:
> > > >> On 11.11.23 01:21, Elliott Mitchell wrote:
> > > >> > This removes the requirement for to create a package for all modules.
> > > >> > Now devices can simply specify in-tree drivers/other to be built as
> > > >> > modules and they will be present in the resultant image.
> > > >> >
> > > >> > Signed-off-by: Elliott Mitchell <ehem+openwrt@m5p.com>
> > > >>
> > > >> It seems to me that this completely ignores the use case of having
> > > >> release builds that ship a lot of kernel modules as installable
> > > >> packages. Did I misread something?
> > > >> If not, this gets a strong NACK from me.
> > > >
> > > > Should be completely orthogonal, though it could have bugs.
> > > >
> > > > Using `cp -l` has two valuable effects:  First, it reduces storage space
> > > > usage.  Second, it serves to mark module files as belonging to a package.
> > > >
> > > > My goal is previously setting a kernel option to "m" in a configuration
> > > > file, but not having a package causes it to be built, then ignored.  I
> > > > want this to do something sensible, not simply waste electricity
> > > > building a module and then tossing it in the garbage.
> > > >
> > > > Hmm, come to think of it, that should be $(XARGS) (fix on commit?).
> > >
> > > Thanks for the explanation, it makes more sense to me now.
> > > That said, I see a few pitfalls here:
> > >
> > > 1. If you select kernel modules that depend on other modules selected
> > > via kmod packages, you end up with non-functional modules with missing
> > > dependencies in the rootfs.
> >
> > Is this actually that much of a problem?  From what I've seen most kmod
> > packages handled during image build get preinstalled onto the root fs
> > image.  As such these would nominally function as long as the packages
> > weren't removed.
> 
> If you do a build with ALL_KMODS=y because you don't know which kmods
> you may need later on, and want to avoid having to reflash in this
> case, there will be plenty of kmod packages build but not installed
> into the rootfs.
> 
> >
> > Wouldn't this also indicate breakage in the module package anyway?
> 
> No, not necessarily. Let's say there is a kmod-foo that packages FOO
> (foo.ko). This is selected as =m.
> 
> Then you run kernel_*config and select BAR=m (bar.ko), since there is
> no kmod defined for it. bar.ko has a dependency on foo.ko
> 
> With your patch (AFAIU) the bar.ko would be then installed in the
> rootfs, but since foo.ko is packaged separately as a m-package, it
> won't be in the rootfs => bar.ko is missing its dependencies in the
> rootfs.

Yet isn't this breakage in the module package?  It has stolen a module
away from the kernel configuration.

Certainly this is a problem and I've got a rough idea how to solve it.
(simply take advantage of what the patch series is aiming for)

> > > 2. If the kmod package selection accidentally ends up selecting extra
> > > modules that aren't stored in the package, you end up with rootfs bloat.
> >
> > Eww, that would be gross.  As with the above, wouldn't this be indicating
> > breakage in the module packaging anyway?
> 
> Maybe, but sometimes modules/drivers in the kernel default to =m for
> silly reasons. And again as the above example, if the triggering kmod
> package is selected as =m, then the additional modules land in the
> rootfs without their dependencies.

The inverse is also a problem.  If the kernel selected CONFIG_USB=m,
then kmod-chaoskey was built, it would be impossible to load the module
since usbcore.ko would be unavailable.

Both directions currently have breakage.

> > > I'm fine with the cp -l change, but I think adding all remaining modules
> > > to the rootfs is not something we should do by default (maybe opt-in?)
> >
> > Perhaps.  This could also be handled by the approach the series as a
> > whole is aiming for.  If kernel module building used a separate object
> > directory from the kernel build, then modules selected in the device
> > configuration could be isolated.
> 
> Maybe instead of putting them into the rootfs, we could wrap them in a
> special package? Then you can select it if you want to include them in
> your image or not. No idea what to name it, kmod-remaining?
> kmod-unaccounted-for? kmod-not-appearing-in-the-definitions?
> 
> We could also try to wrap any unexpected .kos into autogenerated kmod
> packages based in their names (e.g. if we find a foo.ko, we
> autogenerate a kmod-foo.ipk for it), but these wouldn't be selectable
> then in menuconfig. Also not sure how well the build system would
> handle dynamic package generation.
> 
> Also going the other way around, maybe the build system should
> warn/complain about any .ko found that isn't wrapped in a kmod-*
> package.

All of these are plausible.  I think modules selected in device
configurations should get built and installed into the root filesystem.
Otherwise setting a device kernel configuration option to =m is broken.

Whereas forcing the explicit creation of packages for each and every
kernel module forces duplication of Kconfig functionality.
Felix Fietkau Nov. 25, 2023, 6:08 a.m. UTC | #7
On 25.11.23 03:28, Elliott Mitchell wrote:
> On Fri, Nov 24, 2023 at 01:53:01PM +0100, Jonas Gorski wrote:
>> > > I'm fine with the cp -l change, but I think adding all remaining modules
>> > > to the rootfs is not something we should do by default (maybe opt-in?)
>> >
>> > Perhaps.  This could also be handled by the approach the series as a
>> > whole is aiming for.  If kernel module building used a separate object
>> > directory from the kernel build, then modules selected in the device
>> > configuration could be isolated.
>> 
>> Maybe instead of putting them into the rootfs, we could wrap them in a
>> special package? Then you can select it if you want to include them in
>> your image or not. No idea what to name it, kmod-remaining?
>> kmod-unaccounted-for? kmod-not-appearing-in-the-definitions?
>> 
>> We could also try to wrap any unexpected .kos into autogenerated kmod
>> packages based in their names (e.g. if we find a foo.ko, we
>> autogenerate a kmod-foo.ipk for it), but these wouldn't be selectable
>> then in menuconfig. Also not sure how well the build system would
>> handle dynamic package generation.
>> 
>> Also going the other way around, maybe the build system should
>> warn/complain about any .ko found that isn't wrapped in a kmod-*
>> package.
> 
> All of these are plausible.  I think modules selected in device
> configurations should get built and installed into the root filesystem.
> Otherwise setting a device kernel configuration option to =m is broken.
> 
> Whereas forcing the explicit creation of packages for each and every
> kernel module forces duplication of Kconfig functionality.
The duplication you wish to avoid is there for a reason. The way the 
kernel build system is set up, it makes it easy to build a highly 
customized build for one target, or make a distribution build with 
pretty much everything built as module. What it offers no solution for 
is to maintain and keep in sync kernel configurations for a wide array 
of different (often storage constrained) targets, while keeping a lot of 
extra features/drivers optionally installable.
That's exactly what our packaging around kernel modules + our kernel 
config handling scripts were made for.

Since there is no perfect solution, there are always some trade-offs 
involved. One of these trade-offs was our choice to not support adding 
kernel modules in the device kernel config by selecting them as =m 
there. I didn't consider that feature useful enough to justify the cost 
of dealing with all the corner cases.

I still don't fully understand your motivation for adding this feature. 
Is it just because you're bothered by having to write some extra 
boilerplate code for adding kernel modules? Are there some cases where 
the existing system cannot work for what you're trying to do?
Or is there some other reason why you need this?

Just to be clear, I'm not opposed to the feature you're proposing in any 
way. What I want to avoid is adding something that works for your 
special case but quickly breaks in weird ways when other people start 
using it.

- Felix
Elliott Mitchell Nov. 25, 2023, 5:36 p.m. UTC | #8
On Sat, Nov 25, 2023 at 07:08:20AM +0100, Felix Fietkau wrote:
> On 25.11.23 03:28, Elliott Mitchell wrote:
> > On Fri, Nov 24, 2023 at 01:53:01PM +0100, Jonas Gorski wrote:
> >> > > I'm fine with the cp -l change, but I think adding all remaining modules
> >> > > to the rootfs is not something we should do by default (maybe opt-in?)
> >> >
> >> > Perhaps.  This could also be handled by the approach the series as a
> >> > whole is aiming for.  If kernel module building used a separate object
> >> > directory from the kernel build, then modules selected in the device
> >> > configuration could be isolated.
> >> 
> >> Maybe instead of putting them into the rootfs, we could wrap them in a
> >> special package? Then you can select it if you want to include them in
> >> your image or not. No idea what to name it, kmod-remaining?
> >> kmod-unaccounted-for? kmod-not-appearing-in-the-definitions?
> >> 
> >> We could also try to wrap any unexpected .kos into autogenerated kmod
> >> packages based in their names (e.g. if we find a foo.ko, we
> >> autogenerate a kmod-foo.ipk for it), but these wouldn't be selectable
> >> then in menuconfig. Also not sure how well the build system would
> >> handle dynamic package generation.
> >> 
> >> Also going the other way around, maybe the build system should
> >> warn/complain about any .ko found that isn't wrapped in a kmod-*
> >> package.
> > 
> > All of these are plausible.  I think modules selected in device
> > configurations should get built and installed into the root filesystem.
> > Otherwise setting a device kernel configuration option to =m is broken.
> > 
> > Whereas forcing the explicit creation of packages for each and every
> > kernel module forces duplication of Kconfig functionality.

> The duplication you wish to avoid is there for a reason. The way the 
> kernel build system is set up, it makes it easy to build a highly 
> customized build for one target, or make a distribution build with 
> pretty much everything built as module. What it offers no solution for 
> is to maintain and keep in sync kernel configurations for a wide array 
> of different (often storage constrained) targets, while keeping a lot of 
> extra features/drivers optionally installable.
> That's exactly what our packaging around kernel modules + our kernel 
> config handling scripts were made for.
> 
> Since there is no perfect solution, there are always some trade-offs 
> involved. One of these trade-offs was our choice to not support adding 
> kernel modules in the device kernel config by selecting them as =m 
> there. I didn't consider that feature useful enough to justify the cost 
> of dealing with all the corner cases.
> 
> I still don't fully understand your motivation for adding this feature. 
> Is it just because you're bothered by having to write some extra 
> boilerplate code for adding kernel modules? Are there some cases where 
> the existing system cannot work for what you're trying to do?
> Or is there some other reason why you need this?

Mostly I strongly dislike setting =m wasting build time and having near-
zero effect on the resultant output.  In particular I see a
build/configuration targetting VMs to likely want to build/include many
modules, yet many of those not really deserving their own packages (since
many would be unneeded in many cases).

> Just to be clear, I'm not opposed to the feature you're proposing in any 
> way. What I want to avoid is adding something that works for your 
> special case but quickly breaks in weird ways when other people start 
> using it.

That is quite reasonable.  I've pushed this later in my queue since I
concluded the aim of the later patches will make a more acceptable
approach possible.

Hopefully I'll be able to resend the series in a few days, just need
some build testing to ensure I didn't break anything.  The first two
patches are going to remain unchanged as there simply isn't anything
which can be done to them.
Elliott Mitchell Dec. 14, 2023, 4:09 a.m. UTC | #9
On Sat, Nov 25, 2023 at 09:36:33AM -0800, Elliott Mitchell wrote:
> On Sat, Nov 25, 2023 at 07:08:20AM +0100, Felix Fietkau wrote:
> > On 25.11.23 03:28, Elliott Mitchell wrote:
> > 
> > Since there is no perfect solution, there are always some trade-offs 
> > involved. One of these trade-offs was our choice to not support adding 
> > kernel modules in the device kernel config by selecting them as =m 
> > there. I didn't consider that feature useful enough to justify the cost 
> > of dealing with all the corner cases.

I should also add, I rather dislike package building modifying kernel
configuration.  Due to this you can't be sure the kernel specified by
configuration is the one you're actually building.

> > Just to be clear, I'm not opposed to the feature you're proposing in any 
> > way. What I want to avoid is adding something that works for your 
> > special case but quickly breaks in weird ways when other people start 
> > using it.
> 
> That is quite reasonable.  I've pushed this later in my queue since I
> concluded the aim of the later patches will make a more acceptable
> approach possible.
> 
> Hopefully I'll be able to resend the series in a few days, just need
> some build testing to ensure I didn't break anything.  The first two
> patches are going to remain unchanged as there simply isn't anything
> which can be done to them.

Famous last words.  Took rather longer than expected since I went fairly
far with an approach which towards the end became clear would not be
accepted.

Then I tried an alternative and ran into a major weakness of Kbuild.
Hopefully I've brought the issue to the attention of the right person
and it will be fixed in the kernel soon.  Alas I've no idea how long
that will take to fix.  In the mean time I've got a suboptimal
workaround.

I seem to have about 3 patch series going.  First patch series is a VM
oriented build (smallest kernel to boot in multiple VMs).  Second patch
series is trying to split kernel source/build/module directories apart
and implementing better handling of modules.  Third series is misc build
system cleanup which I've run into while fighting these issues.

First two have been partially sent, their later stages are still in
flux.  Would be helpful to start getting some of the earlier patches
committed.

I need a lot of reviewer and committer time...
Elliott Mitchell Jan. 1, 2024, 9:42 p.m. UTC | #10
On Sat, Nov 25, 2023 at 07:08:20AM +0100, Felix Fietkau wrote:
> On 25.11.23 03:28, Elliott Mitchell wrote:
> > 
> > All of these are plausible.  I think modules selected in device
> > configurations should get built and installed into the root filesystem.
> > Otherwise setting a device kernel configuration option to =m is broken.
> > 
> > Whereas forcing the explicit creation of packages for each and every
> > kernel module forces duplication of Kconfig functionality.

> The duplication you wish to avoid is there for a reason. The way the 
> kernel build system is set up, it makes it easy to build a highly 
> customized build for one target, or make a distribution build with 
> pretty much everything built as module. What it offers no solution for 
> is to maintain and keep in sync kernel configurations for a wide array 
> of different (often storage constrained) targets, while keeping a lot of 
> extra features/drivers optionally installable.
> That's exactly what our packaging around kernel modules + our kernel 
> config handling scripts were made for.

What needs to catch the attention of the core of the Linux kernel
developers is Linux is no longer a niche OS.  1.5GB of *source* and
producing 400MB of modules.

Providing individual descriptions of each and every module is really a
task for the kernel developers.  The descriptions in Kconfig files don't
readily convert to individually packaged modules.  I hope other Linux
distributions are also concerned about how large the package for all
built modules has become.


> Since there is no perfect solution, there are always some trade-offs 
> involved. One of these trade-offs was our choice to not support adding 
> kernel modules in the device kernel config by selecting them as =m 
> there. I didn't consider that feature useful enough to justify the cost 
> of dealing with all the corner cases.
> 
> I still don't fully understand your motivation for adding this feature. 
> Is it just because you're bothered by having to write some extra 
> boilerplate code for adding kernel modules? Are there some cases where 
> the existing system cannot work for what you're trying to do?
> Or is there some other reason why you need this?
> 
> Just to be clear, I'm not opposed to the feature you're proposing in any 
> way. What I want to avoid is adding something that works for your 
> special case but quickly breaks in weird ways when other people start 
> using it.

I've already stated a concern.  Having things configured =m in device
kernel configurations simply getting dropped is wrong.

The kernel configurations for bmips/bcm6318 and bmips/bcm6328 set
CONFIG_HW_RANDOM=n.  Turns out despite this, KernelPackage/b43 overrides
the value and those device kernels actually have CONFIG_HW_RANDOM=y.
This is a very unpleasant surprise if you're trying work with one of
these devices.

x86 really demonstrates the need for better module handling.  Every
single Intel pinctrl driver is present in the kernel.  If you need the
functionality then this is valuable.  Alas there are >15 drivers you
don't need.  Further, these would work well as modules since they could
readily be loaded from /.  On top of this, what about someone whose
system has the feature, but doesn't want to use it?



I've got what seems a workable approach, but it isn't yet finished.
While there are some differences between building an in-tree module
versus building an out-of-tree module, the differences are quite small.

I've presently got a PoC of building in-tree modules using the out of
tree system.  This present method I've got is roughly:
`make -C <kbuild_dir> M=<module directory> CONFIG_<SOME-MODULE>=m <module>.ko`

Where <kbuild_dir> is a copy of the Kbuild system.  The list of files
can be extracted from `scripts/package/build<foo>`.  This really needs
a "kbuild_install" so those can share it, and various other uses can
come from it.

Presently I hope the approach of simply specifying all the package
CONFIG_* values on the Make command-line will work.  Alas what may
instead be needed is to generate a variant of the .config file.


Right now I'm seeing two issues.  First, this ends up bypassing the
kernel handling of dependencies.  Due to the impressive nature of
OpenWRT's Makefiles (I'm no slouch, but these are impressive) I'm still
trying to figure out how targets are named for dependency use.
Specifically, what dependency is needed to cause Make to try building
the "compile" target for "package/kernel/linux" before proceeding?
(external modules can depend on an in-tree ones)

Second, this ends up needing slightly different handling between in-tree
modules and out-of-tree.  In particular there is a need to specify each
.ko on Kbuild/Make's command-line.  An ideal differentiator is whether
the package sets KCONFIG.

The result is presently rather more sensitive to package setup and
several OpenWRT packages are less than wonderful.  KernelPackage/b43 had
been abusing KCONFIG, resulting in confusion and would break this.
KernelPackage/fs-nfs-v3 was relying on CONFIG_NFS_V3 to defaulting to
module and the CI system was noticing.
Several the lantiq modules are also violating this and presently I've
got no idea what is going on.  This needs fixing, but I'm presently
unsure of how to fix.


Felix Fietkau, we were having e-mail fun.  In theory this got taken care
of, but lacking confirmation of resolution I'm unsure.  Welcome to the
present day and what spam has done to e-mail.  Interestingly if OpenWRT
had a default to reject all packats to port 25 that might have a
noticable impact.
diff mbox series

Patch

diff --git a/include/kernel-defaults.mk b/include/kernel-defaults.mk
index 05ec9afe09..0b421d2cc3 100644
--- a/include/kernel-defaults.mk
+++ b/include/kernel-defaults.mk
@@ -151,6 +151,8 @@  define Kernel/CompileImage/Default
 	rm -f $(TARGET_DIR)/init
 	+$(KERNEL_MAKE) $(KERNEL_MAKEOPTS_IMAGE) $(if $(KERNELNAME),$(KERNELNAME),all)
 	$(call Kernel/CopyImage)
+	+$(KERNEL_MAKE) $(KERNEL_MAKEOPTS_IMAGE) modules
+	find $(LINUX_DIR) -name \*.ko -a -links 1 -print0 | xargs -0 -r cp -lf -t $(TARGET_DIR)/lib/modules/$(LINUX_VERSION)
 endef
 
 ifneq ($(CONFIG_TARGET_ROOTFS_INITRAMFS),)
diff --git a/include/kernel.mk b/include/kernel.mk
index 8236416132..82d87e8491 100644
--- a/include/kernel.mk
+++ b/include/kernel.mk
@@ -247,7 +247,7 @@  $(call KernelPackage/$(1)/config)
 				echo "NOTICE: module '$$$$$$$$mod' is built-in."; \
 			elif [ -e $$$$$$$$mod ]; then \
 				mkdir -p $$(1)/$(MODULES_SUBDIR) ; \
-				$(CP) -L $$$$$$$$mod $$(1)/$(MODULES_SUBDIR)/ ; \
+				$(CP) -L -l $$$$$$$$mod $$(1)/$(MODULES_SUBDIR)/ ; \
 			else \
 				echo "ERROR: module '$$$$$$$$mod' is missing." >&2; \
 				exit 1; \