Andreas Robinson wrote:
On Sun, 2009-05-10 at 16:35 +0100, Alan Jenkins wrote:
On 5/10/09, Andreas Robinson <andr345@xxxxxxxxx> wrote:
Hi guys,
following Robby's suggestion, I have moved to github.
Please have a look at
http://github.com/andr345/module-init-tools/commits/master/
(or git://github.com/andr345/module-init-tools.git)
This is the second set of patches in the ELF-rewrite, organized for easy
review.
1) I don't think load_strings() needs to be exported in struct
module_ops. It's easy enough to open-code and it assumes you need the
results in a stringtbl. It looks like it's just an implementation
detail of load_symbols(). Just like deref_sym() is an implementation
detail of fetch_tables().
I see your point. load_strings() might be used in a bunch of places in
the next patchset, but I'll remove the export until it makes sense to
put it back.
2) "stringtbl" looks a little out of place here.
And I'm uneasy about
the way it's used for constant strings (weak ? "W" : "U") in
load_dep_syms(),
Hmm ok, I will declare them outside the function somewhere.
GCC treats constant strings as read-only data so they simply cannot go
out of scope, but the C standard is unclear here. It might be
implementation-specific.
as well as for malloc()'d strings.
That the strings are malloc'd elsewhere should not be a problem if the
caller is aware that the strings are in the ELF-file and that they
remain available until release_file() is called. Depmod can't release
any of the modules until it is finished.
Withdrawn.
Sorry, I'm too quick to jump to conclusions :-(. I thought the symbol
names were being copied onto the heap, but they're clearly not. I'm not
worried about string constants per se. I was mistakenly concerned that
one data structure was being used to return strings with very different
lifecycle rules.
It's an important issue, so you might make this more explicit. One way
would be to add an inline strtbl_free() (which collapses to free()).
But you are also welcome to dismiss me as a hopeless reader and leave it
as is:-).
I think a linked list with a field for the "weak" value would fit in
better. Allocate the string inline, like "struct symbol", and readers
don't have to worry about the string allocation as a separate issue.
You could call it "struct elf_symbol" to distinguish it.
AFAIK, depmod is called from init in Debian's/Ubuntu's initramfs.
>
My concern was that load_dep_syms() (and consequently depmod) would be
slower if all the symbols in all modules were malloc'd and copied one by
one.
The string table is faster as there's no copying and fewer allocations.
Urgh. If anyone is still doing that in the initramfs, I'm sure it'll be
fixed soon :-). But I agree it shouldn't be any slower than necessary,
and the string table makes complete sense in that context.
3) I tried running depmod under valgrind (you have to restrict the
number of modules, i.e. just copy a few modules to a staging directory
and use the -b option). Previously, depmod was valgrind-clean.
==12690== Conditional jump or move depends on uninitialised value(s)
==12690== at 0x804A095: output_deps_bin (depmod.c:473)
==12690== by 0x804CD85: main (depmod.c:1364)
==12690== Uninitialised value was created by a stack allocation
==12690== at 0x80517D3: add_value (index.c:86)
==12690==
==12690== Conditional jump or move depends on uninitialised value(s)
==12690== at 0x804B066: output_symbols_bin (depmod.c:783)
==12690== by 0x804CD85: main (depmod.c:1364)
==12690== Uninitialised value was created by a stack allocation
==12690== at 0x80517D3: add_value (index.c:86)
==12690==
==12690== ERROR SUMMARY: 4 errors from 2 contexts (suppressed: 11 from 1)
==12690== malloc/free: in use at exit: 12,849 bytes in 138 blocks.
==12690== malloc/free: 1,191 allocs, 1,053 frees, 321,180 bytes allocated.
Did you try "tests/runtests --valgrind" ? Hopefully that should pick
it up as well, but it's a bit slow :-).
How curious! I'll look into it.
However, I tried valgrind before posting actually, and something is
broken on my system and I haven't figured out what the problem is yet:
$ sudo valgrind ./depmod
==32422== Memcheck, a memory error detector.
[...snip banners...]
==32422==
--32422:0:aspacem Valgrind: FATAL: VG_N_SEGNAMES is too low.
--32422:0:aspacem Increase it and rebuild. Exiting now.
I'm not sure why this happens. This is valgrind-3.4.1-Debian.
*you have to restrict the number of modules, i.e. just copy a few
modules to a staging directory and use the -b option*
VG_N_SEGNAMES is the maximum number of mmap()s which valgrind can handle. depmod mmap()s every module at once.
Oh, and could you please send me add --valgrind patch please?
I wasn't subscribed to this list when you posted it and the archive has
mangled it somehow. (Or PEBKAC, more likely.)
Oh, sorry! I assumed it had already been applied. I'll resend it and
see if it wakes Jon up. It might have been my fault for forgetting to
put [PATCH] in the subject line.
Btw, I'm being serious when I talk about it being slow. "valgrind
./depmod" should be enough to reproduce this one issue, so do that first
:-).
Thanks
Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-modules" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
