[RFC GIT PULL] perf updates

[RFC GIT PULL] perf updates

[Frederic Weisbecker]

Ingo,

Please pull the perf/core branch that can be found at:

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
	perf/core
	
Soeren reported some time ago that callchains were broken on
x86_64 with cpu-clock/task-clock events. After some time I
finally found the problem was in the irq low level arg saving.
It touches fragile code though, I hope some people can
have a look over the fix.

Thanks,
	Frederic
---

Frederic Weisbecker (2):
      x86: Fix rbp saving in pt_regs on irq entry
      perf: Build tools with frame pointer


 arch/x86/kernel/entry_64.S |   31 +++++++++++++++++++------------
 tools/perf/Makefile        |    2 +-
 2 files changed, 20 insertions(+), 13 deletions(-)

[PATCH 2/2] perf: Build tools with frame pointer

[Frederic Weisbecker]

It seems that some gcc versions build by default with frame pointers
and some others omit them.

Just build the tools with frame pointers as the callchains can be an
important part of the perf workflow.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Stephane Eranian <eranian@google.com>
---
 tools/perf/Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index 1b9b13e..2b5387d 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -227,7 +227,7 @@ ifndef PERF_DEBUG
   CFLAGS_OPTIMIZE = -O6
 endif
 
-CFLAGS = -ggdb3 -Wall -Wextra -std=gnu99 -Werror $(CFLAGS_OPTIMIZE) -D_FORTIFY_SOURCE=2 $(EXTRA_WARNINGS) $(EXTRA_CFLAGS)
+CFLAGS = -fno-omit-frame-pointer -ggdb3 -Wall -Wextra -std=gnu99 -Werror $(CFLAGS_OPTIMIZE) -D_FORTIFY_SOURCE=2 $(EXTRA_WARNINGS) $(EXTRA_CFLAGS)
 EXTLIBS = -lpthread -lrt -lelf -lm
 ALL_CFLAGS = $(CFLAGS) -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64
 ALL_LDFLAGS = $(LDFLAGS)
-- 
1.7.3.2

[tip:perf/core] perf tools: Build with frame pointer

[tip-bot for Frederic Weisbecker]

Commit-ID:  6b01f2c4f6188da50d8fe094e369a9c0390424ab
Gitweb:     http://git.kernel.org/tip/6b01f2c4f6188da50d8fe094e369a9c0390424ab
Author:     Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Thu, 6 Jan 2011 15:51:53 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 6 Jan 2011 18:04:54 -0200

perf tools: Build with frame pointer

It seems that some gcc versions build by default with frame pointers
and some others omit them.

Just build the tools with frame pointers as the callchains can be an
important part of the perf workflow.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Stephane Eranian <eranian@google.com>
LKML-Reference: <1294325513-14276-3-git-send-email-fweisbec@gmail.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index 1b9b13e..2b5387d 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -227,7 +227,7 @@ ifndef PERF_DEBUG
   CFLAGS_OPTIMIZE = -O6
 endif
 
-CFLAGS = -ggdb3 -Wall -Wextra -std=gnu99 -Werror $(CFLAGS_OPTIMIZE) -D_FORTIFY_SOURCE=2 $(EXTRA_WARNINGS) $(EXTRA_CFLAGS)
+CFLAGS = -fno-omit-frame-pointer -ggdb3 -Wall -Wextra -std=gnu99 -Werror $(CFLAGS_OPTIMIZE) -D_FORTIFY_SOURCE=2 $(EXTRA_WARNINGS) $(EXTRA_CFLAGS)
 EXTLIBS = -lpthread -lrt -lelf -lm
 ALL_CFLAGS = $(CFLAGS) -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64
 ALL_LDFLAGS = $(LDFLAGS)

Re: [RFC PATCH 1/2] x86: Fix rbp saving in pt_regs on irq entry

[Jan Beulich]

>>> On 06.01.11 at 15:51, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> From the x86_64 low level interrupt handlers, the frame pointer is
> saved in pt_regs in the wrong place, in the offset of rbx.
> 
> rbx is not part of the irq partial saved registers, so it's not
> critical, but later code that uses get_irq_regs() to get the
> interrupted frame pointer instead get a stale rbx value, causing
> unwinding code to fail miserably.

Code using get_irq_regs() can't rely on the not explicitly
saved registers' fields of pt_regs anyway; you now fix this
for %rbp, but someone else might look at a different
register and want that to be saved too. You just shouldn't,
and the fact that %rbp happens to be saved at all shouldn't
be taken to mean you can access it via the provided
pt_regs pointer. This saving of %rbp could go away or be
done in a different way at any point.

> @@ -808,6 +813,8 @@ ret_from_intr:
>  	TRACE_IRQS_OFF
>  	decl PER_CPU_VAR(irq_count)
>  	leaveq
> +	/* we did not save rbx, restore only from ARGOFFSET */
> +	addq $8, %rsp
>  	CFI_RESTORE		rbp
>  	CFI_DEF_CFA_REGISTER	rsp
>  	CFI_ADJUST_CFA_OFFSET	-8

