[PATCH] doc: memory-barriers.txt: Minor correction in Control dependencies

[PATCH] doc: memory-barriers.txt: Minor correction in Control dependencies

[Pranith Kumar]

Minor corrections in memory-barriers.txt document.

Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 Documentation/memory-barriers.txt | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index abec3f9..41a6c9c 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -627,7 +627,7 @@ proving the value of 'a', and the pair of barrier() invocations are
 required to prevent the compiler from pulling the two identical stores
 to 'b' out from the legs of the "if" statement.
 
-It is important to note that control dependencies absolutely require a
+It is important to note that control dependencies absolutely require
 a conditional.  For example, the following "optimized" version of
 the above example breaks ordering, which is why the barrier() invocations
 are absolutely required if you have identical stores in both legs of
@@ -652,7 +652,7 @@ for example, as follows:
 		do_something();
 	} else {
 		barrier();
-		ACCESS_ONCE(b) = q / 3;
+		ACCESS_ONCE(b) = q / 2;
 		do_something_else();
 	}
 
@@ -710,7 +710,7 @@ x and y both being zero:
 
 The above two-CPU example will never trigger the assert().  However,
 if control dependencies guaranteed transitivity (which they do not),
-then adding the following two CPUs would guarantee a related assertion:
+then adding the following CPU would guarantee a related assertion:
 
 	CPU 2
 	=====================
@@ -719,8 +719,8 @@ then adding the following two CPUs would guarantee a related assertion:
 	assert(!(r1 == 2 && r2 == 1 && x == 2)); /* FAILS!!! */
 
 But because control dependencies do -not- provide transitivity, the
-above assertion can fail after the combined four-CPU example completes.
-If you need the four-CPU example to provide ordering, you will need
+above assertion can fail after the combined three-CPU example completes.
+If you need the three-CPU example to provide ordering, you will need
 smp_mb() between the loads and stores in the CPU 0 and CPU 1 code fragments,
 that is, just before or just after the "if" statements.
 
-- 
1.9.1

[PATCH] doc: memory-barriers.txt: Add barrier() to provide ordering

[Pranith Kumar]

In the provably false case, add barrier() in both the legs of the if() condition
to provide ordering.

Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 Documentation/memory-barriers.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index 41a6c9c..0f2903f 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -689,9 +689,11 @@ should do something like the following:
 	q = ACCESS_ONCE(a);
 	BUILD_BUG_ON(MAX <= 1); /* Order load from a with store to b. */
 	if (q % MAX) {
+		barrier();
 		ACCESS_ONCE(b) = p;
 		do_something();
 	} else {
+		barrier();
 		ACCESS_ONCE(b) = p;
 		do_something_else();
 	}
-- 
1.9.1

Re: [PATCH] doc: memory-barriers.txt: Add barrier() to provide ordering

[Paul E. McKenney]

On Mon, Aug 11, 2014 at 06:51:40PM -0400, Pranith Kumar wrote:
> In the provably false case, add barrier() in both the legs of the if() condition
> to provide ordering.
> 
> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
> ---
>  Documentation/memory-barriers.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> index 41a6c9c..0f2903f 100644
> --- a/Documentation/memory-barriers.txt
> +++ b/Documentation/memory-barriers.txt
> @@ -689,9 +689,11 @@ should do something like the following:
>  	q = ACCESS_ONCE(a);
>  	BUILD_BUG_ON(MAX <= 1); /* Order load from a with store to b. */
>  	if (q % MAX) {
> +		barrier();
>  		ACCESS_ONCE(b) = p;
>  		do_something();
>  	} else {
> +		barrier();
>  		ACCESS_ONCE(b) = p;
>  		do_something_else();
>  	}

This section had to be rewritten due to -O3.

							Thanx, Paul

[PATCH] sched: Remove ACCESS_ONCE() for jiffies

[Pranith Kumar]

