On Thu, May 25, 2023 at 02:12:49PM -0700, Linus Torvalds wrote:
> On Thu, May 25, 2023 at 11:45 AM Lucas De Marchi
> <lucas.demarchi@xxxxxxxxx> wrote:
> >
> > Are you willig to merge (a possibly improved version of) your patch
> > or the userspace change is still something that would be desired?
>
> I think a user space change should still be something that people
> should look at, particularly as the kernel side patch I'm willing to
> accept doesn't catch the "completely serial" cases, only the "trying
> to load at the same time that the same module is literally busy being
> loaded".
>
> But I've cleaned up my patch a bit, and while the cleaned-up version
> is rather larger as a patch (mainly because of just also re-organizing
> the finit_module() code to do all the 'struct file' prep), I'm
> actually pretty happy with this attached patch conceptually.
>
> In this form, it actually "makes sense" to me, rather than being just
> clearly a workaround. Also, unlike the previous patch, this doesn't
> actually make any changes to the basic kernel_read_file() set of
> functions, it's all done by the module loading code itself.
>
> Luis, would you mind testing this version on your load? It still won't
> actually handle the purely serial case, so there *will* be those
> spurious double module reads from different CPU's just doing the
> things serially, but the exclusive file access region has been
> extended to not just cover the actual file content reading, but to
> cover the whole "turn it into a a real module" part too.
>
> Also, this does *not* update some of the comments in the module
> loading. I changed finit_module to use "kernel_read_file()" instead of
> "kernel_read_file_from_fd()", since it actually now has to look up the
> file descriptor anyway. But the comments still talk about that
> "from_fd" thing.
>
> Anyway, this is back to "ENTIRELY UNTESTED" territory, in that I've
> compiled this, but haven't booted it. The changes look obvious, but
> hey, mistakes happen.
>
> And the commit message is just a place-holder. Obviously. I won't sign
> off on this or write more of a commit message until it has had some
> real testing.
With 255 vcpus:
Before:
vagrant@kmod ~ $ sudo systemd-analyze
Startup finished in 41.653s (kernel) + 44.305s (userspace) = 1min 25.958s
graphical.target reached after 44.178s in userspace.
root@kmod ~ # grep "Virtual mem wasted bytes"
/sys/kernel/debug/modules/stats
Virtual mem wasted bytes 1949006968
So ~1.8 GiB.
After:
root@kmod ~ # systemd-analyze
Startup finished in 35.872s (kernel) + 41.715s (userspace) = 1min 17.588s
graphical.target reached after 41.594s in userspace.
root@kmod ~ # cat /sys/kernel/debug/modules/stats
Mods ever loaded 66
Mods failed on kread 0
Mods failed on decompress 0
Mods failed on becoming 1
Mods failed on load 0
Total module size 11268096
Total mod text size 4149248
Failed kread bytes 0
Failed decompress bytes 0
Failed becoming bytes 474688
Failed kmod bytes 0
Virtual mem wasted bytes 474688
Average mod size 170729
Average mod text size 62868
Avg fail becoming bytes 474688
Duplicate failed modules:
Module-name How-many-times Reason
cryptd 1 Becoming
root@kmod ~ # du -b /lib/modules/6.3.0-next-20230505+/kernel/crypto/cryptd.ko
475409 /lib/modules/6.3.0-next-20230505+/kernel/crypto/cryptd.ko
So yeah definitely a pretty good improvement. Sometimes the system boots
without any duplicates at all, for some reason Vs the previous attempt.
Tested-by: Luis Chamberlain <mcgrof@xxxxxxxxxx>
Luis
