Re: [PATCH 01/11] x86: Use asm goto to implement better modify_and_test() functions

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 1831 bytes --]

On Tue, Sep 17, 2013 at 4:10 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> Linus suggested using asm goto to get rid of the typical SETcc + TEST
> instruction pair -- which also clobbers an extra register -- for our
> typical modify_and_test() functions.

Thinking about this, we actually have another place in x86 low-level
code where "asm goto" makes a lot of sense: exception handling for
__put_user_asm().

I'd love to use it for __get_user_asm() too, but it doesn't work there
because "asm goto" cannot have outputs (and a get_user obviously needs
an output - the value it gets). But for put_user(), it seems to be a
very good match.

The attached patch is ENTIRELY untested, but I did check some of the
generated assembly language. And the output is absolutely beautiful,
because now gcc sees the error case directly, so the straight-line
code is just he single "mov" instruction, no tests, no nothing. The
exception case will just jump to the local label directly.

Of course, the STAC/CLAC noise is there, and we really should try to
come up with a better model for that (so that the code that uses
__put_user() because it wants to do many of them in one go after
having done one access_ok() check) but that's a separate issue.

hpa, comments? Are you looking at perhaps moving the stac/clac
instructions out? With this, "filldir()" ends up lookng something like

   ...
   data32 xchg %ax,%ax  # stac
   mov    %rcx,0x8(%rax)
   data32 xchg %ax,%ax  # clac
   mov    0x10(%rbx),%r13
   data32 xchg %ax,%ax  # stac
   mov    %r8,0x0(%r13)
   data32 xchg %ax,%ax  # clac
   data32 xchg %ax,%ax  # stac
   mov    %r12w,0x10(%r13)
   data32 xchg %ax,%ax  # clac
   ...

which is a bit sad, since the code really is almost perfect aside from
the tons of extra nops/stac/clac instructions...

                        Linus

[-- Attachment #2: patch.diff --]
[-- Type: application/octet-stream, Size: 1436 bytes --]

 arch/x86/include/asm/uaccess.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 5838fa911aa0..33597722cfc1 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -411,6 +411,24 @@ struct __large_struct { unsigned long buf[100]; };
  * we do not write to any memory gcc knows about, so there are no
  * aliasing issues.
  */
+#ifdef CC_HAVE_ASM_GOTO
+#define __put_user_asm(x, addr, err, itype, rtype, ltype, errret)	\
+do {	__label__ error_label;						\
+	asm goto(ASM_STAC "\n"						\
+		 "1:	mov"itype" %"rtype"0,%1\n"			\
+		 "	" ASM_CLAC "\n"					\
+		 _ASM_EXTABLE(1b,%l[error_label])			\
+		 : /* no outputs */					\
+		 : ltype(x), "m" (__m(addr))				\
+		 : /* no clobbers */					\
+		 : error_label);					\
+	err = 0;							\
+	break;								\
+error_label:								\
+	asm volatile(ASM_CLAC);						\
+	err = errret;							\
+} while (0)
+#else
 #define __put_user_asm(x, addr, err, itype, rtype, ltype, errret)	\
 	asm volatile(ASM_STAC "\n"					\
 		     "1:	mov"itype" %"rtype"1,%2\n"		\
@@ -422,6 +440,7 @@ struct __large_struct { unsigned long buf[100]; };
 		     _ASM_EXTABLE(1b, 3b)				\
 		     : "=r"(err)					\
 		     : ltype(x), "m" (__m(addr)), "i" (errret), "0" (err))
+#endif
 
 #define __put_user_asm_ex(x, addr, itype, rtype, ltype)			\
 	asm volatile("1:	mov"itype" %"rtype"0,%1\n"		\