Re: [PATCH] x86: Optimize tail handling for copy_user

[Vitaly Mayatskikh]

Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> writes:

> Ok, this is starting to look more reasonable. But you cannot split things 
> up like this per-file, because the end result doesn't _work_ with the 
> changes separated.
>
>> BYTES_LEFT_IN_PAGE macro returns PAGE_SIZE, not zero, when the address
>> is well aligned to page.
>
> Hmm. Why? If the address is aligned, then we shouldn't even tro to copy 
> any more, should we? We know we got a fault - and regardless of whether it 
> was because of some offset off the base pointer or not, if the base 
> pointer was at offset zero, it's going to be in the same page. So why try 
> to do an operation we know will fault again?

Yes, I see. I was messed, sorry. Your BYTES_LEFT_IN_PAGE works very
well, tested.

Signed-off-by: Vitaly Mayatskikh <v.mayatskih@xxxxxxxxx>

diff --git a/arch/x86/lib/copy_user_64.S b/arch/x86/lib/copy_user_64.S
index dfdf428..4fbd9d5 100644
--- a/arch/x86/lib/copy_user_64.S
+++ b/arch/x86/lib/copy_user_64.S
@@ -9,12 +9,14 @@
 #include <linux/linkage.h>
 #include <asm/dwarf2.h>
 
+/* Align destination */
 #define FIX_ALIGNMENT 1
 
 #include <asm/current.h>
 #include <asm/asm-offsets.h>
 #include <asm/thread_info.h>
 #include <asm/cpufeature.h>
+#include <asm/uaccess_64.h>
 
 	.macro ALTERNATIVE_JUMP feature,orig,alt
 0:
@@ -44,15 +46,16 @@
 	subl $8,%ecx
 	negl %ecx
 	subl %ecx,%edx
-100:	movb (%rsi),%al
-101:	movb %al,(%rdi)
+100:	movb (%rsi),%ah			/* al is flags */
+101:	movb %ah,(%rdi)
 	incq %rsi
 	incq %rdi
 	decl %ecx
 	jnz 100b
 102:
 	.section .fixup,"ax"
-103:	addl %r8d,%edx			/* ecx is zerorest also */
+103:	addl %ecx,%edx
+	movl %eax,%ecx
 	jmp copy_user_handle_tail
 	.previous
 
@@ -61,7 +64,7 @@
 	.quad 100b,103b
 	.quad 101b,103b
 	.previous
-#endif
+#endif /* FIX_ALIGNMENT */
 	.endm
 
 /* Standard copy_to_user with segment limit checking */
@@ -73,6 +76,7 @@ ENTRY(copy_to_user)
 	jc bad_to_user
 	cmpq TI_addr_limit(%rax),%rcx
 	jae bad_to_user
+	movl $(DEST_IS_USERSPACE),%ecx
 	ALTERNATIVE_JUMP X86_FEATURE_REP_GOOD,copy_user_generic_unrolled,copy_user_generic_string
 	CFI_ENDPROC
 
@@ -85,18 +89,21 @@ ENTRY(copy_from_user)
 	jc bad_from_user
 	cmpq TI_addr_limit(%rax),%rcx
 	jae bad_from_user
+	movl $(SOURCE_IS_USERSPACE | CLEAR_REMAINDER),%ecx
 	ALTERNATIVE_JUMP X86_FEATURE_REP_GOOD,copy_user_generic_unrolled,copy_user_generic_string
 	CFI_ENDPROC
 ENDPROC(copy_from_user)
 
 ENTRY(copy_user_generic)
 	CFI_STARTPROC
+	movl $(SOURCE_IS_USERSPACE | DEST_IS_USERSPACE),%ecx
 	ALTERNATIVE_JUMP X86_FEATURE_REP_GOOD,copy_user_generic_unrolled,copy_user_generic_string
 	CFI_ENDPROC
 ENDPROC(copy_user_generic)
 
 ENTRY(__copy_from_user_inatomic)
 	CFI_STARTPROC
+	movl $(SOURCE_IS_USERSPACE),%ecx
 	ALTERNATIVE_JUMP X86_FEATURE_REP_GOOD,copy_user_generic_unrolled,copy_user_generic_string
 	CFI_ENDPROC
 ENDPROC(__copy_from_user_inatomic)
@@ -125,13 +132,15 @@ ENDPROC(bad_from_user)
  * Input:
  * rdi destination
  * rsi source
- * rdx count
+ * edx count
+ * ecx flags
  *
  * Output:
  * eax uncopied bytes or 0 if successfull.
  */
 ENTRY(copy_user_generic_unrolled)
 	CFI_STARTPROC