*If* the patch was to be taken anyway, I would strongly suggest
getting this mis-insertion fixed first: CFI annotations belong to the
immediately preceding instruction, and hence you must not insert
new instructions between an existing one and its annotation.

Furthermore, an adjustment to %rsp (when it serves as the frame
pointer as is the case here, though would be better visible if the
added instruction was at the right place) needs to be annotated
itself.

Jan

Re: [RFC PATCH 1/2] x86: Fix rbp saving in pt_regs on irq entry

[Frederic Weisbecker]

On Thu, Jan 06, 2011 at 03:18:19PM +0000, Jan Beulich wrote:
> >>> On 06.01.11 at 15:51, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > From the x86_64 low level interrupt handlers, the frame pointer is
> > saved in pt_regs in the wrong place, in the offset of rbx.
> > 
> > rbx is not part of the irq partial saved registers, so it's not
> > critical, but later code that uses get_irq_regs() to get the
> > interrupted frame pointer instead get a stale rbx value, causing
> > unwinding code to fail miserably.
> 
> Code using get_irq_regs() can't rely on the not explicitly
> saved registers' fields of pt_regs anyway; you now fix this
> for %rbp, but someone else might look at a different
> register and want that to be saved too. You just shouldn't,
> and the fact that %rbp happens to be saved at all shouldn't
> be taken to mean you can access it via the provided
> pt_regs pointer. This saving of %rbp could go away or be
> done in a different way at any point.

Right. That was something I was wondering about: is rbp saved here
as the beginning of the proc stack, or was is supposed to be
part of the pt_regs although mistakenly recorded in rbp?

So given what you are explaining me, it rather seem it wasn't supposed
to be of part pt_regs and it's there because it begins the frame of the irq
handler.

I agree that in the case of common irqs, rbp is not supposed to be part
of saved regs. I think may be we can change that semantic though as it
requires very few changes. In fact the only added overhead is that addq
below.

I'm waiting for opinions though.

 
> > @@ -808,6 +813,8 @@ ret_from_intr:
> >  	TRACE_IRQS_OFF
> >  	decl PER_CPU_VAR(irq_count)
> >  	leaveq
> > +	/* we did not save rbx, restore only from ARGOFFSET */
> > +	addq $8, %rsp
> >  	CFI_RESTORE		rbp
> >  	CFI_DEF_CFA_REGISTER	rsp
> >  	CFI_ADJUST_CFA_OFFSET	-8
> 
> *If* the patch was to be taken anyway, I would strongly suggest
> getting this mis-insertion fixed first: CFI annotations belong to the
> immediately preceding instruction, and hence you must not insert
> new instructions between an existing one and its annotation.

Good to know, will fix.

> Furthermore, an adjustment to %rsp (when it serves as the frame
> pointer as is the case here, though would be better visible if the
> added instruction was at the right place) needs to be annotated
> itself.


Hmm, I'm not familiar with those CFI adjustments.
Before we had:


	leaveq

	CFI_RESTORE             rbp
	CFI_DEF_CFA_REGISTER    rsp
	CFI_ADJUST_CFA_OFFSET   -8

So CFI_RESTORE means rbp has now the value of the base frame of
the calling frame (the base frame pointer of the interrupted proc) ?

And what follows means that rsp-8 points to the return address?
But it doesn't seem to be the case, the return address here beeing
rip stored in pt_regs...

I'm confused.

Re: [RFC PATCH 1/2] x86: Fix rbp saving in pt_regs on irq entry

[Jan Beulich]

>>> On 06.01.11 at 16:45, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> Before we had:
> 
> 
> 	leaveq
> 
> 	CFI_RESTORE             rbp
> 	CFI_DEF_CFA_REGISTER    rsp
> 	CFI_ADJUST_CFA_OFFSET   -8
> 
> So CFI_RESTORE means rbp has now the value of the base frame of
> the calling frame (the base frame pointer of the interrupted proc) ?

No - all it means is that %rbp now has its original (caller or
interrupted procedure) value again (i.e. an unwinder should not
try to read it from the stack [or other previously recorded
location] anymore).

> And what follows means that rsp-8 points to the return address?

No - .cfi_def_cfa_register says which register serves as the frame
pointer, and .cfi_adjust_cfa_offset says to adjust the offset from
the frame pointer to the top [or bottom] of frame. At any time

	CFA = cfa_register + cfa_offset

with CFA being what all locations on the stack are expressed
relative to.

Jan

Re: [RFC PATCH 1/2] x86: Fix rbp saving in pt_regs on irq entry

[Frederic Weisbecker]

