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

[Luis Chamberlain]

On Mon, Jun 05, 2023 at 01:26:00PM +0200, David Hildenbrand wrote:
> I only did a single run on each kernel, should be good enough for the purpose here.
> 
> 
> 1) !debug config (not enabling KASAN)
> 
> a) master
> 
> # cat /sys/kernel/debug/modules/stats
>  Virtual mem wasted bytes       20358550424
18 GiB

>          Average mod size       217908
212 KiB

>     Average mod text size       63570
62 KiB

> b) patched
> 
> # cat /sys/kernel/debug/modules/stats
>  Virtual mem wasted bytes       0

> 2) debug config (enabling KASAN)
> 
> a) master
> 
> # cat /sys/kernel/debug/modules/stats
>  Virtual mem wasted bytes       6453862040
6 GiB

>          Average mod size       430517
420 KiB, so ballpark kasan pretty much doubles module size.

>     Average mod text size       197592
192 KiB, and is reflected on module .text too, in fact .text more than doubles.

It would have otherwise been difficult to get some of these stats, so
thanks! I make note of .text just because of the recent development work
going on for a new module_alloc(). About 14 MiB required to house a big
iron kasan enabled module .text, whereas about half is required for !kasan.

> b) patched
> 
> # cat /sys/kernel/debug/modules/stats
>  Virtual mem wasted bytes       6441296

We've gone down from ~6 GiB to ~6 MiB.

> So, with these (helpful) stats,

Extremely useful, yes thanks.

> the improvement is obvious (and explains the ~1s
> improvement I saw staring at the startup times of the udev services).
> 
> There are still some failed module loads with the debug config (only in the
> becoming state), I did not dive deeply into the actual code changes (-EBUSY),

That's fine, Linus' patch does not keep the lock for the entire life
of the module, it releases it right away after we're done with the
kernel_read(), and because of this, there is a small race in between
a thread the kernel_read() finishing and the module then being processed
into the modules linked list. During that time, if a new module with the
same name comes in, we'll have to allow it since the lock was released.
Those extra modules end up lingering to wait for the first one that made
it to the modules linked list.

I don't think we need to worry about 6 MiB, this patch alone should
suffice for a long time until userspace gets its act together and fixes
this properly. Fixing userspace should reduce some latencies as well on
bootup so someone who cares about bootup speeds on high end systems
could likely be encouraged to fix that.

> just spelling it out so we can decide if this is to be expected or some corner
> case that shouldn't be happening.

It is expected, in fact the fact that the heuristic works so well,
without keeping the lock forever, and therefore keeping the code changes
to a minimum is quite an amazing.

  Luis