+	movl %ecx,%eax	/* save flags in eax */
 	cmpl $8,%edx
 	jb 20f		/* less then 8 bytes, go to byte copy loop */
 	ALIGN_DESTINATION
@@ -169,11 +178,11 @@ ENTRY(copy_user_generic_unrolled)
 	leaq 8(%rdi),%rdi
 	decl %ecx
 	jnz 18b
-20:	andl %edx,%edx
+20:	testl %edx,%edx
 	jz 23f
 	movl %edx,%ecx
-21:	movb (%rsi),%al
-22:	movb %al,(%rdi)
+21:	movb (%rsi),%ah		/* al is flags */
+22:	movb %ah,(%rdi)
 	incq %rsi
 	incq %rdi
 	decl %ecx
@@ -188,7 +197,8 @@ ENTRY(copy_user_generic_unrolled)
 40:	lea (%rdx,%rcx,8),%rdx
 	jmp 60f
 50:	movl %ecx,%edx
-60:	jmp copy_user_handle_tail /* ecx is zerorest also */
+60:	movl %eax,%ecx	/* get flags back to ecx*/
+	jmp copy_user_handle_tail
 	.previous
 
 	.section __ex_table,"a"
@@ -230,15 +240,15 @@ ENDPROC(copy_user_generic_unrolled)
  * Input:
  * rdi destination
  * rsi source
- * rdx count
+ * edx count
+ * ecx flags
  *
  * Output:
  * eax uncopied bytes or 0 if successful.
  */
 ENTRY(copy_user_generic_string)
 	CFI_STARTPROC
-	andl %edx,%edx
-	jz 4f
+	movl %ecx,%eax	/* save flags */
 	cmpl $8,%edx
 	jb 2f		/* less than 8 bytes, go to byte copy loop */
 	ALIGN_DESTINATION
@@ -250,12 +260,13 @@ ENTRY(copy_user_generic_string)
 2:	movl %edx,%ecx
 3:	rep
 	movsb
-4:	xorl %eax,%eax
+	xorl %eax,%eax
 	ret
 
 	.section .fixup,"ax"
 11:	lea (%rdx,%rcx,8),%rcx
-12:	movl %ecx,%edx		/* ecx is zerorest also */
+12:	movl %ecx,%edx
+	movl %eax,%ecx	/* get flags back to ecx */
 	jmp copy_user_handle_tail
 	.previous
 
diff --git a/arch/x86/lib/copy_user_nocache_64.S b/arch/x86/lib/copy_user_nocache_64.S
index 40e0e30..8703ccd 100644
--- a/arch/x86/lib/copy_user_nocache_64.S
+++ b/arch/x86/lib/copy_user_nocache_64.S
@@ -9,11 +9,13 @@
 #include <linux/linkage.h>
 #include <asm/dwarf2.h>
 
+/* Align destination */
 #define FIX_ALIGNMENT 1
 
 #include <asm/current.h>
 #include <asm/asm-offsets.h>
 #include <asm/thread_info.h>
+#include <asm/uaccess_64.h>
 
 	.macro ALIGN_DESTINATION
 #ifdef FIX_ALIGNMENT
@@ -24,15 +26,16 @@
 	subl $8,%ecx
 	negl %ecx
 	subl %ecx,%edx
-100:	movb (%rsi),%al
-101:	movb %al,(%rdi)
+100:	movb (%rsi),%ah			/* al is flags */
+101:	movb %ah,(%rdi)
 	incq %rsi
 	incq %rdi
 	decl %ecx
 	jnz 100b
 102:
 	.section .fixup,"ax"
-103:	addl %r8d,%edx			/* ecx is zerorest also */
+103:	addl %ecx,%edx
+	movl %eax,%ecx
 	jmp copy_user_handle_tail
 	.previous
 
@@ -41,7 +44,7 @@
 	.quad 100b,103b
 	.quad 101b,103b
 	.previous
-#endif
+#endif /* FIX_ALIGNMENT */
 	.endm
 
 /*
@@ -50,6 +53,7 @@
  */
 ENTRY(__copy_user_nocache)
 	CFI_STARTPROC
+	movl %ecx,%eax	/* save flags in eax */
 	cmpl $8,%edx
 	jb 20f		/* less then 8 bytes, go to byte copy loop */
 	ALIGN_DESTINATION
@@ -87,11 +91,11 @@ ENTRY(__copy_user_nocache)
 	leaq 8(%rdi),%rdi
 	decl %ecx
 	jnz 18b
-20:	andl %edx,%edx
+20:	testl %edx,%edx
 	jz 23f
 	movl %edx,%ecx
