Re: x86 copy performance regression

[Linus Torvalds]

On Fri, May 26, 2023 at 10:51 AM Eric Dumazet <edumazet@xxxxxxxxxx> 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