On Wed, Jan 07, 2004 at 05:30:40PM -0700, Tom Rini wrote:
> Okay, this is what seems to fix the bug that hch found for me. Does
> this seem right to everyone else? I'm going to poke at it a bit more
> tomorrow, pending time.
>
> ===== arch/ppc/kernel/module.c 1.10 vs edited =====
> --- 1.10/arch/ppc/kernel/module.c Fri Sep 12 09:26:52 2003
> +++ edited//home/trini/work/kernel/testing/linux-2.6/arch/ppc/kernel/module.c Wed Jan 7 17:07:30 2004
> @@ -87,6 +87,9 @@
> if ((strstr(secstrings + sechdrs[i].sh_name, ".init") != 0)
> != is_init)
> continue;
> + /* Skip over debug bits. */
> + if (strstr(secstrings + sechdrs[i].sh_name, ".debug") != 0)
> + continue;
>
> if (sechdrs[i].sh_type == SHT_RELA) {
> DEBUGP("Found relocations in section %u\n", i);
Okay. I've been looking at stock 2.6.1 noticed that the fix for this
issue that Rusty proposed, and that ultimately made it into 2.6.1-rc3
(or so) is not correct. The problem is that we do:
err = module_frob_arch_sections(hdr, sechdrs, secstrings, mod);
/* Which goes over every .debug section and can take _ages_ on something
* like ipv6 */
... skip 100 lines ...
.... Checks Rusty / Linus added ...
The following patch fixes the problem for me on PPC32:
--- 1.96/kernel/module.c Wed Jan 7 22:46:59 2004
+++ edited/kernel/module.c Wed Jan 14 14:05:12 2004
@@ -1439,6 +1439,13 @@
strindex = sechdrs[i].sh_link;
strtab = (char *)hdr + sechdrs[strindex].sh_offset;
}
+
+ /* If we find any debug RELAs, frob these away now. */
+ if (sechdrs[i].sh_type == SHT_RELA &&
+ (strstr(secstrings+sechdrs[i].sh_name, ".debug")
+ != 0))
+ sechdrs[i].sh_type = SHT_NULL;
+
#ifndef CONFIG_MODULE_UNLOAD
/* Don't load .exit sections */
if (strncmp(secstrings+sechdrs[i].sh_name, ".exit", 5) == 0)
IMHO, this shouldn't be covered under a PPC32 test since at least PPC32,
PPC64 and Alpha have this issue, and I suspect that ia64, parisc, s390
and v850 have the problem as well (based on what their module_arch_frob
bits look to be doing).
--
Tom Rini
http://gate.crashing.org/~trini/
On Wed, Jan 14, 2004 at 02:23:29PM -0800, David Mosberger wrote:
> >>>>> On Wed, 14 Jan 2004 14:09:37 -0700, Tom Rini <[email protected]> said:
>
> Tom> The following patch fixes the problem for me on PPC32:
>
> Tom> --- 1.96/kernel/module.c Wed Jan 7 22:46:59 2004
> Tom> +++ edited/kernel/module.c Wed Jan 14 14:05:12 2004
> Tom> @@ -1439,6 +1439,13 @@
> Tom> strindex = sechdrs[i].sh_link;
> Tom> strtab = (char *)hdr + sechdrs[strindex].sh_offset;
> Tom> }
> Tom> +
> Tom> + /* If we find any debug RELAs, frob these away now. */
> Tom> + if (sechdrs[i].sh_type == SHT_RELA &&
> Tom> + (strstr(secstrings+sechdrs[i].sh_name, ".debug")
> Tom> + != 0))
> Tom> + sechdrs[i].sh_type = SHT_NULL;
> Tom> +
> Tom> #ifndef CONFIG_MODULE_UNLOAD
> Tom> /* Don't load .exit sections */
> Tom> if (strncmp(secstrings+sechdrs[i].sh_name, ".exit", 5) == 0)
>
> Tom> IMHO, this shouldn't be covered under a PPC32 test since at
> Tom> least PPC32, PPC64 and Alpha have this issue, and I suspect
> Tom> that ia64, parisc, s390 and v850 have the problem as well
> Tom> (based on what their module_arch_frob bits look to be doing).
>
> As far as ia64 is concerned, adding a check for .debug should be OK,
> but since the debug sections do not have any relocs anyhow, it
> shouldn't make much of a difference one way or another (addresses in
> the debug section a segment-relative).
OK, I wasn't sure. I just did a real quick skim of everyones module.c
to see if they did any for loops and checking of SHT_RELA.
--
Tom Rini
http://gate.crashing.org/~trini/
>>>>> On Wed, 14 Jan 2004 14:09:37 -0700, Tom Rini <[email protected]> said:
Tom> The following patch fixes the problem for me on PPC32:
Tom> --- 1.96/kernel/module.c Wed Jan 7 22:46:59 2004
Tom> +++ edited/kernel/module.c Wed Jan 14 14:05:12 2004
Tom> @@ -1439,6 +1439,13 @@
Tom> strindex = sechdrs[i].sh_link;
Tom> strtab = (char *)hdr + sechdrs[strindex].sh_offset;
Tom> }
Tom> +
Tom> + /* If we find any debug RELAs, frob these away now. */
Tom> + if (sechdrs[i].sh_type == SHT_RELA &&
Tom> + (strstr(secstrings+sechdrs[i].sh_name, ".debug")
Tom> + != 0))
Tom> + sechdrs[i].sh_type = SHT_NULL;
Tom> +
Tom> #ifndef CONFIG_MODULE_UNLOAD
Tom> /* Don't load .exit sections */
Tom> if (strncmp(secstrings+sechdrs[i].sh_name, ".exit", 5) == 0)
Tom> IMHO, this shouldn't be covered under a PPC32 test since at
Tom> least PPC32, PPC64 and Alpha have this issue, and I suspect
Tom> that ia64, parisc, s390 and v850 have the problem as well
Tom> (based on what their module_arch_frob bits look to be doing).
As far as ia64 is concerned, adding a check for .debug should be OK,
but since the debug sections do not have any relocs anyhow, it
shouldn't make much of a difference one way or another (addresses in
the debug section a segment-relative).
--david
In message <[email protected]> you write:
> Okay. I've been looking at stock 2.6.1 noticed that the fix for this
> issue that Rusty proposed, and that ultimately made it into 2.6.1-rc3
> (or so) is not correct. The problem is that we do:
>
> err = module_frob_arch_sections(hdr, sechdrs, secstrings, mod);
> /* Which goes over every .debug section and can take _ages_ on something
> * like ipv6 */
Right. So the arch-specific module_frob_arch_sections() can be slow.
Logically, the fix should be in those module_frob_arch_sections(), not
in the generic code.
> + /* If we find any debug RELAs, frob these away now. */
> + if (sechdrs[i].sh_type == SHT_RELA &&
> + (strstr(secstrings+sechdrs[i].sh_name, ".debug")
> + != 0))
> + sechdrs[i].sh_type = SHT_NULL;
> +
Doesn't cover SHT_REL, and I really dislike name matches: they've bitten
us before.
Really, I prefer the arch-specific optimization.
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.
On Thu, Jan 15, 2004 at 10:00:11AM +1100, Rusty Russell wrote:
> In message <[email protected]> you write:
> > Okay. I've been looking at stock 2.6.1 noticed that the fix for this
> > issue that Rusty proposed, and that ultimately made it into 2.6.1-rc3
> > (or so) is not correct. The problem is that we do:
> >
> > err = module_frob_arch_sections(hdr, sechdrs, secstrings, mod);
> > /* Which goes over every .debug section and can take _ages_ on something
> > * like ipv6 */
>
> Right. So the arch-specific module_frob_arch_sections() can be slow.
> Logically, the fix should be in those module_frob_arch_sections(), not
> in the generic code.
So it was right the first time, OK. :)
> > + /* If we find any debug RELAs, frob these away now. */
> > + if (sechdrs[i].sh_type == SHT_RELA &&
> > + (strstr(secstrings+sechdrs[i].sh_name, ".debug")
> > + != 0))
> > + sechdrs[i].sh_type = SHT_NULL;
> > +
>
> Doesn't cover SHT_REL, and I really dislike name matches: they've bitten
> us before.
>
> Really, I prefer the arch-specific optimization.
FWIW, this isn't an optimization, taking 12 minutes to load the ipv6
module is a bug. :)
Andrew, can you please apply the following patch? Thanks.
--- 1.10/arch/ppc/kernel/module.c Fri Sep 12 09:26:52 2003
+++ edited/arch/ppc/kernel/module.c Thu Jan 15 08:35:40 2004
@@ -88,6 +88,10 @@
!= is_init)
continue;
+ /* We don't want to look at debug sections. */
+ if (strstr(secstrings + sechdrs[i].sh_name, ".debug") != 0)
+ continue;
+
if (sechdrs[i].sh_type == SHT_RELA) {
DEBUGP("Found relocations in section %u\n", i);
DEBUGP("Ptr: %p. Number: %u\n",
--
Tom Rini
http://gate.crashing.org/~trini/