[PATCH][8/8] Arch agnostic completely out of line locks / x86_64

[PATCH][8/8] Arch agnostic completely out of line locks / x86_64

[Zwane Mwaikambo]

 arch/x86_64/kernel/time.c        |   13 +++++++++++++
 arch/x86_64/kernel/vmlinux.lds.S |    1 +
 include/asm-x86_64/ptrace.h      |    4 ++++
 3 files changed, 18 insertions(+)

Andi, i'm not so sure about that return address in profile_pc, i think i
need to read a bit more.

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

Index: linux-2.6.9-rc1-mm1-stage/include/asm-x86_64/ptrace.h
===================================================================
RCS file: /home/cvsroot/linux-2.6.9-rc1-mm1/include/asm-x86_64/ptrace.h,v
retrieving revision 1.1.1.1
diff -u -p -B -r1.1.1.1 ptrace.h
--- linux-2.6.9-rc1-mm1-stage/include/asm-x86_64/ptrace.h	26 Aug 2004 13:13:07 -0000	1.1.1.1
+++ linux-2.6.9-rc1-mm1-stage/include/asm-x86_64/ptrace.h	2 Sep 2004 23:24:05 -0000
@@ -83,7 +83,11 @@ struct pt_regs {
 #if defined(__KERNEL__) && !defined(__ASSEMBLY__)
 #define user_mode(regs) (!!((regs)->cs & 3))
 #define instruction_pointer(regs) ((regs)->rip)
+#if defined(CONFIG_SMP) && defined(CONFIG_FRAME_POINTER)
+extern unsigned long profile_pc(struct pt_regs *regs);
+#else
 #define profile_pc(regs) instruction_pointer(regs)
+#endif
 void signal_fault(struct pt_regs *regs, void __user *frame, char *where);

 enum {
Index: linux-2.6.9-rc1-mm1-stage/arch/x86_64/kernel/time.c
===================================================================
RCS file: /home/cvsroot/linux-2.6.9-rc1-mm1/arch/x86_64/kernel/time.c,v
retrieving revision 1.1.1.1
diff -u -p -B -r1.1.1.1 time.c
--- linux-2.6.9-rc1-mm1-stage/arch/x86_64/kernel/time.c	26 Aug 2004 13:13:06 -0000	1.1.1.1
+++ linux-2.6.9-rc1-mm1-stage/arch/x86_64/kernel/time.c	2 Sep 2004 23:58:08 -0000
@@ -179,6 +179,19 @@ int do_settimeofday(struct timespec *tv)

 EXPORT_SYMBOL(do_settimeofday);

+#if defined(CONFIG_SMP) && defined(CONFIG_FRAME_POINTER)
+unsigned long profile_pc(struct pt_regs *regs)
+{
+	unsigned long pc = instruction_pointer(regs);
+
+	if (pc >= (unsigned long)&__lock_text_start &&
+	    pc <= (unsigned long)&__lock_text_end)
+		return *(unsigned long *)regs->rbp;
+	return pc;
+}
+EXPORT_SYMBOL(profile_pc);
+#endif
+
 /*
  * In order to set the CMOS clock precisely, set_rtc_mmss has to be called 500
  * ms after the second nowtime has started, because when nowtime is written
Index: linux-2.6.9-rc1-mm1-stage/arch/x86_64/kernel/vmlinux.lds.S
===================================================================
RCS file: /home/cvsroot/linux-2.6.9-rc1-mm1/arch/x86_64/kernel/vmlinux.lds.S,v
retrieving revision 1.1.1.1
diff -u -p -B -r1.1.1.1 vmlinux.lds.S
--- linux-2.6.9-rc1-mm1-stage/arch/x86_64/kernel/vmlinux.lds.S	26 Aug 2004 13:13:06 -0000	1.1.1.1
+++ linux-2.6.9-rc1-mm1-stage/arch/x86_64/kernel/vmlinux.lds.S	2 Sep 2004 13:08:15 -0000
@@ -16,6 +16,7 @@ SECTIONS
   .text : {
 	*(.text)
 	SCHED_TEXT
+	LOCK_TEXT
 	*(.fixup)
 	*(.gnu.warning)
 	} = 0x9090

Re: [PATCH][8/8] Arch agnostic completely out of line locks / x86_64

[Andi Kleen]

On Thu, Sep 02, 2004 at 08:03:02PM -0400, Zwane Mwaikambo wrote:
>  arch/x86_64/kernel/time.c        |   13 +++++++++++++
>  arch/x86_64/kernel/vmlinux.lds.S |    1 +
>  include/asm-x86_64/ptrace.h      |    4 ++++
>  3 files changed, 18 insertions(+)
> 
> Andi, i'm not so sure about that return address in profile_pc, i think i
> need to read a bit more.

When frame pointers are enabled the code is correct. But you don't 
even need frame pointers, because the spinlock code should not
spill any registers and in such a function the return address
is always *rsp. Same is true on i386 too. 

-Andi

Re: [PATCH][8/8] Arch agnostic completely out of line locks / x86_64

[Zwane Mwaikambo]

On Sat, 4 Sep 2004, Andi Kleen wrote:

> On Thu, Sep 02, 2004 at 08:03:02PM -0400, Zwane Mwaikambo wrote:
> >  arch/x86_64/kernel/time.c        |   13 +++++++++++++
> >  arch/x86_64/kernel/vmlinux.lds.S |    1 +
> >  include/asm-x86_64/ptrace.h      |    4 ++++
> >  3 files changed, 18 insertions(+)
> >
> > Andi, i'm not so sure about that return address in profile_pc, i think i
> > need to read a bit more.
>
> When frame pointers are enabled the code is correct. But you don't
> even need frame pointers, because the spinlock code should not
> spill any registers and in such a function the return address
> is always *rsp. Same is true on i386 too.

How about the following?

000001f0 <_spin_lock_irqsave>:
 1f0:   55                      push   %ebp
 1f1:   89 e5                   mov    %esp,%ebp
 1f3:   56                      push   %esi
 1f4:   89 c6                   mov    %eax,%esi
 1f6:   53                      push   %ebx
 1f7:   51                      push   %ecx
 1f8:   51                      push   %ecx
 1f9:   9c                      pushf
 1fa:   5b                      pop    %ebx
 1fb:   fa                      cli
 1fc:   b8 00 e0 ff ff          mov    $0xffffe000,%eax
 201:   21 e0                   and    %esp,%eax
 203:   8b 50 14                mov    0x14(%eax),%edx
 206:   42                      inc    %edx

It was a lot easier with the spin stub only out of line (the first round
of patches for i386, x86_64) so there i used esp and didn't depend on
frame pointers.

Thanks,
	Zwane

Re: [PATCH][8/8] Arch agnostic completely out of line locks / x86_64

[Andi Kleen]

On Sat, Sep 04, 2004 at 02:26:12PM -0400, Zwane Mwaikambo wrote:
> On Sat, 4 Sep 2004, Andi Kleen wrote:
> 
> > On Thu, Sep 02, 2004 at 08:03:02PM -0400, Zwane Mwaikambo wrote:
> > >  arch/x86_64/kernel/time.c        |   13 +++++++++++++
> > >  arch/x86_64/kernel/vmlinux.lds.S |    1 +
> > >  include/asm-x86_64/ptrace.h      |    4 ++++
> > >  3 files changed, 18 insertions(+)
> > >
> > > Andi, i'm not so sure about that return address in profile_pc, i think i
> > > need to read a bit more.
> >
> > When frame pointers are enabled the code is correct. But you don't
> > even need frame pointers, because the spinlock code should not
> > spill any registers and in such a function the return address
> > is always *rsp. Same is true on i386 too.
> 
> How about the following?

That is with frame pointers enabled. Indeed with frame pointers
on it is not true you still have to special case that.

But the common case is without frame pointers anyways ... 

-Andi

Re: [PATCH][8/8] Arch agnostic completely out of line locks / x86_64

[Zwane Mwaikambo]

Hi Andi,

On Mon, 6 Sep 2004, Andi Kleen wrote:

> That is with frame pointers enabled. Indeed with frame pointers
> on it is not true you still have to special case that.

Yes that was with frame pointers enabled, but the following was compiled 
without frame pointers, i'm still not sure it's safe to use *esp.

00000070 <_spin_lock>:
  70:   83 ec 04                sub    $0x4,%esp
  73:   89 c2                   mov    %eax,%edx
  75:   b8 00 e0 ff ff          mov    $0xffffe000,%eax
  7a:   21 e0                   and    %esp,%eax
  7c:   ff 40 14                incl   0x14(%eax)
  7f:   31 c0                   xor    %eax,%eax
  81:   86 02                   xchg   %al,(%edx)
  83:   84 c0                   test   %al,%al
  85:   7e 02                   jle    89 <_spin_lock+0x19>
  87:   58                      pop    %eax
  88:   c3                      ret
  89:   89 14 24                mov    %edx,(%esp)
  8c:   e8 fc ff ff ff          call   8d <_spin_lock+0x1d>
  91:   eb f4                   jmp    87 <_spin_lock+0x17>
  93:   8d b6 00 00 00 00       lea    0x0(%esi),%esi
  99:   8d bc 27 00 00 00 00    lea    0x0(%edi),%edi

Thanks,
	Zwane

Re: [PATCH][8/8] Arch agnostic completely out of line locks / x86_64

[Andi Kleen]

On Mon, Sep 06, 2004 at 12:19:24PM -0400, Zwane Mwaikambo wrote:
> Hi Andi,
> 
> On Mon, 6 Sep 2004, Andi Kleen wrote:
> 
> > That is with frame pointers enabled. Indeed with frame pointers
> > on it is not true you still have to special case that.
> 
> Yes that was with frame pointers enabled, but the following was compiled 
> without frame pointers, i'm still not sure it's safe to use *esp.

No, it's not unfortunately. gcc is aligning the stack 
to 8 bytes for floating point. It would if you compiled the file with 
-mpreferred-stack-boundary=4. Actually AFAIK this is only useful
for floating point anyways, so it would be a good idea to always
compile the kernel with this option.

On x86-64 it should just work.

-Andi



> 
> 00000070 <_spin_lock>:
>   70:   83 ec 04                sub    $0x4,%esp

Re: [PATCH][8/8] Arch agnostic completely out of line locks / x86_64

[Zwane Mwaikambo]

On Mon, 6 Sep 2004, Andi Kleen wrote:

> On Mon, Sep 06, 2004 at 12:19:24PM -0400, Zwane Mwaikambo wrote:
> > Hi Andi,
> > 
> > On Mon, 6 Sep 2004, Andi Kleen wrote:
> > 
> > > That is with frame pointers enabled. Indeed with frame pointers
> > > on it is not true you still have to special case that.
> > 
> > Yes that was with frame pointers enabled, but the following was compiled 
> > without frame pointers, i'm still not sure it's safe to use *esp.
> 
> No, it's not unfortunately. gcc is aligning the stack 
> to 8 bytes for floating point. It would if you compiled the file with 
> -mpreferred-stack-boundary=4. Actually AFAIK this is only useful
> for floating point anyways, so it would be a good idea to always
> compile the kernel with this option.

We should give this a go.

> On x86-64 it should just work.

i'll send a patch for that.

Thanks Andi,
	Zwane