On Thu, Jan 06, 2011 at 04:10:55PM +0000, Jan Beulich wrote:
> >>> On 06.01.11 at 16:45, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > Before we had:
> > 
> > 
> > 	leaveq
> > 
> > 	CFI_RESTORE             rbp
> > 	CFI_DEF_CFA_REGISTER    rsp
> > 	CFI_ADJUST_CFA_OFFSET   -8
> > 
> > So CFI_RESTORE means rbp has now the value of the base frame of
> > the calling frame (the base frame pointer of the interrupted proc) ?
> 
> No - all it means is that %rbp now has its original (caller or
> interrupted procedure) value again (i.e. an unwinder should not
> try to read it from the stack [or other previously recorded
> location] anymore).
> 
> > And what follows means that rsp-8 points to the return address?
> 
> No - .cfi_def_cfa_register says which register serves as the frame
> pointer, and .cfi_adjust_cfa_offset says to adjust the offset from
> the frame pointer to the top [or bottom] of frame. At any time
> 
> 	CFA = cfa_register + cfa_offset
> 
> with CFA being what all locations on the stack are expressed
> relative to.

Ok.

So here rsp points to pt_regs::r11

I don't understand why locations relative to the stack must be
expressed here by taking rsp - 8 as a base.

Re: [RFC PATCH 1/2] x86: Fix rbp saving in pt_regs on irq entry

[Jan Beulich]

>>> On 06.01.11 at 17:22, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> On Thu, Jan 06, 2011 at 04:10:55PM +0000, Jan Beulich wrote:
>> >>> On 06.01.11 at 16:45, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>> > Before we had:
>> > 
>> > 
>> > 	leaveq
>> > 
>> > 	CFI_RESTORE             rbp
>> > 	CFI_DEF_CFA_REGISTER    rsp
>> > 	CFI_ADJUST_CFA_OFFSET   -8
>> > 
>> > So CFI_RESTORE means rbp has now the value of the base frame of
>> > the calling frame (the base frame pointer of the interrupted proc) ?
>> 
>> No - all it means is that %rbp now has its original (caller or
>> interrupted procedure) value again (i.e. an unwinder should not
>> try to read it from the stack [or other previously recorded
>> location] anymore).
>> 
>> > And what follows means that rsp-8 points to the return address?
>> 
>> No - .cfi_def_cfa_register says which register serves as the frame
>> pointer, and .cfi_adjust_cfa_offset says to adjust the offset from
>> the frame pointer to the top [or bottom] of frame. At any time
>> 
>> 	CFA = cfa_register + cfa_offset
>> 
>> with CFA being what all locations on the stack are expressed
>> relative to.
> 
> Ok.
> 
> So here rsp points to pt_regs::r11
> 
> I don't understand why locations relative to the stack must be
> expressed here by taking rsp - 8 as a base.

Nothing says rsp-8. The annotations merely say to set the base
register to rsp and to *adjust* the offset by -8 (after all, that's
what the leaveq instruction does).

Jan

Re: [RFC PATCH 1/2] x86: Fix rbp saving in pt_regs on irq entry

[Frederic Weisbecker]

On Thu, Jan 06, 2011 at 04:39:39PM +0000, Jan Beulich wrote:
> >>> On 06.01.11 at 17:22, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > On Thu, Jan 06, 2011 at 04:10:55PM +0000, Jan Beulich wrote:
> >> >>> On 06.01.11 at 16:45, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> >> > Before we had:
> >> > 
> >> > 
> >> > 	leaveq
> >> > 
> >> > 	CFI_RESTORE             rbp
> >> > 	CFI_DEF_CFA_REGISTER    rsp
> >> > 	CFI_ADJUST_CFA_OFFSET   -8
> >> > 
> >> > So CFI_RESTORE means rbp has now the value of the base frame of
> >> > the calling frame (the base frame pointer of the interrupted proc) ?
> >> 
> >> No - all it means is that %rbp now has its original (caller or
> >> interrupted procedure) value again (i.e. an unwinder should not
> >> try to read it from the stack [or other previously recorded
> >> location] anymore).
> >> 
> >> > And what follows means that rsp-8 points to the return address?
> >> 
> >> No - .cfi_def_cfa_register says which register serves as the frame
> >> pointer, and .cfi_adjust_cfa_offset says to adjust the offset from
> >> the frame pointer to the top [or bottom] of frame. At any time
> >> 
> >> 	CFA = cfa_register + cfa_offset
> >> 
> >> with CFA being what all locations on the stack are expressed
> >> relative to.
> > 
> > Ok.
> > 
> > So here rsp points to pt_regs::r11
> > 
> > I don't understand why locations relative to the stack must be
> > expressed here by taking rsp - 8 as a base.
> 
> Nothing says rsp-8. The annotations merely say to set the base
> register to rsp and to *adjust* the offset by -8 (after all, that's
> what the leaveq instruction does).

Ah! So CFA acts like a virtual frame base pointer right?

Re: [RFC PATCH 1/2] x86: Fix rbp saving in pt_regs on irq entry

[Jan Beulich]

