Re: Semaphore assembly-code bug

[linux-os]

On Fri, 29 Oct 2004, Linus Torvalds wrote:

> On Fri, 29 Oct 2004, linux-os wrote:
> > Linus, please check this out.
>
> Yes, I concur. However, I'd suggest changing the "addl $4,%esp" into a
> "popl %ecx", which is smaller and apparently faster on some CPU's (ecx
> obviously gets immediately overwritten by the next popl).
>
> Btw, this is another case where we _really_ want "asmlinkage" to mean that
> the compiler does not own the argument stack. Is there any chance of
> getting a function attribute like that into future versions of gcc?
> Richard, Jan, Andi? Or does it already exist somewhere?
>
> 		Linus

Here's a version that uses `leal 4(esp), esp` to add
4 to the stack-pointer. Since this 'address-calculation`
is done in an different portion of Intel CPUs, there
is some parallel operation that can occur. The 'pop ecx'
would access memory and, therefore be slower than
simple register operations.

FYI I'm running a kernel with this patch already.


--- linux-2.6.9/arch/i386/kernel/semaphore.c.orig	2004-10-29 13:00:17.961579368 -0400
+++ linux-2.6.9/arch/i386/kernel/semaphore.c	2004-10-29 13:03:35.046617888 -0400
@@ -198,9 +198,11 @@
 #endif
 	"pushl %eax\n\t"
 	"pushl %edx\n\t"
-	"pushl %ecx\n\t"
+	"pushl %ecx\n\t"		// Register to save
+	"pushl %ecx\n\t"		// Passed parameter
 	"call __down\n\t"
-	"popl %ecx\n\t"
+	"leal 0x04(%esp), %esp\t\n"	// Bypass corrupted parameter
+	"popl %ecx\n\t"			// Restore original
 	"popl %edx\n\t"
 	"popl %eax\n\t"
 #if defined(CONFIG_FRAME_POINTER)
@@ -220,9 +222,11 @@
 	"movl  %esp,%ebp\n\t"
 #endif
 	"pushl %edx\n\t"
-	"pushl %ecx\n\t"
+	"pushl %ecx\n\t"		// Save register
+	"pushl %ecx\n\t"		// Passed parameter
 	"call __down_interruptible\n\t"
-	"popl %ecx\n\t"
+	"leal 0x04(%esp), %esp\n\t"	// Bypass corrupted parameter
+	"popl %ecx\n\t"			// Restore register
 	"popl %edx\n\t"
 #if defined(CONFIG_FRAME_POINTER)
 	"movl %ebp,%esp\n\t"
@@ -241,9 +245,11 @@
 	"movl  %esp,%ebp\n\t"
 #endif
 	"pushl %edx\n\t"
-	"pushl %ecx\n\t"
+	"pushl %ecx\n\t"		// Save register
+	"pushl %ecx\n\t"		// Passed parameter
 	"call __down_trylock\n\t"
-	"popl %ecx\n\t"
+	"leal 0x04(%esp), %esp\n\t"	// Bypass corrupted parameter
+	"popl %ecx\n\t"			// Restore register
 	"popl %edx\n\t"
 #if defined(CONFIG_FRAME_POINTER)
 	"movl %ebp,%esp\n\t"
@@ -259,9 +265,11 @@
 "__up_wakeup:\n\t"
 	"pushl %eax\n\t"
 	"pushl %edx\n\t"
-	"pushl %ecx\n\t"
+	"pushl %ecx\n\t"		// Save register
+	"pushl %ecx\n\t"		// Passed parameter
 	"call __up\n\t"
-	"popl %ecx\n\t"
+	"leal 0x04(%esp), %esp\n\t"	// Bypass corrupted parameter
+	"popl %ecx\n\t"			// Restore register
 	"popl %edx\n\t"
 	"popl %eax\n\t"
 	"ret"



Cheers,
Dick Johnson
Penguin : Linux version 2.6.9 on an i686 machine (5537.79 BogoMips).
 Notice : All mail here is now cached for review by John Ashcroft.
                 98.36% of all statistics are fiction.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/