-21:	movb (%rsi),%al
-22:	movb %al,(%rdi)
+21:	movb (%rsi),%ah		/* al is flags */
+22:	movb %ah,(%rdi)
 	incq %rsi
 	incq %rdi
 	decl %ecx
@@ -108,7 +112,7 @@ ENTRY(__copy_user_nocache)
 	jmp 60f
 50:	movl %ecx,%edx
 60:	sfence
-	movl %r8d,%ecx
+	movl %eax,%ecx	/* get flags back to ecx*/
 	jmp copy_user_handle_tail
 	.previous
 
diff --git a/arch/x86/lib/usercopy_64.c b/arch/x86/lib/usercopy_64.c
index f4df6e7..d793900 100644
--- a/arch/x86/lib/usercopy_64.c
+++ b/arch/x86/lib/usercopy_64.c
@@ -161,23 +161,32 @@ EXPORT_SYMBOL(copy_in_user);
 /*
  * Try to copy last bytes and clear the rest if needed.
  * Since protection fault in copy_from/to_user is not a normal situation,
- * it is not necessary to optimize tail handling.
+ * it is not necessary to do low level optimization of tail handling.
  */
 unsigned long
-copy_user_handle_tail(char *to, char *from, unsigned len, unsigned zerorest)
+copy_user_handle_tail(char *dst, char *src, unsigned remainder, unsigned flags)
 {
 	char c;
-	unsigned zero_len;
+	unsigned max_copy = remainder;
 
-	for (; len; --len) {
-		if (__get_user_nocheck(c, from++, sizeof(char)))
+	/* Don't even bother trying to cross a page in user space! */
+	if (flags & DEST_IS_USERSPACE)
+		max_copy = min(max_copy, BYTES_LEFT_IN_PAGE(dst));
+	if (flags & SOURCE_IS_USERSPACE)
+		max_copy = min(max_copy, BYTES_LEFT_IN_PAGE(src));
+
+	while (max_copy--) {
+		if (__get_user(c, src))
 			break;
-		if (__put_user_nocheck(c, to++, sizeof(char)))
+		if (__put_user(c, dst))
 			break;
+		src++;
+		dst++;
+		remainder--;
 	}
 
-	for (c = 0, zero_len = len; zerorest && zero_len; --zero_len)
-		if (__put_user_nocheck(c, to++, sizeof(char)))
-			break;
-	return len;
+	if (flags & CLEAR_REMAINDER)
+		memset(dst, 0, remainder);
+
+	return remainder;
 }
diff --git a/include/asm-x86/uaccess_64.h b/include/asm-x86/uaccess_64.h
index 5cfd295..ea042f8 100644
--- a/include/asm-x86/uaccess_64.h
+++ b/include/asm-x86/uaccess_64.h
@@ -1,6 +1,16 @@
 #ifndef ASM_X86__UACCESS_64_H
 #define ASM_X86__UACCESS_64_H
 
+/* Flags for copy_user_handle_tail */
+#define CLEAR_REMAINDER		1
+#define DEST_IS_USERSPACE	2
+#define SOURCE_IS_USERSPACE	4
+
+#define BYTES_LEFT_IN_PAGE(ptr) \
+	(unsigned int)((PAGE_SIZE-1) & -(long)(ptr))
+
+#ifndef __ASSEMBLY__
+
 /*
  * User space memory access functions
  */
@@ -179,23 +189,26 @@ __copy_to_user_inatomic(void __user *dst, const void *src, unsigned size)
 }
 
 extern long __copy_user_nocache(void *dst, const void __user *src,
-				unsigned size, int zerorest);
+				unsigned size, unsigned flags);
 
 static inline int __copy_from_user_nocache(void *dst, const void __user *src,
 					   unsigned size)
 {
 	might_sleep();
-	return __copy_user_nocache(dst, src, size, 1);
+	return __copy_user_nocache(dst, src, size, SOURCE_IS_USERSPACE
+						   | CLEAR_REMAINDER);
 }
 
 static inline int __copy_from_user_inatomic_nocache(void *dst,
 						    const void __user *src,
 						    unsigned size)
 {
-	return __copy_user_nocache(dst, src, size, 0);
+	return __copy_user_nocache(dst, src, size, SOURCE_IS_USERSPACE);
 }
 
 unsigned long
-copy_user_handle_tail(char *to, char *from, unsigned len, unsigned zerorest);
+copy_user_handle_tail(char *dst, char *src, unsigned remainder, unsigned flags);
+
+#endif /* __ASSEMBLY__ */
 
 #endif /* ASM_X86__UACCESS_64_H */

-- 
wbr, Vitaly
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/