Re: [PATCHv2] perf x86_64: Fix rsp register for system call fast path

[Frederic Weisbecker <fweisbec@gmail.com>]

On Tue, Oct 02, 2012 at 04:58:15PM +0200, Jiri Olsa wrote:
> diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
> index 915b876..11d62ff 100644
> --- a/arch/x86/kernel/cpu/perf_event.c
> +++ b/arch/x86/kernel/cpu/perf_event.c
> @@ -34,6 +34,7 @@
>  #include <asm/timer.h>
>  #include <asm/desc.h>
>  #include <asm/ldt.h>
> +#include <asm/syscall.h>
>  
>  #include "perf_event.h"
>  
> @@ -1699,6 +1700,52 @@ void arch_perf_update_userpage(struct perf_event_mmap_page *userpg, u64 now)
>  	userpg->time_offset = this_cpu_read(cyc2ns_offset) - now;
>  }
>  
> +#ifdef CONFIG_X86_64
> +__weak int arch_sample_regs_user(struct pt_regs *oregs, struct pt_regs *regs)
> +{
> +	int kernel = !user_mode(regs);
> +
> +	if (kernel) {
> +		if (current->mm)
> +			regs = task_pt_regs(current);
> +		else
> +			regs = NULL;
> +	}

Shouldn't the above stay in generic code?

> +
> +	if (regs) {
> +		memcpy(oregs, regs, sizeof(*regs));
> +
> +		/*
> +		 * If the perf event was triggered within the kernel code
> +		 * path, then it was either syscall or interrupt. While
> +		 * interrupt stores almost all user registers, the syscall
> +		 * fast path does not. At this point we can at least set
> +		 * rsp register right, which is crucial for dwarf unwind.
> +		 *
> +		 * The syscall_get_nr function returns -1 (orig_ax) for
> +		 * interrupt, and positive value for syscall.
> +		 *
> +		 * We have two race windows in here:
> +		 *
> +		 * 1) Few instructions from syscall entry until old_rsp is
> +		 *    set.
> +		 *
> +		 * 2) In syscall/interrupt path from entry until the orig_ax
> +		 *    is set.
> +		 *
> +		 * Above described race windows are fractional opposed to
> +		 * the syscall fast path, so we get much better results
> +		 * fixing rsp this way.

That said, a race is there already: if the syscall is interrupted before
SAVE_ARGS and co.

I'm trying to scratch my head to find a solution to detect the race and
bail out instead of recording erroneous values but I can't find one.

Anyway this is still better than what we have now.

Another solution could be to force syscall slow path and have some variable
set there that tells us we are in a syscall and every regs have been saved.

But we probably don't want to force syscall slow path...