Re: [PATCH 3/4] x86: save user rsp in pt_regs->sp on SYSCALL64 fastpath

From: Andy Lutomirski <luto@amacapital.net>
To: Ingo Molnar <mingo@kernel.org>
Cc: Denys Vlasenko <dvlasenk@redhat.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Borislav Petkov <bp@alien8.de>, "H. Peter Anvin" <hpa@zytor.com>,
	Oleg Nesterov <oleg@redhat.com>,
	Frederic Weisbecker <fweisbec@gmail.com>,
	Alexei Starovoitov <ast@plumgrid.com>,
	Will Drewry <wad@chromium.org>, Kees Cook <keescook@chromium.org>,
	X86 ML <x86@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 3/4] x86: save user rsp in pt_regs->sp on SYSCALL64 fastpath
Date: Tue, 10 Mar 2015 06:10:29 -0700	[thread overview]
Message-ID: <CALCETrUFbbjeUrZEuUvGQLefrKBTH=RTdVJ0vXgAGBEEuzEMKg@mail.gmail.com> (raw)
In-Reply-To: <20150310125151.GB21686@gmail.com>

On Tue, Mar 10, 2015 at 5:51 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Denys Vlasenko <dvlasenk@redhat.com> wrote:
>
>> PER_CPU(old_rsp) usage is simplified - now it is used only
>> as temp storage, and userspace stack pointer is immediately stored
>> in pt_regs->sp on syscall entry, instead of being used later,
>> on syscall exit.
>>
>> Instead of PER_CPU(old_rsp) and task->thread.usersp, C code
>> uses pt_regs->sp now.
>>
>> FIXUP/RESTORE_TOP_OF_STACK are simplified.
>
> Just trying to judge the performance impact:
>
>> --- a/arch/x86/kernel/entry_64.S
>> +++ b/arch/x86/kernel/entry_64.S
>> @@ -128,8 +128,6 @@ ENDPROC(native_usergs_sysret64)
>>   * manipulation.
>>   */
>>       .macro FIXUP_TOP_OF_STACK tmp offset=0
>> -     movq PER_CPU_VAR(old_rsp),\tmp
>> -     movq \tmp,RSP+\offset(%rsp)
>>       movq $__USER_DS,SS+\offset(%rsp)
>>       movq $__USER_CS,CS+\offset(%rsp)
>>       movq RIP+\offset(%rsp),\tmp  /* get rip */
>> @@ -139,8 +137,7 @@ ENDPROC(native_usergs_sysret64)
>>       .endm
>>
>>       .macro RESTORE_TOP_OF_STACK tmp offset=0
>> -     movq RSP+\offset(%rsp),\tmp
>> -     movq \tmp,PER_CPU_VAR(old_rsp)
>> +     /* nothing to do */
>>       .endm
>>
>>  /*
>> @@ -253,11 +247,13 @@ GLOBAL(system_call_after_swapgs)
>>        */
>>       ENABLE_INTERRUPTS(CLBR_NONE)
>>       ALLOC_PT_GPREGS_ON_STACK 8              /* +8: space for orig_ax */
>> +     movq    %rcx,RIP(%rsp)
>> +     movq    PER_CPU_VAR(old_rsp),%rcx
>> +     movq    %r11,EFLAGS(%rsp)
>> +     movq    %rcx,RSP(%rsp)
>> +     movq_cfi rax,ORIG_RAX
>>       SAVE_C_REGS_EXCEPT_RAX_RCX_R11
>>       movq    $-ENOSYS,RAX(%rsp)
>> -     movq_cfi rax,ORIG_RAX
>> -     movq    %r11,EFLAGS(%rsp)
>> -     movq    %rcx,RIP(%rsp)
>>       CFI_REL_OFFSET rip,RIP
>>       testl $_TIF_WORK_SYSCALL_ENTRY,TI_flags+THREAD_INFO(%rsp,RIP)
>>       jnz tracesys
>
> So there are now +2 instructions (5 instead of 3) in the system_call
> path, but there are -2 instructions in the SYSRETQ path, so combined
> it's a wash performance-wise, right?

I think that the SYSRETQ path is the same number of instructions --
RESTORE_TOP_OF_STACK is only used in somewhat unusual circumstances.

On SYSRETQ, I think we touch one fewer cache line, although that cache
line will be in L1 unless the syscall pushed it out.  The added
instructions on entry are probably nearly free, though, as one of them
is a load from a forwarded address and the other is a store to a hot
cacheline.

--And