Re: [PATCH] remove LOCK_SECTION from x86_64 spin_lock asm

[Andi Kleen <ak@suse.de>]

On Thu, Sep 16, 2004 at 08:27:59AM +0200, Ingo Molnar wrote:
> 
> * Andi Kleen <ak@suse.de> wrote:
> 
> > Known problem. Interrupts don't save regs->rbp, but the new profile_pc
> > that was introduced recently uses it.
> > 
> > One quick fix is to just use SAVE_ALL in the interrupt entry code, but
> > I don't like this because it will affect interrupt latency.
> > 
> > The real fix is to fix profile_pc to not reference it.
> 
> it would be nice if we could profile the callers of the spinlock
> functions instead of the opaque spinlock functions.
> 
> the ebp trick is nice, but forcing a formal stack frame for every
> function has global performance implications. Couldnt we define some

I don't think that is needed anyways.  It would seem to 
be overkill to me to make a relatively fast path slower
just for the profiler interrupt.

I think the idea was that the spinlock functions should be small 
enough that they don't have any stack local variables. In this case 
for the standard non FP build you can just use *regs->rsp. The only problem
was with CONFIG_FRAME_POINTER, because then the compiler puts
an additional word onto the stack. I think the right way 
is to just correct for this word and still use rsp.

Here's an untested patch to implement this. Does this fix the problem for 
you?

-Andi


Index: linux/arch/x86_64/kernel/time.c
===================================================================
--- linux.orig/arch/x86_64/kernel/time.c	2004-09-13 22:19:22.%N +0200
+++ linux/arch/x86_64/kernel/time.c	2004-09-16 08:43:06.%N +0200
@@ -184,9 +184,11 @@
 unsigned long profile_pc(struct pt_regs *regs)
 {
 	unsigned long pc = instruction_pointer(regs);
-
+	/* [0] is the frame pointer, [1] is the return address.
+	   This assumes that the spinlock function is small enough
+	   to not have any stack variables. */	
 	if (in_lock_functions(pc))
-		return *(unsigned long *)regs->rbp;
+		return ((unsigned long *)regs->rsp)[1];
 	return pc;
 }
 EXPORT_SYMBOL(profile_pc);