Re: [GIT PULL] Additional x86 fixes for 2.6.31-rc5

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

Hmm.

I just noticed another issue on x86 code generation, since I was looking 
at assembly language generation due to the do_sigaltstack() kernel stack 
info leak thing.

Our "get_current()" seriously sucks now that it's a per-cpu variable.

Look at the code generated for something like

	current->sas_ss_sp = (unsigned long) ss_sp;
	current->sas_ss_size = ss_size;

and notice how the code really really sucks:

        movq %gs:per_cpu__current_task,%rcx
        movq    %rdx, 1152(%rcx)
        movq %gs:per_cpu__current_task,%rdx
        movq    %rax, 1160(%rdx)

because it reloads that silly per-cpu variable every time, because the 
assembler has a constraint of

	"m" (per_cpu__current_task)

and so gcc is worried that the stores will invalidate the result of the 
load from the per-cpu variable.

I don't know how to fix that _well_, but here's a not-so-very-pretty patch 
that seems to shave off 4.5kB from my kernel, and gives gcc much better 
scheduling for 'current' and 'thread_info' because now it can load them 
early - and cache them - even in the presense of stores.

It uses a "p" (&var) constraint instead of a "m" (var) one, to make gcc 
think there is no actual "load" from memory. This obviously _only_ works 
for percpu variables that are stable within a thread, but 'current' and 
'kernel_stack' should be that way.

End result: the above horror becomes a more reasonable

        movq %gs:per_cpu__current_task,%rax
        movq    %rcx, 1152(%rax)
        movq    %rdx, 1160(%rax)

instead (it still doesn't cache it over the whole function, but it's 
certainly better).

NOTE! I did not test that it all worked. I only looked at the asm, and 
checked out the improvements. All the ones I looked at looked reasonable.

Worthwhile? You be the judge.

There's another detail that may be worth looking at: we often get 
'current' and 'thread_info' together, and they are _not_ in the same 
cache-line. It might be worth defining them together in the per-cpu data, 
and making sure they are in the same cacheline too. In general, we should 
probably look at which per-pcu variables are hot and read-only, and try to 
gathe them all together.

		Linus

---
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sat, 1 Aug 2009 11:50:54 -0700
Subject: [PATCH] x86-64: Add 'percpu_read_stable()' interface for cacheable accesses

This is very useful for some common things like 'get_current()' and
'get_thread_info()', which can be used multiple times in a function, and
where the result is cacheable.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 arch/x86/include/asm/current.h     |    2 +-
 arch/x86/include/asm/percpu.h      |   13 +++++++------
 arch/x86/include/asm/thread_info.h |    2 +-
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/current.h b/arch/x86/include/asm/current.h
index c68c361..4d447b7 100644
--- a/arch/x86/include/asm/current.h
+++ b/arch/x86/include/asm/current.h
@@ -11,7 +11,7 @@ DECLARE_PER_CPU(struct task_struct *, current_task);
 
 static __always_inline struct task_struct *get_current(void)
 {
-	return percpu_read(current_task);
+	return percpu_read_stable(current_task);
 }
 
 #define current get_current()
diff --git a/arch/x86/include/asm/percpu.h b/arch/x86/include/asm/percpu.h
index 103f1dd..3fd619b 100644
--- a/arch/x86/include/asm/percpu.h
+++ b/arch/x86/include/asm/percpu.h
@@ -104,36 +104,37 @@ do {							\
 	}						\
 } while (0)
 
-#define percpu_from_op(op, var)				\
+#define percpu_from_op(op, var, constraint)		\
 ({							\
 	typeof(var) ret__;				\
 	switch (sizeof(var)) {				\
 	case 1:						\
 		asm(op "b "__percpu_arg(1)",%0"		\
 		    : "=q" (ret__)			\
-		    : "m" (var));			\
+		    : constraint);			\
 		break;					\
 	case 2:						\
 		asm(op "w "__percpu_arg(1)",%0"		\
 		    : "=r" (ret__)			\
-		    : "m" (var));			\
+		    : constraint);			\
 		break;					\
 	case 4:						\
 		asm(op "l "__percpu_arg(1)",%0"		\
 		    : "=r" (ret__)			\
-		    : "m" (var));			\
+		    : constraint);			\
 		break;					\
 	case 8:						\
 		asm(op "q "__percpu_arg(1)",%0"		\
 		    : "=r" (ret__)			\
-		    : "m" (var));			\
+		    : constraint);			\
 		break;					\
 	default: __bad_percpu_size();			\
 	}						\
 	ret__;						\
 })
 
-#define percpu_read(var)	percpu_from_op("mov", per_cpu__##var)
+#define percpu_read(var)	percpu_from_op("mov", per_cpu__##var,"m" (per_cpu__##var))
+#define percpu_read_stable(var)	percpu_from_op("mov", per_cpu__##var,"p" (&per_cpu__##var))
 #define percpu_write(var, val)	percpu_to_op("mov", per_cpu__##var, val)
 #define percpu_add(var, val)	percpu_to_op("add", per_cpu__##var, val)
 #define percpu_sub(var, val)	percpu_to_op("sub", per_cpu__##var, val)
diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index fad7d40..a1bb5a1 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -213,7 +213,7 @@ DECLARE_PER_CPU(unsigned long, kernel_stack);
 static inline struct thread_info *current_thread_info(void)
 {
 	struct thread_info *ti;
-	ti = (void *)(percpu_read(kernel_stack) +
+	ti = (void *)(percpu_read_stable(kernel_stack) +
 		      KERNEL_STACK_OFFSET - THREAD_SIZE);
 	return ti;
 }