jiffies is declared as a volatile variable. Therefore it is not neccessary to
use ACCESS_ONCE() while reading it.

Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 kernel/sched/core.c | 2 +-
 kernel/sched/proc.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index ec1a286..381d0d0 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2552,7 +2552,7 @@ void scheduler_tick(void)
 u64 scheduler_tick_max_deferment(void)
 {
 	struct rq *rq = this_rq();
-	unsigned long next, now = ACCESS_ONCE(jiffies);
+	unsigned long next, now = jiffies;
 
 	next = rq->last_sched_tick + HZ;
 
diff --git a/kernel/sched/proc.c b/kernel/sched/proc.c
index 8ecd552..87ab31b 100644
--- a/kernel/sched/proc.c
+++ b/kernel/sched/proc.c
@@ -526,7 +526,7 @@ static inline unsigned long get_rq_runnable_load(struct rq *rq)
  */
 void update_idle_cpu_load(struct rq *this_rq)
 {
-	unsigned long curr_jiffies = ACCESS_ONCE(jiffies);
+	unsigned long curr_jiffies = jiffies;
 	unsigned long load = get_rq_runnable_load(this_rq);
 	unsigned long pending_updates;
 
@@ -548,7 +548,7 @@ void update_idle_cpu_load(struct rq *this_rq)
 void update_cpu_load_nohz(void)
 {
 	struct rq *this_rq = this_rq();
-	unsigned long curr_jiffies = ACCESS_ONCE(jiffies);
+	unsigned long curr_jiffies = jiffies;
 	unsigned long pending_updates;
 
 	if (curr_jiffies == this_rq->last_load_update_tick)
-- 
1.9.1

Re: [PATCH] sched: Remove ACCESS_ONCE() for jiffies

[Peter Zijlstra]

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

On Mon, Aug 11, 2014 at 06:51:41PM -0400, Pranith Kumar wrote:
> jiffies is declared as a volatile variable. Therefore it is not neccessary to
> use ACCESS_ONCE() while reading it.

It also doesn't hurt and it documents intent, so I'm inclined to keep
it.

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] sched: Remove ACCESS_ONCE() for jiffies

[Pranith Kumar]

On Tue, Aug 12, 2014 at 1:55 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, Aug 11, 2014 at 06:51:41PM -0400, Pranith Kumar wrote:
>> jiffies is declared as a volatile variable. Therefore it is not neccessary to
>> use ACCESS_ONCE() while reading it.
>
> It also doesn't hurt and it documents intent, so I'm inclined to keep
> it.

It looks more like an inconsistency. These are the only three places
which use ACCESS_ONCE() for jiffies. Also, in doc/memory-barriers.txt,
it explicitly states that ACCESS_ONCE(jiffies) is not necessary. I
grepped for this usage after reading that document just to make sure
and found these three uses.

But yes, there is no harm being done by using ACCESS_ONCE().

-- 
Pranith

Re: [PATCH] sched: Remove ACCESS_ONCE() for jiffies

[David Rientjes]

On Tue, 12 Aug 2014, Pranith Kumar wrote:

> It looks more like an inconsistency. These are the only three places
> which use ACCESS_ONCE() for jiffies. Also, in doc/memory-barriers.txt,
> it explicitly states that ACCESS_ONCE(jiffies) is not necessary. I
> grepped for this usage after reading that document just to make sure
> and found these three uses.
> 
> But yes, there is no harm being done by using ACCESS_ONCE().
> 

One could argue that doing ACCESS_ONCE(jiffies) is always better because 
of the context it which it is used rather than qualifying jiffies itself 
as volatile.

[PATCH] compiler.h: Move __memory_barrier() use to compiler-intel.h

[Pranith Kumar]

Commit 73679e5082012 ("compiler-intel.h: Remove duplicate definition") removed a
duplicate definition of barrier() from compiler-intel.h. This is the wrong
duplicate definition removed since __memory_barrier() is an intel compiler
specific intrinsic and should live in compiler-intel.h.

This commit reverts the previous commit and removes the definition from
compiler.h

Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 include/linux/compiler-intel.h | 3 +++
 include/linux/compiler.h       | 5 -----
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/include/linux/compiler-intel.h b/include/linux/compiler-intel.h
index ba147a1..5529c52 100644
--- a/include/linux/compiler-intel.h
+++ b/include/linux/compiler-intel.h
@@ -13,9 +13,12 @@
 /* Intel ECC compiler doesn't support gcc specific asm stmts.
  * It uses intrinsics to do the equivalent things.
  */
+#undef barrier
 #undef RELOC_HIDE
 #undef OPTIMIZER_HIDE_VAR
 
+#define barrier() __memory_barrier()
+
 #define RELOC_HIDE(ptr, off)					\
   ({ unsigned long __ptr;					\
      __ptr = (unsigned long) (ptr);				\
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index d5ad7b1..bc24cad 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -160,11 +160,6 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
 # define unlikely(x)	__builtin_expect(!!(x), 0)
 #endif
 
-/* Optimization barrier */
-#ifndef barrier
-# define barrier() __memory_barrier()
-#endif
-
 /* Unreachable code */
 #ifndef unreachable
 # define unreachable() do { } while (1)
-- 
1.9.1

Re: [PATCH] doc: memory-barriers.txt: Minor correction in Control dependencies

[Paul E. McKenney]

On Mon, Aug 11, 2014 at 06:51:39PM -0400, Pranith Kumar wrote:
> Minor corrections in memory-barriers.txt document.
> 
> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
> ---
>  Documentation/memory-barriers.txt | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
> index abec3f9..41a6c9c 100644
> --- a/Documentation/memory-barriers.txt
> +++ b/Documentation/memory-barriers.txt
> @@ -627,7 +627,7 @@ proving the value of 'a', and the pair of barrier() invocations are
>  required to prevent the compiler from pulling the two identical stores
>  to 'b' out from the legs of the "if" statement.
> 
> -It is important to note that control dependencies absolutely require a
> +It is important to note that control dependencies absolutely require
>  a conditional.  For example, the following "optimized" version of
>  the above example breaks ordering, which is why the barrier() invocations
>  are absolutely required if you have identical stores in both legs of
> @@ -652,7 +652,7 @@ for example, as follows:
>  		do_something();
>  	} else {
>  		barrier();
> -		ACCESS_ONCE(b) = q / 3;
> +		ACCESS_ONCE(b) = q / 2;
>  		do_something_else();
>  	}

This one is gone due to the two-legged "if" statements being reworked.

> @@ -710,7 +710,7 @@ x and y both being zero:
> 
>  The above two-CPU example will never trigger the assert().  However,
>  if control dependencies guaranteed transitivity (which they do not),
> -then adding the following two CPUs would guarantee a related assertion:
> +then adding the following CPU would guarantee a related assertion:
> 
>  	CPU 2
>  	=====================
> @@ -719,8 +719,8 @@ then adding the following two CPUs would guarantee a related assertion:
>  	assert(!(r1 == 2 && r2 == 1 && x == 2)); /* FAILS!!! */
> 
>  But because control dependencies do -not- provide transitivity, the
> -above assertion can fail after the combined four-CPU example completes.
> -If you need the four-CPU example to provide ordering, you will need
> +above assertion can fail after the combined three-CPU example completes.
> +If you need the three-CPU example to provide ordering, you will need
>  smp_mb() between the loads and stores in the CPU 0 and CPU 1 code fragments,
>  that is, just before or just after the "if" statements.

Nikolay Samofatov beat you to this one by a couple of weeks.

Nevertheless, thank you for your careful review!

							Thanx, Paul