Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 30 Sep 2022 13:30:10 -0700
From:   Luis Chamberlain <mcgrof@kernel.org>
To:     Petr Pavlu <petr.pavlu@suse.com>
Cc:     pmladek@suse.com, linux-modules@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] module: Merge same-name module load requests
Message-ID: <YzdR0gRNQI2BGnJ9@bombadil.infradead.org>
References: <20220919123233.8538-1-petr.pavlu@suse.com>
 <20220919123233.8538-3-petr.pavlu@suse.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220919123233.8538-3-petr.pavlu@suse.com>
Sender: Luis Chamberlain <mcgrof@infradead.org>
Precedence: bulk

On Mon, Sep 19, 2022 at 02:32:33PM +0200, Petr Pavlu 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. 

Please take a look at kmod selftest lib/test_kmod.c and the respective shell
selftest tools/testing/selftests/kmod/kmod.sh. Can you modify it to add
support to reproduce this issue?

> For instance, udev can make a request to insert
> a frequency module for each individual CPU 

That seems stupid indeed, it would seem we should be able for sure to prevent
such cases, it can't just be happening for frequency modules.

> when another frequency module
> is already loaded which causes the init function of the new module to
> return an error.

Holy smokes.

> 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.

Sure..

> 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.

OK nice.

> * 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.

Groovy.

> 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.

I'm having a hard time with this, because it is not clear if you are
suggesting this is a regression introduced by 6e6de3dee51a or not. I'd
like you to evaluate the impact of *not* merging a fix to older kernels.
In practice I think we'd end up with delays on boot, but is that all?
Would boot ever fail? The commit log does not make that clear.

The commit log should make it clear if this a regression or not and the
impact of not having these fixes merged. Please not that bots will try
to scrape for fixes and I suspect bots will pour their heart out on this
commit log and identify and assume this if a fix already as-is.

If this *is* a regression, we should try to see how perhaps we can split
this up into a part which is mergable to stable and then a secondary
part which  does some new fancy optimizations.

  Luis