x86/asm: Optimize unnecessarily wide TEST instructions
authorDenys Vlasenko <dvlasenk@redhat.com>
Fri, 6 Mar 2015 20:55:32 +0000 (21:55 +0100)
committerIngo Molnar <mingo@kernel.org>
Sat, 7 Mar 2015 10:12:43 +0000 (11:12 +0100)
By the nature of the TEST operation, it is often possible to test
a narrower part of the operand:

    "testl $3,  mem"  ->  "testb $3, mem",
    "testq $3, %rcx"  ->  "testb $3, %cl"

This results in shorter instructions, because the TEST instruction
has no sign-entending byte-immediate forms unlike other ALU ops.

Note that this change does not create any LCP (Length-Changing Prefix)
stalls, which happen when adding a 0x66 prefix, which happens when
16-bit immediates are used, which changes such TEST instructions:

  [test_opcode] [modrm] [imm32]

to:

  [0x66] [test_opcode] [modrm] [imm16]

where [imm16] has a *different length* now: 2 bytes instead of 4.
This confuses the decoder and slows down execution.

REX prefixes were carefully designed to almost never hit this case:
adding REX prefix does not change instruction length except MOVABS
and MOV [addr],RAX instruction.

This patch does not add instructions which would use a 0x66 prefix,
code changes in assembly are:

    -48 f7 07 01 00 00 00  testq  $0x1,(%rdi)
    +f6 07 01              testb  $0x1,(%rdi)
    -48 f7 c1 01 00 00 00  test   $0x1,%rcx
    +f6 c1 01              test   $0x1,%cl
    -48 f7 c1 02 00 00 00  test   $0x2,%rcx
    +f6 c1 02              test   $0x2,%cl
    -41 f7 c2 01 00 00 00  test   $0x1,%r10d
    +41 f6 c2 01           test   $0x1,%r10b
    -48 f7 c1 04 00 00 00  test   $0x4,%rcx
    +f6 c1 04              test   $0x4,%cl
    -48 f7 c1 08 00 00 00  test   $0x8,%rcx
    +f6 c1 08              test   $0x8,%cl

Linus further notes:

   "There are no stalls from using 8-bit instruction forms.

    Now, changing from 64-bit or 32-bit 'test' instructions to 8-bit ones
    *could* cause problems if it ends up having forwarding issues, so that
    instead of just forwarding the result, you end up having to wait for
    it to be stable in the L1 cache (or possibly the register file). The
    forwarding from the store buffer is simplest and most reliable if the
    read is done at the exact same address and the exact same size as the
    write that gets forwarded.

    But that's true only if:

     (a) the write was very recent and is still in the write queue. I'm
         not sure that's the case here anyway.

     (b) on at least most Intel microarchitectures, you have to test a
         different byte than the lowest one (so forwarding a 64-bit write
         to a 8-bit read ends up working fine, as long as the 8-bit read
         is of the low 8 bits of the written data).

    A very similar issue *might* show up for registers too, not just
    memory writes, if you use 'testb' with a high-byte register (where
    instead of forwarding the value from the original producer it needs to
    go through the register file and then shifted). But it's mainly a
    problem for store buffers.

    But afaik, the way Denys changed the test instructions, neither of the
    above issues should be true.

    The real problem for store buffer forwarding tends to be "write 8
    bits, read 32 bits". That can be really surprisingly expensive,
    because the read ends up having to wait until the write has hit the
    cacheline, and we might talk tens of cycles of latency here. But
    "write 32 bits, read the low 8 bits" *should* be fast on pretty much
    all x86 chips, afaik."

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
Acked-by: Andy Lutomirski <luto@amacapital.net>
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: H. Peter Anvin <hpa@linux.intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Will Drewry <wad@chromium.org>
Link: http://lkml.kernel.org/r/1425675332-31576-1-git-send-email-dvlasenk@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
arch/x86/kernel/head_64.S
arch/x86/kernel/relocate_kernel_32.S
arch/x86/kernel/relocate_kernel_64.S
arch/x86/lib/checksum_32.S
arch/x86/lib/csum-copy_64.S

