On Thu, Dec 15, 2016 at 10:57:42AM +1030, Rusty Russell wrote: > "Luis R. Rodriguez" <mcgrof@xxxxxxxxxx> 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. I have the patches to add support for the aliases now though and this is part of what helped shave off time from the tests. > We already have module_wq for this, we just need a bit more code to > share the return value; and there's a weird corner case there where we > have "modprobe foo param=invalid" then "modprobe foo param=valid" and we > fail both with -EINVAL, but it's probably not worth fixing. Hm OK. Although the set of patches I have fix and optimize now some of these corner cases one issue that I still didn't quite yet figure out was that a failure propagates secondary failures. That is, say a module fails and you have loaded 4 request for the same module, if the first request failed the last 3 *could* also fail. You can trigger and see this with the latest script: http://drvbp1.linux-foundation.org/~mcgrof/2016/12/16/kmod.sh The latest version of the test_kmod driver: http://drvbp1.linux-foundation.org/~mcgrof/2016/12/16/test_kmod.patch ./kmod.sh -t 0008 ./kmod.sh -t 0009 When either of these fail you'll on dmesg that either a few NULL or errors were found. It may not be worth fixing this race... given that after apply all of my patches I no longer see this at all, but I'm pretty sure a test case can be created to replicate more easily. FWIW a few things did occur to me: a) list_add_rcu() is used so new modules get added first b) find_module_all() returns the last module which was added as it traverses the module list Because of a) and b) if two modules for the same driver can be on the list at the same time then we'll get very likely a module which is unformed or going than a live module. Changing module addition to use list_add_tail_rcu() should mean we typically get the first module added to the list for the module name I think, but other than that I could not think clearly of the root case to allowing multiple errors. BTW should find_module_all() use rcu to traverse? --- 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; -- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
