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:   Wed, 19 Apr 2023 16:32:30 -0700
From:   Luis Chamberlain <mcgrof@kernel.org>
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     david@redhat.com, patches@lists.linux.dev,
        linux-modules@vger.kernel.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, pmladek@suse.com,
        petr.pavlu@suse.com, prarit@redhat.com,
        torvalds@linux-foundation.org, rafael@kernel.org,
        christophe.leroy@csgroup.eu, tglx@linutronix.de,
        peterz@infradead.org, song@kernel.org, rppt@kernel.org,
        dave@stgolabs.net, willy@infradead.org, vbabka@suse.cz,
        mhocko@suse.com, dave.hansen@linux.intel.com,
        colin.i.king@gmail.com, jim.cromie@gmail.com,
        catalin.marinas@arm.com, jbaron@akamai.com,
        rick.p.edgecombe@intel.com
Subject: Re: [PATCH] module: add debugging auto-load duplicate module support
Message-ID: <ZEB6DmF+l3LVrpFI@bombadil.infradead.org>
References: <20230418204636.791699-1-mcgrof@kernel.org>
 <2023041951-evolution-unwitting-1791@gregkh>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2023041951-evolution-unwitting-1791@gregkh>
Sender: Luis Chamberlain <mcgrof@infradead.org>
Precedence: bulk

On Wed, Apr 19, 2023 at 09:15:11AM +0200, Greg KH wrote:
> On Tue, Apr 18, 2023 at 01:46:36PM -0700, Luis Chamberlain wrote:
> You get 72 columns for changelog text, so you can use it :)

Sure! I forget what the limit is, but now I won't forget, new vimrc
settings:

set textwidth=100
autocmd FileType gitcommit set textwidth=72 
set colorcolumn=+1

> > +config MODULE_AUTOLOAD_SUPRESS_DUPS
> 
> MODULE_DEBUG_DUPLICATE perhaps?  It has nothing to do with autoloading
> (other than that is what userspace is doing)

I refer to module auto-loading as the kernel's use of the *request_module()
APIs. This code is used by the module auto-loading request_module() API
callers, prior to us even dealing with userspace.

> and you aren't suppressing anything except throwing up warnings, right?

Actually the code does converge duplicate auto-loading requests into one, but'll
just rename to MODULE_DEBUG_AUTOLOAD_DUPS.

> > +	bool "Debug duplicate modules with auto-loading"
> > +	help
> > +	  Module autoloading allows in-kernel code to request modules through
> > +	  the *request_module*() API calls. This in turn just calls userspace
> > +	  modprobe. Although modprobe checks to see if a module is already
> > +	  loaded before trying to load a module there is a small time window in
> > +	  which multiple duplicate requests can end up in userspace and multiple
> > +	  modprobe calls race calling finit_module() around the same time for
> > +	  duplicate modules. The finit_module() system call can consume in the
> > +	  worst case more than twice the respective module size in virtual
> > +	  memory for each duplicate module requests. Although duplicate module
> > +	  requests are non-fatal virtual memory is a limited resource and each
> > +	  duplicate module request ends up just wasting virtual memory.
> 
> It's not "wasted", as it is returned when the module is determined to be
> a duplicate.  Otherwise everyone will want this enabled as they think it
> will actually save memory.

I'll change the language to be clear the issue is memory pressure early
on boot. I'll also add a bit of language to help at least guide people
to realize that the real value-add for this, ie, I'll have to mention we
suspect issue is udev and not module auto-loading and that this however
may still help find a few cases we can optimize for.

  Luis