>>> On 06.01.11 at 17:54, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> On Thu, Jan 06, 2011 at 04:39:39PM +0000, Jan Beulich wrote:
>> >>> On 06.01.11 at 17:22, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>> > On Thu, Jan 06, 2011 at 04:10:55PM +0000, Jan Beulich wrote:
>> >> >>> On 06.01.11 at 16:45, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>> >> > Before we had:
>> >> > 
>> >> > 
>> >> > 	leaveq
>> >> > 
>> >> > 	CFI_RESTORE             rbp
>> >> > 	CFI_DEF_CFA_REGISTER    rsp
>> >> > 	CFI_ADJUST_CFA_OFFSET   -8
>> >> > 
>> >> > So CFI_RESTORE means rbp has now the value of the base frame of
>> >> > the calling frame (the base frame pointer of the interrupted proc) ?
>> >> 
>> >> No - all it means is that %rbp now has its original (caller or
>> >> interrupted procedure) value again (i.e. an unwinder should not
>> >> try to read it from the stack [or other previously recorded
>> >> location] anymore).
>> >> 
>> >> > And what follows means that rsp-8 points to the return address?
>> >> 
>> >> No - .cfi_def_cfa_register says which register serves as the frame
>> >> pointer, and .cfi_adjust_cfa_offset says to adjust the offset from
>> >> the frame pointer to the top [or bottom] of frame. At any time
>> >> 
>> >> 	CFA = cfa_register + cfa_offset
>> >> 
>> >> with CFA being what all locations on the stack are expressed
>> >> relative to.
>> > 
>> > Ok.
>> > 
>> > So here rsp points to pt_regs::r11
>> > 
>> > I don't understand why locations relative to the stack must be
>> > expressed here by taking rsp - 8 as a base.
>> 
>> Nothing says rsp-8. The annotations merely say to set the base
>> register to rsp and to *adjust* the offset by -8 (after all, that's
>> what the leaveq instruction does).
> 
> Ah! So CFA acts like a virtual frame base pointer right?

Correct.

Re: [RFC PATCH 1/2] x86: Fix rbp saving in pt_regs on irq entry

[Frederic Weisbecker]

On Thu, Jan 06, 2011 at 04:58:54PM +0000, Jan Beulich wrote:
> >>> On 06.01.11 at 17:54, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > On Thu, Jan 06, 2011 at 04:39:39PM +0000, Jan Beulich wrote:
> >> >>> On 06.01.11 at 17:22, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> >> > On Thu, Jan 06, 2011 at 04:10:55PM +0000, Jan Beulich wrote:
> >> >> >>> On 06.01.11 at 16:45, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> >> >> > Before we had:
> >> >> > 
> >> >> > 
> >> >> > 	leaveq
> >> >> > 
> >> >> > 	CFI_RESTORE             rbp
> >> >> > 	CFI_DEF_CFA_REGISTER    rsp
> >> >> > 	CFI_ADJUST_CFA_OFFSET   -8
> >> >> > 
> >> >> > So CFI_RESTORE means rbp has now the value of the base frame of
> >> >> > the calling frame (the base frame pointer of the interrupted proc) ?
> >> >> 
> >> >> No - all it means is that %rbp now has its original (caller or
> >> >> interrupted procedure) value again (i.e. an unwinder should not
> >> >> try to read it from the stack [or other previously recorded
> >> >> location] anymore).
> >> >> 
> >> >> > And what follows means that rsp-8 points to the return address?
> >> >> 
> >> >> No - .cfi_def_cfa_register says which register serves as the frame
> >> >> pointer, and .cfi_adjust_cfa_offset says to adjust the offset from
> >> >> the frame pointer to the top [or bottom] of frame. At any time
> >> >> 
> >> >> 	CFA = cfa_register + cfa_offset
> >> >> 
> >> >> with CFA being what all locations on the stack are expressed
> >> >> relative to.
> >> > 
> >> > Ok.
> >> > 
> >> > So here rsp points to pt_regs::r11
> >> > 
> >> > I don't understand why locations relative to the stack must be
> >> > expressed here by taking rsp - 8 as a base.
> >> 
> >> Nothing says rsp-8. The annotations merely say to set the base
> >> register to rsp and to *adjust* the offset by -8 (after all, that's
> >> what the leaveq instruction does).
> > 
> > Ah! So CFA acts like a virtual frame base pointer right?
> 
> Correct.

Ah great. I was starting to prepare for the case you come to stab me :)

So what do you think about that:

        leaveq

        CFI_RESTORE             rbp
        CFI_DEF_CFA_REGISTER    rsp
        CFI_ADJUST_CFA_OFFSET   -8

       /* we did not save rbx, restore only from ARGOFFSET */
       addq $8, %rsp
       CFI_ADJUST_CFA_OFFSET   -16


Does that look correct to you? We increased rsp to start recovering
the regs from the right place, but the frame pointer of the current
proc must stay what it was.

Now I don't understand how this is all useful as this is not a normal
proc but an interruption. We can't get back the return address from
the CFA. Or am I missing something?

Re: [RFC PATCH 1/2] x86: Fix rbp saving in pt_regs on irq entry

[Frederic Weisbecker]

