Re: [PATCH v2 2/2] module: Merge same-name module load requests

[Petr Pavlu]

On 10/14/22 09:54, David Hildenbrand wrote:
> On Mon, Sep 19, 2022 at 2:33 PM Petr Pavlu <petr.pavlu@xxxxxxxx> wrote:
>>
>> During a system boot, it can happen that the kernel receives a burst of
>> requests to insert the same module but loading it eventually fails
>> during its init call. For instance, udev can make a request to insert
>> a frequency module for each individual CPU when another frequency module
>> is already loaded which causes the init function of the new module to
>> return an error.
>>
>> The module loader currently serializes all such requests, with the
>> barrier in add_unformed_module(). This creates a lot of unnecessary work
>> and delays the boot.
>>
>> This patch improves the behavior as follows:
>> * A check whether a module load matches an already loaded module is
>>   moved right after a module name is determined. -EEXIST continues to be
>>   returned if the module exists and is live, -EBUSY is returned if
>>   a same-name module is going.
>> * A new reference-counted shared_load_info structure is introduced to
>>   keep track of duplicate load requests. Two loads are considered
>>   equivalent if their module name matches. In case a load duplicates
>>   another running insert, the code waits for its completion and then
>>   returns -EEXIST or -EBUSY depending on whether it succeeded.
>>
>> Note that prior to 6e6de3dee51a ("kernel/module.c: Only return -EEXIST
>> for modules that have finished loading"), the kernel already did merge
>> some of same load requests but it was more by accident and relied on
>> specific timing. The patch brings this behavior back in a more explicit
>> form.
>>
>> Signed-off-by: Petr Pavlu <petr.pavlu@xxxxxxxx>
>> ---
> 
> Hi Petr,
> 
> as you might have seen I sent a patch/fix yesterday (not being aware
> of this patch and that
> this is also a performance issue, which is interesting), that
> similarly makes sure that modules
> are unique early.
> 
> https://lkml.kernel.org/r/20221013180518.217405-1-david@xxxxxxxxxx
> 
> It doesn't perform the -EBUSY changes or use something like
> shared_load_info/refcounts;
> it simply uses a second list while the module cannot be placed onto
> the module list yet.
> 
> Not sure if that part is really required (e.g., for performance
> reasons). Like Luis, I feel like
> some of these parts could be split into separate patches, if the other
> parts are really required.

The shared_load_info/refcounts/-EBUSY logic is actually an important part
which addresses the regression mentioned in the commit message and which I'm
primarily trying to fix.

> I just tested your patch in the environment where I can reproduce the
> vmap allocation issue, and
> (unsurprisingly) this patch similarly seems to fix the issue.
> 
> So if your patch ends up upstream, it would be good to add some details
> of my patch description (vmap allocation issue) to this patch description.

Thanks for testing this patch. I will add a note about the vmap allocation
issue to the patch description.

Petr