Re: [PATCH 2/2] module: add support to avoid duplicates early on load

[Luis Chamberlain]

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