On Thu, Jan 06, 2011 at 06:12:33PM +0100, Frederic Weisbecker wrote:
> On Thu, Jan 06, 2011 at 04:58:54PM +0000, Jan Beulich wrote:
> > >>> On 06.01.11 at 17:54, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > > On Thu, Jan 06, 2011 at 04:39:39PM +0000, Jan Beulich wrote:
> > >> >>> On 06.01.11 at 17:22, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > >> > On Thu, Jan 06, 2011 at 04:10:55PM +0000, Jan Beulich wrote:
> > >> >> >>> On 06.01.11 at 16:45, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > >> >> > Before we had:
> > >> >> > 
> > >> >> > 
> > >> >> > 	leaveq
> > >> >> > 
> > >> >> > 	CFI_RESTORE             rbp
> > >> >> > 	CFI_DEF_CFA_REGISTER    rsp
> > >> >> > 	CFI_ADJUST_CFA_OFFSET   -8
> > >> >> > 
> > >> >> > So CFI_RESTORE means rbp has now the value of the base frame of
> > >> >> > the calling frame (the base frame pointer of the interrupted proc) ?
> > >> >> 
> > >> >> No - all it means is that %rbp now has its original (caller or
> > >> >> interrupted procedure) value again (i.e. an unwinder should not
> > >> >> try to read it from the stack [or other previously recorded
> > >> >> location] anymore).
> > >> >> 
> > >> >> > And what follows means that rsp-8 points to the return address?
> > >> >> 
> > >> >> No - .cfi_def_cfa_register says which register serves as the frame
> > >> >> pointer, and .cfi_adjust_cfa_offset says to adjust the offset from
> > >> >> the frame pointer to the top [or bottom] of frame. At any time
> > >> >> 
> > >> >> 	CFA = cfa_register + cfa_offset
> > >> >> 
> > >> >> with CFA being what all locations on the stack are expressed
> > >> >> relative to.
> > >> > 
> > >> > Ok.
> > >> > 
> > >> > So here rsp points to pt_regs::r11
> > >> > 
> > >> > I don't understand why locations relative to the stack must be
> > >> > expressed here by taking rsp - 8 as a base.
> > >> 
> > >> Nothing says rsp-8. The annotations merely say to set the base
> > >> register to rsp and to *adjust* the offset by -8 (after all, that's
> > >> what the leaveq instruction does).
> > > 
> > > Ah! So CFA acts like a virtual frame base pointer right?
> > 
> > Correct.
> 
> Ah great. I was starting to prepare for the case you come to stab me :)
> 
> So what do you think about that:
> 
>         leaveq
> 
>         CFI_RESTORE             rbp
>         CFI_DEF_CFA_REGISTER    rsp
>         CFI_ADJUST_CFA_OFFSET   -8
> 
>        /* we did not save rbx, restore only from ARGOFFSET */
>        addq $8, %rsp
>        CFI_ADJUST_CFA_OFFSET   -16

Or if CFI_ADJUST_CFA_OFFSET is already relative to its previous value,
it should be CFI_ADJUST_CFA_OFFSET -8

Re: [RFC PATCH 1/2] x86: Fix rbp saving in pt_regs on irq entry

[Jan Beulich]

>>> On 06.01.11 at 18:12, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> On Thu, Jan 06, 2011 at 04:58:54PM +0000, Jan Beulich wrote:
>> >>> On 06.01.11 at 17:54, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>> > On Thu, Jan 06, 2011 at 04:39:39PM +0000, Jan Beulich wrote:
>> >> >>> On 06.01.11 at 17:22, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>> >> > On Thu, Jan 06, 2011 at 04:10:55PM +0000, Jan Beulich wrote:
>> >> >> >>> On 06.01.11 at 16:45, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>> >> >> > Before we had:
>> >> >> > 
>> >> >> > 
>> >> >> > 	leaveq
>> >> >> > 
>> >> >> > 	CFI_RESTORE             rbp
>> >> >> > 	CFI_DEF_CFA_REGISTER    rsp
>> >> >> > 	CFI_ADJUST_CFA_OFFSET   -8
>> >> >> > 
>> >> >> > So CFI_RESTORE means rbp has now the value of the base frame of
>> >> >> > the calling frame (the base frame pointer of the interrupted proc) ?
>> >> >> 
>> >> >> No - all it means is that %rbp now has its original (caller or
>> >> >> interrupted procedure) value again (i.e. an unwinder should not
>> >> >> try to read it from the stack [or other previously recorded
>> >> >> location] anymore).
>> >> >> 
>> >> >> > And what follows means that rsp-8 points to the return address?
>> >> >> 
>> >> >> No - .cfi_def_cfa_register says which register serves as the frame
>> >> >> pointer, and .cfi_adjust_cfa_offset says to adjust the offset from
>> >> >> the frame pointer to the top [or bottom] of frame. At any time
>> >> >> 
>> >> >> 	CFA = cfa_register + cfa_offset
>> >> >> 
>> >> >> with CFA being what all locations on the stack are expressed
>> >> >> relative to.
>> >> > 
>> >> > Ok.
>> >> > 
>> >> > So here rsp points to pt_regs::r11
>> >> > 
>> >> > I don't understand why locations relative to the stack must be
>> >> > expressed here by taking rsp - 8 as a base.
>> >> 
>> >> Nothing says rsp-8. The annotations merely say to set the base
>> >> register to rsp and to *adjust* the offset by -8 (after all, that's
>> >> what the leaveq instruction does).
>> > 
>> > Ah! So CFA acts like a virtual frame base pointer right?
>> 
>> Correct.
> 
> Ah great. I was starting to prepare for the case you come to stab me :)
> 
> So what do you think about that:
> 
>         leaveq
> 
>         CFI_RESTORE             rbp
>         CFI_DEF_CFA_REGISTER    rsp
>         CFI_ADJUST_CFA_OFFSET   -8
> 
>        /* we did not save rbx, restore only from ARGOFFSET */
>        addq $8, %rsp
>        CFI_ADJUST_CFA_OFFSET   -16
> 
> 
> Does that look correct to you? We increased rsp to start recovering
> the regs from the right place, but the frame pointer of the current
> proc must stay what it was.

