module: fix bne2 "gave up waiting for init of module libcrc32c"
in [Kernel]
|
Prev: Article in Phoronix about loss of performance in 2.6.35 release candidates
Next: oom: introduce find_lock_task_mm() to fix !mm false positives
From: Rusty Russell on 2 Jun 2010 05:10 On Wed, 2 Jun 2010 05:42:32 pm Linus Torvalds wrote: > > On Wed, 2 Jun 2010, Linus Torvalds wrote: > > > > IOW, things like this.. Pure code movement to peel off some stuff. > > Here's a second one. It's slightly less trivial - since we now have error > cases - and equally untested so it may well be totally broken. But it also > cleans up a bit more, and avoids one of the goto targets, because the > "move_module()" helper now does both allocations or none. > > But now I'm bored and tired, so I think this will be all for tonight. The > load_module function has gone from ~490 lines to ~370 lines, but that's > still so many that I don't honestly think this makes any readability > improvement yet. Both applied and I'll play a little more later... Thanks! Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Rusty Russell on 2 Jun 2010 10:10 On Wed, 2 Jun 2010 04:51:46 pm Linus Torvalds wrote: > > On Wed, 2 Jun 2010, Rusty Russell wrote: > > > > Moved all the sysfs-exposing stuff to the end just after we put in the > > list (and thus to after the find check). > > Yeah, makes more sense that way. No reason to expose anything to sysfs > early. And splitting it into two patches makes it easier to follow than > the patch I posted. Ack. Found another locking issue: the code which verifies we don't export over an existing symbol needs to be atomic wrt. adding us to the list. This will be in tomorrow's linux-next. And load_module is down to 259 lines. The label chain at the end is no shorter tho :( I'll leave those cleanups until next merge window. The following changes since commit aef4b9aaae1decc775778903922bd0075cce7a88: Linus Torvalds (1): Merge branch 'next' of git://git.kernel.org/.../benh/powerpc are available in the git repository at: ssh://master.kernel.org/pub/scm/linux/kernel/git/rusty/linux-2.6 modules Eric Dumazet (1): module: module_unload_init() cleanup Linus Torvalds (4): module: Make the 'usage' lists be two-way module: move find_module check to end module: refactor load_module module: refactor load_module part 2 Rusty Russell (9): module: fix kdb's illicit use of struct module_use. module: move sysfs exposure to end of load_module module: Make module sysfs functions private. module: make locking more fine-grained. module: verify_export_symbols under the lock module: fix bne2 "gave up waiting for init of module libcrc32c" module: refactor load_module part 3 module: refactor load_module part 4 module: refactor load_module part 5 include/linux/module.h | 44 +-- kernel/debug/kdb/kdb_main.c | 12 +- kernel/module.c | 899 +++++++++++++++++++++++++------------------ 3 files changed, 547 insertions(+), 408 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Rusty Russell on 3 Jun 2010 09:10 On Thu, 3 Jun 2010 12:20:48 am Linus Torvalds wrote: > > On Wed, 2 Jun 2010, Rusty Russell wrote: > > > > Found another locking issue: the code which verifies we don't export over > > an existing symbol needs to be atomic wrt. adding us to the list. > > Yup. > > And now that I'm looking at that call-chain (to see if it would make sense > to use some other more specific lock - doesn't look like it: all the > readers are using RCU and this is the only writer), I also give you this > trivial one-liner. It changes each_symbol() to not put that constant array > on the stack, resulting in changing > > movq $C.388.31095, %rsi #, tmp85 > subq $376, %rsp #, > movq %rdi, %rbx # fn, fn > leaq -208(%rbp), %rdi #, tmp84 > movq %rbx, %rdx # fn, > rep movsl > xorl %esi, %esi # > leaq -208(%rbp), %rdi #, tmp87 > movq %r12, %rcx # data, > call each_symbol_in_section.clone.0 # > > into > > xorl %esi, %esi # > subq $216, %rsp #, > movq %rdi, %rbx # fn, fn > movq $arr.31078, %rdi #, > call each_symbol_in_section.clone.0 # > > which is not so much about being obviously shorter and simpler because we > don't unnecessarily copy that constant array around onto the stack, but > also about having a much smaller stack footprint (376 vs 216 bytes - see > the update of 'rsp'). > > Signed-off-by: Linus Torvalds <torvalds(a)linux-foundation.org> BTW, applied, thanks! I've finished the cleanup now, and removed the noinline on load_module; we're down to 124 bytes of stack for sys_init_module here (32 bit). The following changes since commit aef4b9aaae1decc775778903922bd0075cce7a88: Linus Torvalds (1): Merge branch 'next' of git://git.kernel.org/.../benh/powerpc are available in the git repository at: ssh://master.kernel.org/pub/scm/linux/kernel/git/rusty/linux-2.6 modules Eric Dumazet (1): module: module_unload_init() cleanup Linus Torvalds (6): module: Make the 'usage' lists be two-way module: move find_module check to end module: refactor load_module module: refactor load_module part 2 module: reduce stack usage for each_symbol() module: fix bne2 "gave up waiting for init of module libcrc32c" Rusty Russell (18): module: fix kdb's illicit use of struct module_use. module: move sysfs exposure to end of load_module module: Make module sysfs functions private. module: make locking more fine-grained. module: verify_export_symbols under the lock module: fix bne2 "gave up waiting for init of module libcrc32c" module: refactor load_module part 3 module: refactor load_module part 4 module: refactor load_module part 5 module: refactor out section header rewriting module: kallsyms functions take struct load_info module: layout_and_allocate module: sysfs cleanup module: pass load_info into other functions module: move module args strndup_user to just before use module: simplify per-cpu handling a little module: group post-relocation functions into post_relocation() module: cleanup comments, remove noinline include/linux/module.h | 44 +-- kernel/debug/kdb/kdb_main.c | 12 +- kernel/module.c | 1361 ++++++++++++++++++++++++------------------- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Rusty Russell on 3 Jun 2010 21:10 On Fri, 4 Jun 2010 01:54:19 am Linus Torvalds wrote: > > On Thu, 3 Jun 2010, Rusty Russell wrote: > > > > However, you're right that it has potential. I'll rename module_info to > > load_info if you don't mind tho: contains more semantic punch IMHO. > > Umm. One problem is that you will almost certainly eventually want to > expose that to the architecture "fixup" routines (ie things like > module_frob_arch_sections(), arch_mod_section_prepend()), and at that > point "load_info" is a horribly bad structure name, since it would show > up in <linux/module.h> and thus be exported all over. > > At least call it "struct module_load_info". But yes, I do agree that the > "load" part is important. Looking at the arch code, it has the advantage that it's self-contained. They've been pleasantly undemanding from the core over the years; I think archs doing tricky things with elf prefer to parse the object themselves anyway. And I'm not sure they want to revisit it, either. So I don't think we'd win much from changing them. I'm wrong later, I'll prepend "module_" to the struct name as an internal change then hit them all. > I looked at that particularly when doing that whole > > mod = setup_module_info(&info); > if (IS_ERR(mod)) { > err = PTR_ERR(mod); > goto free_hdr; > } > > thing, because that made "mod" have _three_ totally different values > (error, before, after) when jumping out to the failure paths. Yep, it now is back to sanity. Let's see if today's linux-next is happy. If so, do you want just the fixes or the whole refactoring too, while it's nice and fresh? Thanks! Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Rusty Russell on 4 Jun 2010 01:30
On Fri, 4 Jun 2010 11:25:00 am Linus Torvalds wrote: > But I suspect that is just myself trying to fool/argue my smarter half > into taking it all. Similar motivation for even asking. They can stew in linux-next for another cycle: just found a trivial !SMP compile issue, and with all the other config options in there it could use some baking... Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |