Skip to contentSkip to conversation

2.6 && module + -g && kernel w/o -g

RFC PATCH Tom Rini 3 participants 5 messages Jan 14, 2004 – Jan 15, 2004 1 message not in the archive

lore
Share & tools
lore

Tom Rini

author
#1 lore raw
In reply to a message not in the archive
Quoted 8 lines · Tom Rini

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 =====

/home/trini/work/kernel/testing/linux-2.6/arch/ppc/kernel/module.c line 95

Quoted 9 lines
/home/trini/work/kernel/testing/linux-2.6/arch/ppc/kernel/module.c renamed from arch/ppc/kernel/module.c +3 -0
@@ -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:

kernel/module.c +7 -0
@@ -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).

Signature · 2 lines
Tom Rini
http://gate.crashing.org/~trini/
#2 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Tom Rini

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
author
#3 lore raw
Quoted 26 lines · David Mosberger
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Tom Rini

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).

Quoted 4 lines

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.

Signature · 2 lines
Tom Rini
http://gate.crashing.org/~trini/
#4 lore raw
In reply to Tom Rini

In message <[email protected]> you write:

Quoted 7 lines

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.

kernel/module.c line 1448

Quoted 6 lines
kernel/module.c +6 -0
+		/* 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.

author
#5 lore raw
Quoted 5 lines · Rusty Russell

In message <[email protected]> you write:

Quoted 4 lines

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:

Quoted 7 lines
Quoted 3 lines

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. :)

Quoted 11 lines

Review

Quoted 6 lines
(quoted diff) +6 -0
+		/* 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.

arch/ppc/kernel/module.c +4 -0
@@ -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",
Signature · 2 lines
Tom Rini
http://gate.crashing.org/~trini/