Messages in this thread Patch in this message | | | From | Linus Torvalds <> | Date | Fri, 26 May 2023 11:33:25 -0700 | Subject | Re: x86 copy performance regression |
| |
On Fri, May 26, 2023 at 10:51 AM Eric Dumazet <edumazet@google.com> wrote: > > Hmmm > > [ 25.532236] RIP: 0010:0xffffffffa5a85134 > [ 25.536173] Code: Unable to access opcode bytes at 0xffffffffa5a8510a.
This was the other reason I really didn't want to use alternatives on the conditional branch instructions. The relocations are really not very natural, and we have odd rules for those things. So I suspect our instruction rewriting simply gets this wrong, because that's such a nasty pattern.
I really wanted my "just hardcode the instruction bytes" to work. Not only did it get me the small 2-byte conditional jump, it meant that there was no relocation on it. But objtool really hates not understanding what the alternatives code does.
Which is fair enough, but it's frustrating here when it only results in more problems.
Anyway, I guess *this* avoids all issues. It creates an extra jump to a jump for the case where the CPU doesn't have ERMS, but I guess we don't really care about those CPUs anyway.
And it avoids all the "alternative instructions have relocations" issues. And it creates all small two-byte jumps, and the "rep movsb" fits exactly on that same 2 bytes too. Which I guess all argues for this being what I should have started with.
This time it *really* works.
Famous last words.
Linus arch/x86/lib/copy_user_64.S | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/arch/x86/lib/copy_user_64.S b/arch/x86/lib/copy_user_64.S index 4fc5c2de2de4..01c5de4c279b 100644 --- a/arch/x86/lib/copy_user_64.S +++ b/arch/x86/lib/copy_user_64.S @@ -7,6 +7,8 @@ */ #include <linux/linkage.h> +#include <asm/cpufeatures.h> +#include <asm/alternative.h> #include <asm/asm.h> #include <asm/export.h> @@ -29,7 +31,7 @@ */ SYM_FUNC_START(rep_movs_alternative) cmpq $64,%rcx - jae .Lunrolled + jae .Llarge cmp $8,%ecx jae .Lword @@ -65,6 +67,12 @@ SYM_FUNC_START(rep_movs_alternative) _ASM_EXTABLE_UA( 2b, .Lcopy_user_tail) _ASM_EXTABLE_UA( 3b, .Lcopy_user_tail) +.Llarge: +0: ALTERNATIVE "jmp .Lunrolled", "rep movsb", X86_FEATURE_ERMS +1: RET + + _ASM_EXTABLE_UA( 0b, 1b) + .p2align 4 .Lunrolled: 10: movq (%rsi),%r8 | |