On Sun, Jun 21, 2015 at 9:52 AM, Ingo Molnar <mi...@kernel.org> wrote:
>
> * Alexey Dobriyan <adobri...@gmail.com> 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/