As you hinted in your subsequent reply - it's -8 here (that's
why the directive is named *adjust*; there are other
directives allowing to *set* an offset).

> Now I don't understand how this is all useful as this is not a normal
> proc but an interruption. We can't get back the return address from
> the CFA. Or am I missing something?

Unwind annotations, when written correctly, allow unwinding
through all kinds of execution flows, including interrupts or
exceptions as well as including stack switches.

Jan

Re: [RFC PATCH 1/2] x86: Fix rbp saving in pt_regs on irq entry

[Ingo Molnar]

* Jan Beulich <JBeulich@novell.com> wrote:

> > Now I don't understand how this is all useful as this is not a normal proc but 
> > an interruption. We can't get back the return address from the CFA. Or am I 
> > missing something?
> 
> Unwind annotations, when written correctly, allow unwinding through all kinds of 
> execution flows, including interrupts or exceptions as well as including stack 
> switches.

Yeah and that's rather useful, as exception contexts can nest in very weird ways, 
especially with NMIs involved. For example a 7-context combination is possible:

  user-space -> syscall -> pagefault -> softirq -> hardirq -> debug trap -> NMI

And the call frame walking logic needs to be able to get all the way back to 
user-space ...

For that every transition needs to work flawlessly, for debugging (and CFI based 
profiling) to work fine.

Most of those transitions can happen at any instruction boundary that a given 
context executes, so the total number of possible combinations is virtually endless.

Unfortunately we dont seem to have a good way to test any of this automatically. 
Putting a perf probe on every assembly instruction perhaps, and checking whether the 
frame manages to go back all the way to user-space?

Thanks,

	Ingo

Re: [RFC PATCH 1/2] x86: Fix rbp saving in pt_regs on irq entry

[Frederic Weisbecker]

On Fri, Jan 07, 2011 at 01:31:30PM +0100, Ingo Molnar wrote:
> 
> * Jan Beulich <JBeulich@novell.com> wrote:
> 
> > > Now I don't understand how this is all useful as this is not a normal proc but 
> > > an interruption. We can't get back the return address from the CFA. Or am I 
> > > missing something?
> > 
> > Unwind annotations, when written correctly, allow unwinding through all kinds of 
> > execution flows, including interrupts or exceptions as well as including stack 
> > switches.
> 
> Yeah and that's rather useful, as exception contexts can nest in very weird ways, 
> especially with NMIs involved. For example a 7-context combination is possible:
> 
>   user-space -> syscall -> pagefault -> softirq -> hardirq -> debug trap -> NMI
> 
> And the call frame walking logic needs to be able to get all the way back to 
> user-space ...
> 
> For that every transition needs to work flawlessly, for debugging (and CFI based 
> profiling) to work fine.
> 
> Most of those transitions can happen at any instruction boundary that a given 
> context executes, so the total number of possible combinations is virtually endless.
> 
> Unfortunately we dont seem to have a good way to test any of this automatically. 
> Putting a perf probe on every assembly instruction perhaps, and checking whether the 
> frame manages to go back all the way to user-space?

May be.

Once I'll have perf callchain based on CFI ready, we'll perhaps find some issues
there. Although I guess there are already tools that can make use of that.

> 
> Thanks,
> 
> 	Ingo

Re: [RFC PATCH 1/2] x86: Fix rbp saving in pt_regs on irq entry

[Jan Beulich]

>>> On 07.01.11 at 17:05, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> Once I'll have perf callchain based on CFI ready, we'll perhaps find some 
> issues
> there. Although I guess there are already tools that can make use of that.

