Date: Fri, 16 Dec 2016 09:39:56 +0100
From: "Luis R. Rodriguez" <mcgrof@kernel.org>
To: Petr Mladek <pmladek@suse.com>
Cc: "Luis R. Rodriguez" <mcgrof@kernel.org>, shuah@kernel.org,
        jeyu@redhat.com, rusty@rustcorp.com.au, ebiederm@xmission.com,
        dmitry.torokhov@gmail.com, acme@redhat.com, corbet@lwn.net,
        martin.wilck@suse.com, mmarek@suse.com, hare@suse.com, rwright@hpe.com,
        jeffm@suse.com, DSterba@suse.com, fdmanana@suse.com, neilb@suse.com,
        linux@roeck-us.net, rgoldwyn@suse.com, subashab@codeaurora.org,
        xypron.glpk@gmx.de, keescook@chromium.org, atomlin@redhat.com,
        mbenes@suse.cz, paulmck@linux.vnet.ibm.com, dan.j.williams@intel.com,
        jpoimboe@redhat.com, davem@davemloft.net, mingo@redhat.com,
        akpm@linux-foundation.org, torvalds@linux-foundation.org,
        linux-kselftest@vger.kernel.org, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC 03/10] kmod: add dynamic max concurrent thread count
Message-ID: <20161216083956.GG13946@wotan.suse.de>
References: <20161208184801.1689-1-mcgrof@kernel.org>
 <20161208194814.2485-1-mcgrof@kernel.org>
 <20161214153827.GH16064@pathway.suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20161214153827.GH16064@pathway.suse.cz>
User-Agent: Mutt/1.6.0 (2016-04-01)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1880
Lines: 59

On Wed, Dec 14, 2016 at 04:38:27PM +0100, Petr Mladek wrote:
> On Thu 2016-12-08 11:48:14, Luis R. Rodriguez wrote:
> > diff --git a/init/Kconfig b/init/Kconfig
> > index 271692a352f1..da2c25746937 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -2111,6 +2111,29 @@ config TRIM_UNUSED_KSYMS
> >  
> >  	  If unsure, or if you need to build out-of-tree modules, say N.
> >  
> > +config MAX_KMOD_CONCURRENT
> > +	int "Max allowed concurrent request_module() calls (6=>64, 10=>1024)"
> > +	range 0 14
> 
> Would not too small range break loading module dependencies?

No, dependencies are resolved by depmod, so userspace looks at the list and
just finit_module() the depenencies, skipping kmod. So the limit is
really only for kernel acting like a boss.

> I am not sure how it is implemented but it might require having
> some more module loads in progress.

Dependencies should be OK, a more serious concern with dependencies is
the aggregate memory it takes to load all dep modules for one required
module since finit_module() ends up allocating the struct module to copy
over data from userspace.

> I would give 6 as minimum. Nobody has troubles with the current limit.

Fair enough! Although disabling modprobe calls all together seemed like
a fun test, that should we allow that via the module parameter at least?

> > +	default 6 if !BASE_SMALL
> > +	default 7 if BASE_SMALL
> 
> Aren't the conditions inversed?

Whoops yes, sorry.

> > +void __init init_kmod_umh(void)
> > +{
> > +	if (!max_modprobes)
> > +		max_modprobes = min(max_threads/2,
> > +				    2 << CONFIG_MAX_KMOD_CONCURRENT);
> 
> This should be
> 
> 	1 << CONFIG_MAX_KMOD_CONCURRENT);
> 
> 1 << 1 = 2;
> 
> Note that this calculation is mentioned also some comments and
> documentation.

Heh sorry, yes fixed! Good thing I had still tested all along with the
value I intended though :P

  Luis