Re: [PATCH 1/2] x86: use alternatives for clear_user()

[Alexey Dobriyan]

On Sun, Jun 21, 2015 at 9:52 AM, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
> * Alexey Dobriyan <adobriyan@xxxxxxxxx> wrote:
>
>> Alternatives allow to pick faster code: REP STOSQ, REP STOSB or else. Default to
>> REP STOSQ (as memset() does).

> 2)
>
> Please cite before/after /usr/bin/size comparisons of the .o (and vmlinux where
> sensible) in the changelog (you can also do objdump -d comparisons), especially
> for the first and second patch this will show that the move is an invariant, and
> that the effects of the modernization of the MOVQ method.

Difference is obviously negligible: ADD/SUB adds 1 byte compared to INC/DEC,
MOVQ 0 adds several.

>> +
>> +     ALTERNATIVE_2 "jmp __clear_user_movq",                  \
>> +             "", X86_FEATURE_REP_GOOD,                       \
>> +             "jmp __clear_user_rep_stosb", X86_FEATURE_ERMS
>
> Can we move this into clear_user(), and patch in CALL instructions instead of
> jumps? There's no reason to do these extra jumps.

Yes, should be faster.

> So for consistency's sake I'd put a label here that names the default function
> __clear_user_rep_stosq. So that we know what it is when it shows up in 'perf top'.
>
> (With the 'CALL' patching approach this would become __clear_user_rep_stosq in a
> natural fashion - so in that case the extra label is not needed.)
>
>> +     ASM_STAC
>> +     xor     %eax, %eax
>> +     mov     %rsi, %rcx
>> +     and     $7, %esi
>> +     shr     $3, %rcx
>> +1:   rep stosq
>> +     mov     %esi, %ecx
>> +2:   rep stosb
>> +3:
>> +     mov     %rcx, %rax
>> +     ASM_CLAC
>> +     ret
>
> So I'd switch the ASM_CLAC with the MOV, because flags manipulation probably has
> higher latency than a simple register move. (the same reason we do the STAC as the
> first step)

Maybe, it is written this way for symmetry, if STAC is first
instruction, CLAC is the last,
no mismatches possible.

>> +     mov     %rsi, %rcx
>> +     and     $7, %esi
>> +     shr     $3, %rcx
>> +     jz      2f
>> +     .p2align 4
>
> There's no need to align this small loop - the NOP only slows things down.

Alignment was copied from memset()/copy_from_user() loops, probably
doesn't matter.

>>  EXPORT_SYMBOL(clear_user);
>> +EXPORT_SYMBOL(__clear_user);
>
> Also, please consider inlining clear_user()'s access_ok() check: so that the only
> function left is __clear_user(). (if then that should be a separate patch)

Inlining access_ok() will add ~30 bytes to every call site.
And there is might_fault() eyesore as well.

0000000000000050 <clear_user>:
  50:   48 8d 14 37             lea    (%rdi,%rsi,1),%rdx
  54:   65 48 8b 04 25 00 00    mov    %gs:0x0,%rax
  5b:   00 00
                        59: R_X86_64_32S        cpu_tss+0x4
  5d:   48 39 d6                cmp    %rdx,%rsi
  60:   48 8b 88 18 c0 ff ff    mov    -0x3fe8(%rax),%rcx
  67:   48 89 f0                mov    %rsi,%rax
  6a:   77 0f                   ja     7b <clear_user+0x2b>
  6c:   48 39 d1                cmp    %rdx,%rcx
  6f:   72 0a                   jb     7b <clear_user+0x2b>
  71:   55                      push   %rbp
  72:   48 89 e5                mov    %rsp,%rbp
  75:   e8 00 00 00 00          callq  7a <clear_user+0x2a>
                        76: R_X86_64_PC32       __clear_user-0x4
  7a:   5d                      pop    %rbp
  7b:   c3                      retq

I'll fix according to your suggestions and resend.

    Alexey
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/