Is this to read that you're planning to do a re-spin of the CFI
unwinding code (which I'm not allowed to submit another time,
but which we've been using for years in SuSE distros) then?

Jan

Re: [RFC PATCH 1/2] x86: Fix rbp saving in pt_regs on irq entry

[Frederic Weisbecker]

On Fri, Jan 07, 2011 at 04:13:58PM +0000, Jan Beulich wrote:
> >>> On 07.01.11 at 17:05, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > Once I'll have perf callchain based on CFI ready, we'll perhaps find some 
> > issues
> > there. Although I guess there are already tools that can make use of that.
> 
> Is this to read that you're planning to do a re-spin of the CFI
> unwinding code (which I'm not allowed to submit another time,
> but which we've been using for years in SuSE distros) then?

An in-kernel CFI unwinder?

No the intended CFI unwinding that I'm working on for perf is made
on post-processing, on top of partial stack and regs snapshots.

The true unwinding is computed in userspace.

Re: [RFC PATCH 1/2] x86: Fix rbp saving in pt_regs on irq entry

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> On Fri, Jan 07, 2011 at 04:13:58PM +0000, Jan Beulich wrote:
> > >>> On 07.01.11 at 17:05, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > > Once I'll have perf callchain based on CFI ready, we'll perhaps find some 
> > > issues
> > > there. Although I guess there are already tools that can make use of that.
> > 
> > Is this to read that you're planning to do a re-spin of the CFI
> > unwinding code (which I'm not allowed to submit another time,
> > but which we've been using for years in SuSE distros) then?
> 
> An in-kernel CFI unwinder?
> 
> No the intended CFI unwinding that I'm working on for perf is made
> on post-processing, on top of partial stack and regs snapshots.
> 
> The true unwinding is computed in userspace.

I think that design will be fundamentally more robust and more flexible than an 
in-kernel unwinder.

Thanks,

	Ingo

Re: [RFC PATCH 1/2] x86: Fix rbp saving in pt_regs on irq entry

[Frederic Weisbecker]

On Fri, Jan 07, 2011 at 05:58:43PM +0100, Ingo Molnar wrote:
> 
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > On Fri, Jan 07, 2011 at 04:13:58PM +0000, Jan Beulich wrote:
> > > >>> On 07.01.11 at 17:05, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > > > Once I'll have perf callchain based on CFI ready, we'll perhaps find some 
> > > > issues
> > > > there. Although I guess there are already tools that can make use of that.
> > > 
> > > Is this to read that you're planning to do a re-spin of the CFI
> > > unwinding code (which I'm not allowed to submit another time,
> > > but which we've been using for years in SuSE distros) then?
> > 
> > An in-kernel CFI unwinder?
> > 
> > No the intended CFI unwinding that I'm working on for perf is made
> > on post-processing, on top of partial stack and regs snapshots.
> > 
> > The true unwinding is computed in userspace.
> 
> I think that design will be fundamentally more robust and more flexible than an 
> in-kernel unwinder.

But in fact for the kernel unwinding it still uses frame pointers which is certainly
the fastest and lightest way to unwind a stack. Very nice for our profiling.

Now one may argue about the loss of a register and its associated overhead, not
sure it's always noticeable though...

Re: [RFC PATCH 1/2] x86: Fix rbp saving in pt_regs on irq entry

[Frederic Weisbecker]

On Fri, Jan 07, 2011 at 04:13:58PM +0000, Jan Beulich wrote:
> >>> On 07.01.11 at 17:05, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > Once I'll have perf callchain based on CFI ready, we'll perhaps find some 
> > issues
> > there. Although I guess there are already tools that can make use of that.
> 
> Is this to read that you're planning to do a re-spin of the CFI
> unwinding code (which I'm not allowed to submit another time,
> but which we've been using for years in SuSE distros) then?

Actually I couldn't test kernel CFI annotations with perf as we
keep the kernel unwinding under frame pointer style.

The CFI unwinding we'll do is only for userspace.

Re: [RFC PATCH 1/2] x86: Fix rbp saving in pt_regs on irq entry

[Frederic Weisbecker]

On Fri, Jan 07, 2011 at 07:45:53AM +0000, Jan Beulich wrote:
> >>> On 06.01.11 at 18:12, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > On Thu, Jan 06, 2011 at 04:58:54PM +0000, Jan Beulich wrote:
> >> >>> On 06.01.11 at 17:54, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> >> > On Thu, Jan 06, 2011 at 04:39:39PM +0000, Jan Beulich wrote:
> >> >> >>> On 06.01.11 at 17:22, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> >> >> > On Thu, Jan 06, 2011 at 04:10:55PM +0000, Jan Beulich wrote:
> >> >> >> >>> On 06.01.11 at 16:45, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> >> >> >> > Before we had:
> >> >> >> > 
> >> >> >> > 
> >> >> >> > 	leaveq
> >> >> >> > 
> >> >> >> > 	CFI_RESTORE             rbp
> >> >> >> > 	CFI_DEF_CFA_REGISTER    rsp
> >> >> >> > 	CFI_ADJUST_CFA_OFFSET   -8
> >> >> >> > 
> >> >> >> > So CFI_RESTORE means rbp has now the value of the base frame of
> >> >> >> > the calling frame (the base frame pointer of the interrupted proc) ?
> >> >> >> 
> >> >> >> No - all it means is that %rbp now has its original (caller or
> >> >> >> interrupted procedure) value again (i.e. an unwinder should not
> >> >> >> try to read it from the stack [or other previously recorded
> >> >> >> location] anymore).
> >> >> >> 
> >> >> >> > And what follows means that rsp-8 points to the return address?
> >> >> >> 
> >> >> >> No - .cfi_def_cfa_register says which register serves as the frame
> >> >> >> pointer, and .cfi_adjust_cfa_offset says to adjust the offset from
> >> >> >> the frame pointer to the top [or bottom] of frame. At any time
> >> >> >> 
> >> >> >> 	CFA = cfa_register + cfa_offset
> >> >> >> 
> >> >> >> with CFA being what all locations on the stack are expressed
> >> >> >> relative to.
> >> >> > 
> >> >> > Ok.
> >> >> > 
> >> >> > So here rsp points to pt_regs::r11
> >> >> > 
> >> >> > I don't understand why locations relative to the stack must be
> >> >> > expressed here by taking rsp - 8 as a base.
> >> >> 
> >> >> Nothing says rsp-8. The annotations merely say to set the base
> >> >> register to rsp and to *adjust* the offset by -8 (after all, that's
> >> >> what the leaveq instruction does).
> >> > 
> >> > Ah! So CFA acts like a virtual frame base pointer right?
> >> 
> >> Correct.
> > 
> > Ah great. I was starting to prepare for the case you come to stab me :)
> > 
> > So what do you think about that:
> > 
> >         leaveq
> > 
> >         CFI_RESTORE             rbp
> >         CFI_DEF_CFA_REGISTER    rsp
> >         CFI_ADJUST_CFA_OFFSET   -8
> > 
> >        /* we did not save rbx, restore only from ARGOFFSET */
> >        addq $8, %rsp
> >        CFI_ADJUST_CFA_OFFSET   -16
> > 
> > 
> > Does that look correct to you? We increased rsp to start recovering
> > the regs from the right place, but the frame pointer of the current
> > proc must stay what it was.
> 
> As you hinted in your subsequent reply - it's -8 here (that's
> why the directive is named *adjust*; there are other
> directives allowing to *set* an offset).

Ok, I'll respin with a proper patch then.
 
> > Now I don't understand how this is all useful as this is not a normal
> > proc but an interruption. We can't get back the return address from
> > the CFA. Or am I missing something?
> 
> Unwind annotations, when written correctly, allow unwinding
> through all kinds of execution flows, including interrupts or
> exceptions as well as including stack switches.

Hmm I see, I guess this is handled through the movq_cfi things we have,
so that the unwinder can ignore the whole part with the saved registers
after which we can find the instruction pointer (considered as the return
address) saved by the hardware.

Fine, thanks for your explanations!

Re: [RFC GIT PULL] perf updates

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> Ingo,
> 
> Please pull the perf/core branch that can be found at:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
> 	perf/core
> 	
> Soeren reported some time ago that callchains were broken on
> x86_64 with cpu-clock/task-clock events. After some time I
> finally found the problem was in the irq low level arg saving.
> It touches fragile code though, I hope some people can
> have a look over the fix.
> 
> Thanks,
> 	Frederic
> ---
> 
> Frederic Weisbecker (2):
>       x86: Fix rbp saving in pt_regs on irq entry
>       perf: Build tools with frame pointer

Is this variant fine with Jan too?

Thanks,

	Ingo

Re: [RFC GIT PULL] perf updates

[Frederic Weisbecker]

On Fri, Jan 07, 2011 at 01:23:17PM +0100, Ingo Molnar wrote:
> 
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > Ingo,
> > 
> > Please pull the perf/core branch that can be found at:
> > 
> > git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
> > 	perf/core
> > 	
> > Soeren reported some time ago that callchains were broken on
> > x86_64 with cpu-clock/task-clock events. After some time I
> > finally found the problem was in the irq low level arg saving.
> > It touches fragile code though, I hope some people can
> > have a look over the fix.
> > 
> > Thanks,
> > 	Frederic
> > ---
> > 
> > Frederic Weisbecker (2):
> >       x86: Fix rbp saving in pt_regs on irq entry
> >       perf: Build tools with frame pointer
> 
> Is this variant fine with Jan too?

So, Arnaldo has picked up the second patch.

For the first I need to resend with a fix on CFI annotations.

Re: [RFC GIT PULL] perf updates

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> On Fri, Jan 07, 2011 at 01:23:17PM +0100, Ingo Molnar wrote:
> > 
> > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > 
> > > Ingo,
> > > 
> > > Please pull the perf/core branch that can be found at:
> > > 
> > > git://git.kernel.org/pub/scm/linux/kernel/git/frederic/random-tracing.git
> > > 	perf/core
> > > 	
> > > Soeren reported some time ago that callchains were broken on
> > > x86_64 with cpu-clock/task-clock events. After some time I
> > > finally found the problem was in the irq low level arg saving.
> > > It touches fragile code though, I hope some people can
> > > have a look over the fix.
> > > 
> > > Thanks,
> > > 	Frederic
> > > ---
> > > 
> > > Frederic Weisbecker (2):
> > >       x86: Fix rbp saving in pt_regs on irq entry
> > >       perf: Build tools with frame pointer
> > 
> > Is this variant fine with Jan too?
> 
> So, Arnaldo has picked up the second patch.
> 
> For the first I need to resend with a fix on CFI annotations.

ok, i'll wait with pulling then.

Thanks,

	Ingo