2004-09-03 00:07:36

by Zwane Mwaikambo

[permalink] [raw]
Subject: [PATCH][8/8] Arch agnostic completely out of line locks / x86_64

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 <[email protected]>

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


2004-09-04 11:34:27

by Andi Kleen

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

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

2004-09-04 18:21:48

by Zwane Mwaikambo

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

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

2004-09-06 07:35:12

by Andi Kleen

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

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

2004-09-06 16:14:59

by Zwane Mwaikambo

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

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

2004-09-06 17:55:25

by Andi Kleen

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

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

2004-09-07 15:37:15

by Zwane Mwaikambo

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

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