On Thu, Jun 11, 2015 at 4:47 AM, Denys Vlasenko <[email protected]> wrote: > "setbe %al" insn has a register merge stall: it needs to combine > previous %eax value with new value for the lowest byte. > Subsequent "movzbl %al,%edi" in turn depends on its completion. > > This patch replaces "setbe %al + movzbl %al,%edi" pair of insns > with "xor %edi,%edi" before the comparison, and conditional "inc %edi". > > This results in the same value of %edi as produced by old code, > but first insn has no dependencies, and we end up with having > only one insn with deps which executes only if %eax contains error > return, and both insns are shorter: 2 bytes each versus 3 bytes each. > > (The old code was inherited from 32-bit code, where it allowed to avoid > a conditional jump. Here we have to use a jump anyway). > > Signed-off-by: Denys Vlasenko <[email protected]> > CC: Linus Torvalds <[email protected]> > CC: Steven Rostedt <[email protected]> > CC: Ingo Molnar <[email protected]> > CC: Borislav Petkov <[email protected]> > CC: "H. Peter Anvin" <[email protected]> > CC: Andy Lutomirski <[email protected]> > CC: Oleg Nesterov <[email protected]> > CC: Frederic Weisbecker <[email protected]> > CC: Alexei Starovoitov <[email protected]> > CC: Will Drewry <[email protected]> > CC: Kees Cook <[email protected]> > CC: [email protected] > CC: [email protected] > --- > arch/x86/entry/entry_64_compat.S | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/entry/entry_64_compat.S > b/arch/x86/entry/entry_64_compat.S > index bb187a6..96f33a4 100644 > --- a/arch/x86/entry/entry_64_compat.S > +++ b/arch/x86/entry/entry_64_compat.S > @@ -213,12 +213,13 @@ sysexit_from_sys_call: > jnz ia32_ret_from_sys_call > TRACE_IRQS_ON > ENABLE_INTERRUPTS(CLBR_NONE) > + xor %edi, %edi > movl %eax, %esi /* second arg, syscall return value */ > cmpl $-MAX_ERRNO, %eax /* is it an error ? */ > jbe 1f
We go here if !be, which is allegedly the error case, which confuses me, because setbe will set al (and hence edi) if be, which is also claimed to be the error case. Ignoring the comments for now... > movslq %eax, %rsi /* if error sign extend to 64 bits */ > -1: setbe %al /* 1 if error, 0 if not */ > - movzbl %al, %edi /* zero-extend that into %edi */ Old code: edi == 1 if be and edi == 0 if !be. > + inc %edi New code: edi == 1 if !be and edi == 1 if be. So I think that both the comment and the new code are wrong. Am I just confused? --Andy > +1: /* edi: 1 if error, 0 if not */ > call __audit_syscall_exit > movq RAX(%rsp), %rax /* reload syscall return value */ > movl $(_TIF_ALLWORK_MASK & ~_TIF_SYSCALL_AUDIT), %edi > -- > 1.8.1.4 > -- Andy Lutomirski AMA Capital Management, LLC -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
