Re: Additional compiler barrier required in sched_preempt_enable_no_resched?

[Peter Zijlstra]

On Mon, May 16, 2016 at 12:55:57PM +0200, Peter Zijlstra wrote:
> > So the barriers already forbid that the compiler reorders code. How on earth
> > is the compiler supposed to optimize the dec/inc out?
> 
> Order things like:
> 
> > #define sched_preempt_enable_no_resched() \
> > do { \
> >         barrier(); \
> >         preempt_count_dec(); \
> > } while (0)
> 
> > #define preempt_disable() \
> > do { \
> >         preempt_count_inc(); \
> >         barrier(); \
> > } while (0)
> 
> And there is no barrier between the dec and inc, and a smarty pants
> compiler could just decide to forgo the update, since in program order
> there is no observable difference either way.
> 
> Making the thing volatile tells the compiler there can be external
> observations of the memory and it cannot assume things like that and
> must emit the operations.

...

> So I'll go write a proper changelog for the volatile thing and get it
> merged with a Cc to stable.


---
Subject: sched,preempt: Fix preempt_count manipulations
From: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
Date: Mon May 16 15:01:11 CEST 2016

Vikram reported that his ARM64 compiler managed to 'optimize' away the
preempt_count manipulations in code like:

	preempt_enable_no_resched();
	put_user();
	preempt_disable();

Irrespective of that fact that that is horrible code that should be
fixed for many reasons, it does highlight a deficiency in the generic
preempt_count manipulators. As it is never right to combine/elide
preempt_count manipulations like this.

Therefore sprinkle some volatile in the two generic accessors to
ensure the compiler is aware of the fact that the preempt_count is
observed outside of the regular program-order view and thus cannot be
optimized away like this.

x86; the only arch not using the generic code is not affected as we
do all this in asm in order to use the segment base per-cpu stuff.

Cc: stable@xxxxxxxxxxxxxxx
Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
Fixes: a787870924db ("sched, arch: Create asm/preempt.h")
Reported-by: Vikram Mulukutla <markivx@xxxxxxxxxxxxxx>
Tested-by: Vikram Mulukutla <markivx@xxxxxxxxxxxxxx>
Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
---
 include/asm-generic/preempt.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/include/asm-generic/preempt.h
+++ b/include/asm-generic/preempt.h
@@ -7,10 +7,10 @@
 
 static __always_inline int preempt_count(void)
 {
-	return current_thread_info()->preempt_count;
+	return READ_ONCE(current_thread_info()->preempt_count);
 }
 
-static __always_inline int *preempt_count_ptr(void)
+static __always_inline volatile int *preempt_count_ptr(void)
 {
 	return &current_thread_info()->preempt_count;
 }