From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756982AbcLQDyq (ORCPT ); Fri, 16 Dec 2016 22:54:46 -0500 Received: from ozlabs.org ([103.22.144.67]:49855 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753440AbcLQDyp (ORCPT ); Fri, 16 Dec 2016 22:54:45 -0500 From: Rusty Russell To: "Luis R. Rodriguez" Cc: "Luis R. Rodriguez" , shuah@kernel.org, jeyu@redhat.com, ebiederm@xmission.com, dmitry.torokhov@gmail.com, acme@redhat.com, corbet@lwn.net, martin.wilck@suse.com, mmarek@suse.com, pmladek@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 10/10] kmod: add a sanity check on module loading In-Reply-To: <20161216083123.GF13946@wotan.suse.de> References: <20161208184801.1689-1-mcgrof@kernel.org> <20161208194930.2816-1-mcgrof@kernel.org> <87k2b2kpdt.fsf@rustcorp.com.au> <20161216083123.GF13946@wotan.suse.de> User-Agent: Notmuch/0.22.1 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Sat, 17 Dec 2016 14:24:30 +1030 Message-ID: <87zijvjjm1.fsf@rustcorp.com.au> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org "Luis R. Rodriguez" writes: > On Thu, Dec 15, 2016 at 10:57:42AM +1030, Rusty Russell wrote: >> "Luis R. Rodriguez" writes: >> > kmod has an optimization in place whereby if a some kernel code >> > uses request_module() on a module already loaded we never bother >> > userspace as the module already is loaded. This is not true for >> > get_fs_type() though as it uses aliases. >> >> Well, the obvious thing to do here is block kmod if we're currently >> loading the same module. > > OK thanks, I've now added this, it sure helps. Test cases 0008 and 0009 require > hammering on the test over and over to see a failure on vanilla kernels, > an upper bound I found was about 150 times each test. Running test 0008 > 150 times with this enhancement you mentioned shaves off ~4 seconds. > For test 0009 it shaves off ~16 seconds, but as I note below the alias support > was needed as well. > >> Otherwise it has to do some weird spinning >> thing in userspace anyway. > > Right, but note that the get_fs_type() tests would still fail given > module.c was not alias-aware yet. AFAICT the mistake here is that kmod is returning "done, OK" when the module it is trying to load is already loading (but not finished loading). That's the root problem; it's an attempt at optimization by kmod which goes awry. Looking at the code in the kernel, we *already* get this right: block if a module is still loading anyway. Once it succeeds we return -EBUSY; if it fails we'll proceed to try to load it again. I don't understand what you're trying to fix with adding aliases in-kernel? > FWIW a few things did occur to me: > > a) list_add_rcu() is used so new modules get added first Only after we're sure that there are no duplicates. > b) find_module_all() returns the last module which was added as it traverses > the module list > BTW should find_module_all() use rcu to traverse? Yes; the kallsyms code does this on Oops. Not really a big issue in practice, but a nice fix. Thanks, Rusty. > > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -594,7 +594,7 @@ static struct module *find_module_all(const char *name, size_t len, > > module_assert_mutex_or_preempt(); > > - list_for_each_entry(mod, &modules, list) { > + list_for_each_entry_rcu(mod, &modules, list) { > if (!even_unformed && mod->state == MODULE_STATE_UNFORMED) > continue; > if (strlen(mod->name) == len && !memcmp(mod->name, name, len)) > @@ -3532,7 +3532,7 @@ static int add_unformed_module(struct module *mod) > goto out; > } > mod_update_bounds(mod); > - list_add_rcu(&mod->list, &modules); > + list_add_tail_rcu(&mod->list, &modules); > mod_tree_insert(mod); > err = 0; >