[PATCH][2.6] Allow x86_64 to reenable interrupts on contention

[PATCH][2.6] Allow x86_64 to reenable interrupts on contention

[Zwane Mwaikambo]

This is a follow up to the previous patches for ia64 and i386, it will
allow x86_64 to reenable interrupts during contested locks depending on
previous interrupt enable status. It has been runtime and compile tested
on UP and 2x SMP Linux-tiny/x86_64.

Signed-off-by: Zwane Mwaikambo <zwane@fsmlabs.com>

--- linux-2.6-amd64/include/asm-x86_64/spinlock.h.orig	2004-07-27 03:32:18.689949016 -0400
+++ linux-2.6-amd64/include/asm-x86_64/spinlock.h	2004-07-27 03:45:19.610231088 -0400
@@ -41,7 +41,6 @@ typedef struct {

 #define spin_is_locked(x)	(*(volatile signed char *)(&(x)->lock) <= 0)
 #define spin_unlock_wait(x)	do { barrier(); } while(spin_is_locked(x))
-#define _raw_spin_lock_flags(lock, flags) _raw_spin_lock(lock)

 #define spin_lock_string \
 	"\n1:\t" \
@@ -55,6 +54,23 @@ typedef struct {
 	"jmp 1b\n" \
 	LOCK_SECTION_END

+#define spin_lock_string_flags \
+	"\n1:\t" \
+	"lock ; decb %0\n\t" \
+	"js 2f\n\t" \
+	LOCK_SECTION_START("") \
+	"2:\t" \
+	"test $0x200, %1\n\t" \
+	"jz 3f\n\t" \
+	"sti\n\t" \
+	"3:\t" \
+	"rep;nop\n\t" \
+	"cmpb $0, %0\n\t" \
+	"jle 3b\n\t" \
+	"cli\n\t" \
+	"jmp 1b\n" \
+	LOCK_SECTION_END
+
 /*
  * This works. Despite all the confusion.
  * (except on PPro SMP or if we are using OOSTORE)
@@ -125,6 +141,20 @@ printk("eip: %p\n", &&here);
 		:"=m" (lock->lock) : : "memory");
 }

+static inline void _raw_spin_lock_flags (spinlock_t *lock, unsigned long flags)
+{
+#ifdef CONFIG_DEBUG_SPINLOCK
+	__label__ here;
+here:
+	if (unlikely(lock->magic != SPINLOCK_MAGIC)) {
+		printk("eip: %p\n", &&here);
+		BUG();
+	}
+#endif
+	__asm__ __volatile__(spin_lock_string_flags
+		:"=m" (lock->lock) : "r" (flags) : "memory");
+}
+

 /*
  * Read-write spinlocks, allowing multiple readers

Re: [PATCH][2.6] Allow x86_64 to reenable interrupts on contention

[Andi Kleen]

On Tue, 27 Jul 2004 05:29:10 -0400 (EDT)
Zwane Mwaikambo <zwane@linuxpower.ca> wrote:

> This is a follow up to the previous patches for ia64 and i386, it will
> allow x86_64 to reenable interrupts during contested locks depending on
> previous interrupt enable status. It has been runtime and compile tested
> on UP and 2x SMP Linux-tiny/x86_64.

This will likely increase code size. Do you have numbers by how much? And is it 
really worth it?

-Andi

Re: [PATCH][2.6] Allow x86_64 to reenable interrupts on contention

[Zwane Mwaikambo]

On Tue, 27 Jul 2004, Andi Kleen wrote:

> On Tue, 27 Jul 2004 05:29:10 -0400 (EDT)
> Zwane Mwaikambo <zwane@linuxpower.ca> wrote:
>
> > This is a follow up to the previous patches for ia64 and i386, it will
> > allow x86_64 to reenable interrupts during contested locks depending on
> > previous interrupt enable status. It has been runtime and compile tested
> > on UP and 2x SMP Linux-tiny/x86_64.
>
> This will likely increase code size. Do you have numbers by how much? And is it
> really worth it?

Yes there is a growth;

   text    data     bss     dec     hex filename
3655358 1340511  486128 5481997  53a60d vmlinux-after
3648445 1340511  486128 5475084  538b0c vmlinux-before

And this was on i386;

   text    data     bss     dec     hex filename
2628024  921731       0 3549755  362a3b vmlinux-after
2621369  921731       0 3543100  36103c vmlinux-before

Keith Owens managed to get increased throughput as the original patch was
driven by poor performance from a workload. I think it's worth it just for
the reduced interrupt latency, the code size issue can also be taken care
of, but that requires benchmarking as the change is a bit more drastic.

Re: [PATCH][2.6] Allow x86_64 to reenable interrupts on contention

[Andi Kleen]

On Tue, 27 Jul 2004 10:31:27 -0400 (EDT)
Zwane Mwaikambo <zwane@linuxpower.ca> wrote:

> On Tue, 27 Jul 2004, Andi Kleen wrote:
> 
> > On Tue, 27 Jul 2004 05:29:10 -0400 (EDT)
> > Zwane Mwaikambo <zwane@linuxpower.ca> wrote:
> >
> > > This is a follow up to the previous patches for ia64 and i386, it will
> > > allow x86_64 to reenable interrupts during contested locks depending on
> > > previous interrupt enable status. It has been runtime and compile tested
> > > on UP and 2x SMP Linux-tiny/x86_64.
> >
> > This will likely increase code size. Do you have numbers by how much? And is it
> > really worth it?
> 
> Yes there is a growth;
> 
>    text    data     bss     dec     hex filename
> 3655358 1340511  486128 5481997  53a60d vmlinux-after
> 3648445 1340511  486128 5475084  538b0c vmlinux-before

That's significant.

> 
> And this was on i386;
> 
>    text    data     bss     dec     hex filename
> 2628024  921731       0 3549755  362a3b vmlinux-after
> 2621369  921731       0 3543100  36103c vmlinux-before
> 
> Keith Owens managed to get increased throughput as the original patch was
> driven by poor performance from a workload. I think it's worth it just for

What workload was that?

I am not sure it is a good idea to "fix" workloads by making spinlocks better.
It is likely better to just fix the locking for that workload, that would
likely improve the workload a lot more.

> the reduced interrupt latency, the code size issue can also be taken care
> of, but that requires benchmarking as the change is a bit more drastic.

Do you have numbers on that? Frankly if someone is spinning on irq disabled
locks for a long time they should just fix their code.

-Andi

Re: [PATCH][2.6] Allow x86_64 to reenable interrupts on contention

[Ricky Beam]

On Tue, 27 Jul 2004, Andi Kleen wrote:
>> Yes there is a growth;
>>
>>    text    data     bss     dec     hex filename
>> 3655358 1340511  486128 5481997  53a60d vmlinux-after
>> 3648445 1340511  486128 5475084  538b0c vmlinux-before
(diff: 6,913bytes, 0.19% increase)

>That's significant.

6,913bytes is "significant"?  *Now*, after a decade of bloat, you want to
pinch pennies? (Do I need to start the flame war(s) again?  Need I mention
"numcpus"?)

>> 2628024  921731       0 3549755  362a3b vmlinux-after
>> 2621369  921731       0 3543100  36103c vmlinux-before
(diff: 6,655bytes, 0.25% increase)

--Ricky

Re: [PATCH][2.6] Allow x86_64 to reenable interrupts on contention

[Andrew Morton]

Zwane Mwaikambo <zwane@linuxpower.ca> wrote:
>
> On Tue, 27 Jul 2004, Andi Kleen wrote:
> 
>  > On Tue, 27 Jul 2004 05:29:10 -0400 (EDT)
>  > Zwane Mwaikambo <zwane@linuxpower.ca> wrote:
>  >
>  > > This is a follow up to the previous patches for ia64 and i386, it will
>  > > allow x86_64 to reenable interrupts during contested locks depending on
>  > > previous interrupt enable status. It has been runtime and compile tested
>  > > on UP and 2x SMP Linux-tiny/x86_64.
>  >
>  > This will likely increase code size. Do you have numbers by how much? And is it
>  > really worth it?
> 
>  Yes there is a growth;
> 
>     text    data     bss     dec     hex filename
>  3655358 1340511  486128 5481997  53a60d vmlinux-after
>  3648445 1340511  486128 5475084  538b0c vmlinux-before

The growth is all in the out-of-line section, so there should be no
significant additional icache pressure.

Re: [PATCH][2.6] Allow x86_64 to reenable interrupts on contention

[William Lee Irwin III]

On Tue, 27 Jul 2004, Andi Kleen wrote:
>>> This will likely increase code size. Do you have numbers by how
>>> much? And is it really worth it?

Zwane Mwaikambo <zwane@linuxpower.ca> wrote:
>>  Yes there is a growth;
>>     text    data     bss     dec     hex filename
>>  3655358 1340511  486128 5481997  53a60d vmlinux-after
>>  3648445 1340511  486128 5475084  538b0c vmlinux-before

On Tue, Jul 27, 2004 at 12:01:25PM -0700, Andrew Morton wrote:
> The growth is all in the out-of-line section, so there should be no
> significant additional icache pressure.

There are also flash and similar absolute space footprints to consider.
Experiments seem to suggest that consolidating the lock sections and
other spinlock code can reduce kernel image size by as much as 220KB on
ia32 with no performance impact (rigorous benchmarks still in progress).

That said, I agree with zwane's interrupt enablement change.


-- wli

Re: [PATCH][2.6] Allow x86_64 to reenable interrupts on contention

[Keith Owens]

On Tue, 27 Jul 2004 17:14:15 -0700, 
William Lee Irwin III <wli@holomorphy.com> wrote:
>On Tue, 27 Jul 2004, Andi Kleen wrote:
>>>> This will likely increase code size. Do you have numbers by how
>>>> much? And is it really worth it?
>
>Zwane Mwaikambo <zwane@linuxpower.ca> wrote:
>>>  Yes there is a growth;
>>>     text    data     bss     dec     hex filename
>>>  3655358 1340511  486128 5481997  53a60d vmlinux-after
>>>  3648445 1340511  486128 5475084  538b0c vmlinux-before
>
>On Tue, Jul 27, 2004 at 12:01:25PM -0700, Andrew Morton wrote:
>> The growth is all in the out-of-line section, so there should be no
>> significant additional icache pressure.
>
>There are also flash and similar absolute space footprints to consider.
>Experiments seem to suggest that consolidating the lock sections and
>other spinlock code can reduce kernel image size by as much as 220KB on
>ia32 with no performance impact (rigorous benchmarks still in progress).

I consolidated the spinlock contention path to a single routine on
ia64, with big space savings.  The problem with the ia64 consolidation
was backtracing through a contended lock; the ia64 unwind API is not
designed for code that is shared between multiple code paths but uses
non-standard entry and exit conventions.  In the end, David Mosberger
did a patch to gcc to do lightweight calls to the out of line
contention code, just to get reliable backtraces.

kdb has workarounds for backtracing through ia64 contended locks when
the kernel is built with older versions of gcc.  gdb (and hence kgdb)
has no idea about the special out of line code.  Mind you, the same is
true right now with the out of line i386 code, you need special
heuristics to backtrace the existing spinlock code reliably.  That will
only get worse with Zwane's patch, interrupts can now occur in the out
of line code.

Are you are planning to consolidate the out of line code for i386?  Is
there a patch (even work in progress) so I can start thinking about
doing reliable backtraces?

Re: [PATCH][2.6] Allow x86_64 to reenable interrupts on contention

[William Lee Irwin III]

On Tue, 27 Jul 2004 17:14:15 -0700, 
>> There are also flash and similar absolute space footprints to consider.
>> Experiments seem to suggest that consolidating the lock sections and
>> other spinlock code can reduce kernel image size by as much as 220KB on
>> ia32 with no performance impact (rigorous benchmarks still in progress).

On Wed, Jul 28, 2004 at 10:35:08AM +1000, Keith Owens wrote:
> I consolidated the spinlock contention path to a single routine on
> ia64, with big space savings.  The problem with the ia64 consolidation
> was backtracing through a contended lock; the ia64 unwind API is not
> designed for code that is shared between multiple code paths but uses
> non-standard entry and exit conventions.  In the end, David Mosberger
> did a patch to gcc to do lightweight calls to the out of line
> contention code, just to get reliable backtraces.
> kdb has workarounds for backtracing through ia64 contended locks when
> the kernel is built with older versions of gcc.  gdb (and hence kgdb)
> has no idea about the special out of line code.  Mind you, the same is
> true right now with the out of line i386 code, you need special
> heuristics to backtrace the existing spinlock code reliably.  That will
> only get worse with Zwane's patch, interrupts can now occur in the out
> of line code.

It's good to know there is a precedent and that the backtrace issue has
been looked at on other architectures.


On Wed, Jul 28, 2004 at 10:35:08AM +1000, Keith Owens wrote:
> Are you are planning to consolidate the out of line code for i386?  Is
> there a patch (even work in progress) so I can start thinking about
> doing reliable backtraces?

The experiments were carried out using the standard calling convention.
We may investigate less standard calling conventions, but they should
actually already be there given __write_lock_failed/__read_lock_failed.
i.e. if reliable backtraces are going to be an issue they should
already be an issue for rwlocks.


-- wli

Re: [PATCH][2.6] Allow x86_64 to reenable interrupts on contention

[Zwane Mwaikambo]

On Wed, 28 Jul 2004, Keith Owens wrote:

> I consolidated the spinlock contention path to a single routine on
> ia64, with big space savings.  The problem with the ia64 consolidation
> was backtracing through a contended lock; the ia64 unwind API is not
> designed for code that is shared between multiple code paths but uses
> non-standard entry and exit conventions.  In the end, David Mosberger
> did a patch to gcc to do lightweight calls to the out of line
> contention code, just to get reliable backtraces.
>
> kdb has workarounds for backtracing through ia64 contended locks when
> the kernel is built with older versions of gcc.  gdb (and hence kgdb)
> has no idea about the special out of line code.  Mind you, the same is
> true right now with the out of line i386 code, you need special
> heuristics to backtrace the existing spinlock code reliably.  That will
> only get worse with Zwane's patch, interrupts can now occur in the out
> of line code.
>
> Are you are planning to consolidate the out of line code for i386?  Is
> there a patch (even work in progress) so I can start thinking about
> doing reliable backtraces?

Another problem i ran into was profiling contended locks. Here is a patch
(against 2.6.8-rc2, should apply to recent -mm) which does completely out
of line spinlocks on i386 with shared lock text, it's stable and has been
tested on 32x. This patch can be modified so that we have the entire
spinlock text out of line or simply during contention, the former has
significantly more space savings, something like;

   text    data     bss     dec     hex filename
5437496  831862  323792 6593150  649a7e vmlinux-ool
5449254  834525  323792 6607571  64d2d3 vmlinux-before
5431258  831862  323792 6586912  648220 vmlinux-cool

-ool is only the contended path out of line in shared text, -before is
mainline and -cool is completely out of line shared spinlock text.

Index: linux-2.6.8-rc2/arch/i386/kernel/i386_ksyms.c
===================================================================
RCS file: /home/cvsroot/linux-2.6.8-rc2/arch/i386/kernel/i386_ksyms.c,v
retrieving revision 1.1.1.1
diff -u -p -B -r1.1.1.1 i386_ksyms.c
--- linux-2.6.8-rc2/arch/i386/kernel/i386_ksyms.c	20 Jul 2004 18:14:54 -0000	1.1.1.1
+++ linux-2.6.8-rc2/arch/i386/kernel/i386_ksyms.c	28 Jul 2004 00:29:05 -0000
@@ -49,6 +49,14 @@ EXPORT_SYMBOL(default_idle);
 #ifdef CONFIG_SMP
 extern void FASTCALL( __write_lock_failed(rwlock_t *rw));
 extern void FASTCALL( __read_lock_failed(rwlock_t *rw));
+extern void asmlinkage __spin_lock_failed(spinlock_t *);
+extern void asmlinkage __spin_lock_failed_flags(spinlock_t *, unsigned long);
+extern void asmlinkage __spin_lock_loop(spinlock_t *);
+extern void asmlinkage __spin_lock_loop_flags(spinlock_t *, unsigned long);
+EXPORT_SYMBOL(__spin_lock_failed);
+EXPORT_SYMBOL(__spin_lock_failed_flags);
+EXPORT_SYMBOL(__spin_lock_loop);
+EXPORT_SYMBOL(__spin_lock_loop_flags);
 #endif

 #if defined(CONFIG_BLK_DEV_IDE) || defined(CONFIG_BLK_DEV_HD) || defined(CONFIG_BLK_DEV_IDE_MODULE) || defined(CONFIG_BLK_DEV_HD_MODULE)
Index: linux-2.6.8-rc2/arch/i386/lib/Makefile
===================================================================
RCS file: /home/cvsroot/linux-2.6.8-rc2/arch/i386/lib/Makefile,v
retrieving revision 1.1.1.1
diff -u -p -B -r1.1.1.1 Makefile
--- linux-2.6.8-rc2/arch/i386/lib/Makefile	20 Jul 2004 18:14:54 -0000	1.1.1.1
+++ linux-2.6.8-rc2/arch/i386/lib/Makefile	28 Jul 2004 00:29:05 -0000
@@ -6,5 +6,6 @@
 lib-y = checksum.o delay.o usercopy.o getuser.o memcpy.o strstr.o \
 	bitops.o

+lib-$(CONFIG_SMP) += spinlock.o
 lib-$(CONFIG_X86_USE_3DNOW) += mmx.o
 lib-$(CONFIG_HAVE_DEC_LOCK) += dec_and_lock.o
Index: linux-2.6.8-rc2/arch/i386/lib/spinlock.c
===================================================================
RCS file: linux-2.6.8-rc2/arch/i386/lib/spinlock.c
diff -N linux-2.6.8-rc2/arch/i386/lib/spinlock.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ linux-2.6.8-rc2/arch/i386/lib/spinlock.c	28 Jul 2004 00:29:05 -0000
@@ -0,0 +1,53 @@
+#define PROC(name)	\
+	".align 4\n" \
+	".globl " #name"\n" \
+	#name":\n"
+
+asm (PROC(__spin_lock_failed_flags)
+	"testl $0x200, %ebx\n"
+	"jz 1f\n"
+	"sti\n"
+	"1:\n\t"
+	"rep; nop\n"
+	"cmpb $0, (%eax)\n"
+	"jle 1b\n"
+	"cli\n"
+	"lock; decb (%eax)\n\t"
+	"js __spin_lock_failed_flags\n\t"
+	"ret\n"
+);
+
+asm (PROC(__spin_lock_loop_flags)
+	"lock; decb (%eax)\n\t"
+	"js 1f\n\t"
+	"ret\n\t"
+	"1:\n\t"
+	"testl $0x200, %ebx\n\t"
+	"jz 1f\n\t"
+	"sti\n\t"
+	"2: rep; nop\n\t"
+	"cmpb $0, (%eax)\n\t"
+	"jle 2b\n\t"
+	"cli\n\t"
+	"jmp __spin_lock_loop_flags\n\t"
+);
+
+asm (PROC(__spin_lock_failed)
+	"rep; nop\n\t"
+	"cmpb $0, (%eax)\n\t"
+	"jle __spin_lock_failed\n\t"
+	"lock; decb (%eax)\n\t"
+	"js __spin_lock_failed\n\t"
+	"ret\n\t"
+);
+
+asm (PROC(__spin_lock_loop)
+	"lock; decb (%eax)\n\t"
+	"js 1f\n\t"
+	"ret\n\t"
+	"1: rep; nop\n\t"
+	"cmpb $0, (%eax)\n\t"
+	"jle 1b\n\t"
+	"jmp __spin_lock_loop\n\t"
+);
+
Index: linux-2.6.8-rc2/include/asm-i386/spinlock.h
===================================================================
RCS file: /home/cvsroot/linux-2.6.8-rc2/include/asm-i386/spinlock.h,v
retrieving revision 1.1.1.1
diff -u -p -B -r1.1.1.1 spinlock.h
--- linux-2.6.8-rc2/include/asm-i386/spinlock.h	20 Jul 2004 18:15:14 -0000	1.1.1.1
+++ linux-2.6.8-rc2/include/asm-i386/spinlock.h	28 Jul 2004 00:37:44 -0000
@@ -43,34 +43,25 @@ typedef struct {
 #define spin_is_locked(x)	(*(volatile signed char *)(&(x)->lock) <= 0)
 #define spin_unlock_wait(x)	do { barrier(); } while(spin_is_locked(x))

-#define spin_lock_string \
-	"\n1:\t" \
-	"lock ; decb %0\n\t" \
-	"js 2f\n" \
-	LOCK_SECTION_START("") \
-	"2:\t" \
-	"rep;nop\n\t" \
-	"cmpb $0,%0\n\t" \
-	"jle 2b\n\t" \
-	"jmp 1b\n" \
-	LOCK_SECTION_END
-
-#define spin_lock_string_flags \
-	"\n1:\t" \
-	"lock ; decb %0\n\t" \
-	"js 2f\n\t" \
-	LOCK_SECTION_START("") \
-	"2:\t" \
-	"testl $0x200, %1\n\t" \
-	"jz 3f\n\t" \
-	"sti\n\t" \
-	"3:\t" \
-	"rep;nop\n\t" \
-	"cmpb $0, %0\n\t" \
-	"jle 3b\n\t" \
-	"cli\n\t" \
-	"jmp 1b\n" \
-	LOCK_SECTION_END
+#if 0
+	#define spin_lock_string \
+		"lock ; decb (%0)\n\t" \
+		"jns 1f\n\t" \
+		"call __spin_lock_failed\n\t" \
+		"1:\n\t"
+
+	#define spin_lock_string_flags \
+		"lock ; decb (%0)\n\t" \
+		"jns 1f\n\t" \
+		"call __spin_lock_failed_flags\n\t" \
+		"1:\n\t"
+#else
+	#define spin_lock_string \
+		"call __spin_lock_loop\n\t"
+
+	#define spin_lock_string_flags \
+		"call __spin_lock_loop_flags\n\t"
+#endif

 /*
  * This works. Despite all the confusion.
@@ -139,7 +130,7 @@ here:
 #endif
 	__asm__ __volatile__(
 		spin_lock_string
-		:"=m" (lock->lock) : : "memory");
+		: : "a" (&lock->lock) : "memory");
 }

 static inline void _raw_spin_lock_flags (spinlock_t *lock, unsigned long flags)
@@ -154,7 +145,7 @@ here:
 #endif
 	__asm__ __volatile__(
 		spin_lock_string_flags
-		:"=m" (lock->lock) : "r" (flags) : "memory");
+		: : "a" (&lock->lock), "b" (flags) : "memory");
 }

 /*

Re: [PATCH][2.6] Allow x86_64 to reenable interrupts on contention

[Zwane Mwaikambo]

On Tue, 27 Jul 2004, Andi Kleen wrote:

> > the reduced interrupt latency, the code size issue can also be taken care
> > of, but that requires benchmarking as the change is a bit more drastic.
>
> Do you have numbers on that? Frankly if someone is spinning on irq disabled
> locks for a long time they should just fix their code.

It wasn't i who saw the contention but something Keith ran into, the
contention probably wasn't bad enough to require serious locking changes.

Re: [PATCH][2.6] Allow x86_64 to reenable interrupts on contention

[Keith Owens]

On Tue, 27 Jul 2004 17:40:30 -0700, 
William Lee Irwin III <wli@holomorphy.com> wrote:
>It's good to know there is a precedent and that the backtrace issue has
>been looked at on other architectures.
>
>On Wed, Jul 28, 2004 at 10:35:08AM +1000, Keith Owens wrote:
>> Are you are planning to consolidate the out of line code for i386?  Is
>> there a patch (even work in progress) so I can start thinking about
>> doing reliable backtraces?
>
>The experiments were carried out using the standard calling convention.
>We may investigate less standard calling conventions, but they should
>actually already be there given __write_lock_failed/__read_lock_failed.
>i.e. if reliable backtraces are going to be an issue they should
>already be an issue for rwlocks.

rwlocks are already a issue on i386, but not a big one.  The return
address from __write_lock_failed/__read_lock_failed is pointing into
the out of line code, not the code that really took the rwlock.  The
nearest label is LOCK_SECTION_NAME, i.e. .text.lock.KBUILD_BASENAME.  A
backtrace through a contended rwlock on i386 looks like this

  interrupt
  __write_lock_failed/__read_lock_failed
  .text.lock.KBUILD_BASENAME
  caller of routine that took the lock
  ...

when it should really be

  interrupt
  __write_lock_failed/__read_lock_failed
  .text.lock.KBUILD_BASENAME
  routine that took the look  <=== missing information
  caller of routine that took the lock
  ...

IOW we only get the name of the object, not the function within the
object that took the lock.  i386 gets away with this because
.text.lock.KBUILD_BASENAME is usually enough information to determine
which lock is the problem, and the i386 backtrace algorithm has enough
redundancy to get back in sync for the rest of the trace, even with the
missing function entry.

OTOH, ia64 unwind is incredibly sensitive to the exact instruction
pointer and there is zero redundancy in the unwind data.  If the return
ip is not known to the unwind code, then the ia64 unwinder cannot
backtrace correctly.  Which meant that I had to get the ia64 out of
line code exactly right, close enough was not good enough.

With Zwane's patch, any contended spinlock on i386 will look like
rwsem, it will be missing the routine that took the look.  Good enough
for most cases.

kdb does unwind through out of line spinlock code exactly right, simply
because I added extra heuristics to kdb to cope with this special case.
When people complain that the kdb i386 backtrace code is too messy,
they are really saying that they do not care about getting exact data
for all the hand crafted assembler in the kernel.

BTW, if anybody is planning to switch to dwarf for debugging the i386
kernel then you _must_ have valid dwarf data for the out of line code.

Re: [PATCH][2.6] Allow x86_64 to reenable interrupts on contention

[William Lee Irwin III]

On Wed, Jul 28, 2004 at 11:48:05AM +1000, Keith Owens wrote:
> rwlocks are already a issue on i386, but not a big one.  The return
> address from __write_lock_failed/__read_lock_failed is pointing into
> the out of line code, not the code that really took the rwlock.  The
> nearest label is LOCK_SECTION_NAME, i.e. .text.lock.KBUILD_BASENAME.  A
> backtrace through a contended rwlock on i386 looks like this
>   interrupt
>   __write_lock_failed/__read_lock_failed
>   .text.lock.KBUILD_BASENAME
>   caller of routine that took the lock
>   ...
> when it should really be
>   interrupt
>   __write_lock_failed/__read_lock_failed
>   .text.lock.KBUILD_BASENAME
>   routine that took the look  <=== missing information
>   caller of routine that took the lock
>   ...
> IOW we only get the name of the object, not the function within the
> object that took the lock.  i386 gets away with this because
> .text.lock.KBUILD_BASENAME is usually enough information to determine
> which lock is the problem, and the i386 backtrace algorithm has enough
> redundancy to get back in sync for the rest of the trace, even with the
> missing function entry.

IMHO the out-of-line call to the rwlock contention code in the lock
section is completely worthless bloat and rwlocks should just call the
out-of-line contention case directly from the inline path.

The patch I used to determine the maximum aggregate text size
reduction achievable literally made normal C function entrypoints for
spin_lock(), read_lock(), and all the various spinlocking functions
for spinlocks and rwlocks, moving all of the spinlocking code from
include/linux/spinlock.h to a new kernel/spinlock.c, using the normal
inline _raw_spin_lock()/etc. to avoid unnecessary call frames. This is
what resulted in the aggregate kernel size reduction of 220KB, i.e.
almost 1/4 of 1MB, and had no effect on performance, positive or
negative, in some non-rigorous benchmarking. This obviously
interoperates quite well with backtracing and so on, but doesn't appear
to be under consideration at the moment, and may also have issues with
architectures (e.g. sparc/sparc64) which need the interrupt disablement
in e.g. spin_lock_irqsave() to be done in the same call frame as the
spin_unlock_irqrestore() etc.


On Wed, Jul 28, 2004 at 11:48:05AM +1000, Keith Owens wrote:
> OTOH, ia64 unwind is incredibly sensitive to the exact instruction
> pointer and there is zero redundancy in the unwind data.  If the return
> ip is not known to the unwind code, then the ia64 unwinder cannot
> backtrace correctly.  Which meant that I had to get the ia64 out of
> line code exactly right, close enough was not good enough.
> With Zwane's patch, any contended spinlock on i386 will look like
> rwsem, it will be missing the routine that took the look.  Good enough
> for most cases.
> kdb does unwind through out of line spinlock code exactly right, simply
> because I added extra heuristics to kdb to cope with this special case.
> When people complain that the kdb i386 backtrace code is too messy,
> they are really saying that they do not care about getting exact data
> for all the hand crafted assembler in the kernel.
> BTW, if anybody is planning to switch to dwarf for debugging the i386
> kernel then you _must_ have valid dwarf data for the out of line code.

This is a different issue. That should definitely be looked at for the
consolidated contention case out-of-line strategies now being
considered or others involving nonstandard calling conventions. IMHO we
are truly best off using normal calling conventions to invoke the
out-of-line cases regardless of their role. Confusing debuggers and the
extra bloat to push the return address on the stack and the extra jump
are just not worth it.


-- wli

Re: [PATCH][2.6] Allow x86_64 to reenable interrupts on contention

[David S. Miller]

On Tue, 27 Jul 2004 19:21:31 -0700
William Lee Irwin III <wli@holomorphy.com> wrote:

> and may also have issues with
> architectures (e.g. sparc/sparc64) which need the interrupt disablement
> in e.g. spin_lock_irqsave() to be done in the same call frame as the
> spin_unlock_irqrestore() etc.

This only was a problem on sparc, and it no longer exists at all
in 2.6.x kernels.  It was too much of a pain to keep teaching
people that violated this, and it resulted in some contorted
code as well.

So don't worry about this at all in 2.6.x and later.

Re: [PATCH][2.6] Allow x86_64 to reenable interrupts on contention

[William Lee Irwin III]

On Tue, 27 Jul 2004 19:21:31 -0700 William Lee Irwin III wrote:
>> and may also have issues with
>> architectures (e.g. sparc/sparc64) which need the interrupt disablement
>> in e.g. spin_lock_irqsave() to be done in the same call frame as the
>> spin_unlock_irqrestore() etc.

On Tue, Jul 27, 2004 at 07:24:54PM -0700, David S. Miller wrote:
> This only was a problem on sparc, and it no longer exists at all
> in 2.6.x kernels.  It was too much of a pain to keep teaching
> people that violated this, and it resulted in some contorted
> code as well.
> So don't worry about this at all in 2.6.x and later.

Thanks; that explains whether it worked by coincidence or design.
No idea why I thought sparc64 did similar.


-- wli

Re: [PATCH][2.6] Allow x86_64 to reenable interrupts on contention

[Paul Jackson]

Ricky:
> Need I mention "numcpus"?

Could some kind soul provide a link to that flamage?  It sounds like a
piece of lkml history before my time, but in an area that I work and
should know of.

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373