index 9a0919678f97558a6ba2d86b057082a9b4ae4e2c..ae6588b301c248b3c281a1e072802e6764e9ac44 100644 (file)
@@ -146,7 +146,7 @@ startup_64:
        leaq    level2_kernel_pgt(%rip), %rdi
        leaq    4096(%rdi), %r8
        /* See if it is a valid page table entry */
-1:     testq   $1, 0(%rdi)
+1:     testb   $1, 0(%rdi)
        jz      2f
        addq    %rbp, 0(%rdi)
        /* Go to the next page */
index e13f8e7c22a68c3d9590b8deb69e116d2f625f68..77630d57e7bf30ac7f281e15c440cd7b7bae37a9 100644 (file)
@@ -226,23 +226,23 @@ swap_pages:
        movl    (%ebx), %ecx
        addl    $4, %ebx
 1:
-       testl   $0x1,   %ecx  /* is it a destination page */
+       testb   $0x1, %cl     /* is it a destination page */
        jz      2f
        movl    %ecx,   %edi
        andl    $0xfffff000, %edi
        jmp     0b
 2:
-       testl   $0x2,   %ecx  /* is it an indirection page */
+       testb   $0x2, %cl    /* is it an indirection page */
        jz      2f
        movl    %ecx,   %ebx
        andl    $0xfffff000, %ebx
        jmp     0b
 2:
-       testl   $0x4,   %ecx /* is it the done indicator */
+       testb   $0x4, %cl    /* is it the done indicator */
        jz      2f
        jmp     3f
 2:
-       testl   $0x8,   %ecx /* is it the source indicator */
+       testb   $0x8, %cl    /* is it the source indicator */
        jz      0b           /* Ignore it otherwise */
        movl    %ecx,   %esi /* For every source page do a copy */
        andl    $0xfffff000, %esi
index 3fd2c693e4752d01e071de68ef57e2db8b47b605..04cb1790a596841c778008522f490e01d451839b 100644 (file)
@@ -221,23 +221,23 @@ swap_pages:
        movq    (%rbx), %rcx
        addq    $8,     %rbx
 1:
-       testq   $0x1,   %rcx  /* is it a destination page? */
+       testb   $0x1,   %cl   /* is it a destination page? */
        jz      2f
        movq    %rcx,   %rdi
        andq    $0xfffffffffffff000, %rdi
        jmp     0b
 2:
-       testq   $0x2,   %rcx  /* is it an indirection page? */
+       testb   $0x2,   %cl   /* is it an indirection page? */
        jz      2f
        movq    %rcx,   %rbx
        andq    $0xfffffffffffff000, %rbx
        jmp     0b
 2:
-       testq   $0x4,   %rcx  /* is it the done indicator? */
+       testb   $0x4,   %cl   /* is it the done indicator? */
        jz      2f
        jmp     3f
 2:
-       testq   $0x8,   %rcx  /* is it the source indicator? */
+       testb   $0x8,   %cl   /* is it the source indicator? */
        jz      0b            /* Ignore it otherwise */
        movq    %rcx,   %rsi  /* For ever source page do a copy */
        andq    $0xfffffffffffff000, %rsi
index c3b9953d3fa0b6e9ef32c929f81006cf18209a5a..9bc944a9127481ead40689a73054d80e50f0bc10 100644 (file)
@@ -125,7 +125,7 @@ ENTRY(csum_partial)
 6:     addl %ecx,%eax
        adcl $0, %eax 
 7:     
-       testl $1, 12(%esp)
+       testb $1, 12(%esp)
        jz 8f
        roll $8, %eax
 8:
@@ -245,7 +245,7 @@ ENTRY(csum_partial)
        addl %ebx,%eax
        adcl $0,%eax
 80: 
-       testl $1, 12(%esp)
+       testb $1, 12(%esp)
        jz 90f
        roll $8, %eax
 90: 
index 2419d5fefae30ac2453fabf71c633ede013d2e4a..9734182966f3be925a38c0762aa71e3148156148 100644 (file)
@@ -196,7 +196,7 @@ ENTRY(csum_partial_copy_generic)
 
        /* handle last odd byte */
 .Lhandle_1:
-       testl $1, %r10d
+       testb $1, %r10b
        jz    .Lende
        xorl  %ebx, %ebx
        source