[PATCH v3 0/3] sched: account fair load avg consistently

[PATCH v3 0/3] sched: account fair load avg consistently

[byungchul.park]

From: Byungchul Park <byungchul.park@lge.com>

* change from v2 to v3
- consider otimization in the case of migration
- split patches to 3 to be reviewed easily

* change from v1 to v2
- make set_task_rq() do that role instead of migration callback
- make set_task_rq() do that role instead of move group callback
- remove the dependancy between last_update_time and check for migration

Byungchul Park (3):
  sched: introduce a new migration flag to task_struct
  sched: factor out the code getting cfs_rq's last_update_time
  sched: make it possible to account fair class load avg consistently

 include/linux/sched.h |    3 +++
 kernel/sched/core.c   |    1 +
 kernel/sched/fair.c   |   63 ++++++++++++++++++++++++++++++++-----------------
 kernel/sched/sched.h  |    5 ++++
 4 files changed, 51 insertions(+), 21 deletions(-)

-- 
1.7.9.5

[PATCH v3 1/3] sched: introduce a new migration flag to task_struct

[byungchul.park]

From: Byungchul Park <byungchul.park@lge.com>

This patch removes a weird coupling between se->avg.last_update_time and
the condition checking for migration, and introduce a new migration flag.
Now, scheduler can use the flag instead of se->avg.last_update_time to
check if migration already happened or not.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 include/linux/sched.h |    3 +++
 kernel/sched/core.c   |    1 +
 kernel/sched/fair.c   |   22 ++++++++++++----------
 kernel/sched/sched.h  |    1 +
 4 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 699228b..a104c72 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1379,6 +1379,9 @@ struct task_struct {
 #endif
 	int on_rq;
 
+	/* For indicating if a migration has happened. */
+	int migrated;
+
 	int prio, static_prio, normal_prio;
 	unsigned int rt_priority;
 	const struct sched_class *sched_class;
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index a91df61..57f4300 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2068,6 +2068,7 @@ void __dl_clear_params(struct task_struct *p)
 static void __sched_fork(unsigned long clone_flags, struct task_struct *p)
 {
 	p->on_rq			= 0;
+	p->migrated			= 0;
 
 	p->se.on_rq			= 0;
 	p->se.exec_start		= 0;
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 077076f..0f76903 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -2771,14 +2771,15 @@ static void detach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *s
 
 /* Add the load generated by se into cfs_rq's load average */
 static inline void
-enqueue_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
+enqueue_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
 {
 	struct sched_avg *sa = &se->avg;
 	u64 now = cfs_rq_clock_task(cfs_rq);
-	int migrated, decayed;
+	int decayed;
+	int migrated = flags & ENQUEUE_MIGRATED;
+	int created = !sa->last_update_time;
 
-	migrated = !sa->last_update_time;
-	if (!migrated) {
+	if (!migrated && !created) {
 		__update_load_avg(now, cpu_of(rq_of(cfs_rq)), sa,
 			se->on_rq * scale_load_down(se->load.weight),
 			cfs_rq->curr == se, NULL);
@@ -2789,10 +2790,10 @@ enqueue_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
 	cfs_rq->runnable_load_avg += sa->load_avg;
 	cfs_rq->runnable_load_sum += sa->load_sum;
 
-	if (migrated)
+	if (migrated || created)
 		attach_entity_load_avg(cfs_rq, se);
 
-	if (decayed || migrated)
+	if (decayed || migrated || created)
 		update_tg_load_avg(cfs_rq, 0);
 }
 
@@ -2868,7 +2869,7 @@ static int idle_balance(struct rq *this_rq);
 
 static inline void update_load_avg(struct sched_entity *se, int update_tg) {}
 static inline void
-enqueue_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se) {}
+enqueue_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags) {}
 static inline void
 dequeue_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se) {}
 static inline void remove_entity_load_avg(struct sched_entity *se) {}
@@ -3008,7 +3009,7 @@ enqueue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
 	 * Update run-time statistics of the 'current'.
 	 */
 	update_curr(cfs_rq);
-	enqueue_entity_load_avg(cfs_rq, se);
+	enqueue_entity_load_avg(cfs_rq, se, flags);
 	account_entity_enqueue(cfs_rq, se);
 	update_cfs_shares(cfs_rq);
 
@@ -4136,6 +4137,7 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
 	struct cfs_rq *cfs_rq;
 	struct sched_entity *se = &p->se;
 
+	flags = flags | (xchg(&p->migrated, 0) ? ENQUEUE_MIGRATED : 0);
 	for_each_sched_entity(se) {
 		if (se->on_rq)
 			break;
@@ -5021,7 +5023,7 @@ static void migrate_task_rq_fair(struct task_struct *p, int next_cpu)
 	remove_entity_load_avg(&p->se);
 
 	/* Tell new CPU we are migrated */
-	p->se.avg.last_update_time = 0;
+	p->migrated = 1;
 
 	/* We have migrated, no longer consider this task hot */
 	p->se.exec_start = 0;
@@ -8082,7 +8084,7 @@ static void task_move_group_fair(struct task_struct *p)
 	set_task_rq(p, task_cpu(p));
 
 #ifdef CONFIG_SMP
-	/* Tell se's cfs_rq has been changed -- migrated */
+	/* Tell se's cfs_rq has been changed */
 	p->se.avg.last_update_time = 0;
 #endif
 	attach_task_cfs_rq(p);
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index af6f252..66d0552 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1158,6 +1158,7 @@ static const u32 prio_to_wmult[40] = {
 #define ENQUEUE_WAKING		0
 #endif
 #define ENQUEUE_REPLENISH	8
+#define ENQUEUE_MIGRATED	16
 
 #define DEQUEUE_SLEEP		1
 
-- 
1.7.9.5

[PATCH v3 2/3] sched: factor out the code getting cfs_rq's last_update_time

[byungchul.park]

From: Byungchul Park <byungchul.park@lge.com>

In the case that rq->lock may not be held, care must be taken to get a
cfs_rq's last_update_time instead of just reading the variable. Since
it can happen at serveral places in the code in future, this patch
factors it out to a helper function.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 kernel/sched/fair.c |   20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 0f76903..08589a0 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -2809,15 +2809,10 @@ dequeue_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
 		max_t(s64,  cfs_rq->runnable_load_sum - se->avg.load_sum, 0);
 }
 
-/*
- * Task first catches up with cfs_rq, and then subtract
- * itself from the cfs_rq (task must be off the queue now).
- */
-void remove_entity_load_avg(struct sched_entity *se)
+/* This function is useful for the case that rq->lock may not be held */
+static inline u64 get_last_update_time(struct cfs_rq *cfs_rq)
 {
-	struct cfs_rq *cfs_rq = cfs_rq_of(se);
 	u64 last_update_time;
-
 #ifndef CONFIG_64BIT
 	u64 last_update_time_copy;
 
@@ -2829,6 +2824,17 @@ void remove_entity_load_avg(struct sched_entity *se)
 #else
 	last_update_time = cfs_rq->avg.last_update_time;
 #endif
+	return last_update_time;
+}
+
+/*
+ * Task first catches up with cfs_rq, and then subtract
+ * itself from the cfs_rq (task must be off the queue now).
+ */
+void remove_entity_load_avg(struct sched_entity *se)
+{
+	struct cfs_rq *cfs_rq = cfs_rq_of(se);
+	u64 last_update_time = get_last_update_time(cfs_rq);
 
 	__update_load_avg(last_update_time, cpu_of(rq_of(cfs_rq)), &se->avg, 0, 0, NULL);
 	atomic_long_add(se->avg.load_avg, &cfs_rq->removed_load_avg);
-- 
1.7.9.5

[PATCH v3 3/3] sched: make it possible to account fair class load avg consistently

[byungchul.park]

From: Byungchul Park <byungchul.park@lge.com>

Current code can account fair class load average for the time the task
was absent from the fair class thanks to ATTACH_AGE_LOAD. However, it
doesn't work in the cases that either migration or group change happened
in the other sched classes.

This patch introduces more general way to care fair class load average
accounting, and it works consistently in any case e.g. migration or
cgroup change in other sched classes.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 kernel/sched/fair.c  |   23 ++++++++++++++++++-----
 kernel/sched/sched.h |    4 ++++
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 08589a0..ad5d34c 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -2827,6 +2827,24 @@ static inline u64 get_last_update_time(struct cfs_rq *cfs_rq)
 	return last_update_time;
 }
 
+#ifdef CONFIG_FAIR_GROUP_SCHED
+/*
+ * Called within set_task_rq() right before setting a task's cpu. The
+ * caller only guarantees p->pi_lock is held; no other assumptions,
+ * including the state of rq->lock, should be made. Thus we need to
+ * use get_last_update_time(). See get_last_update_time().
+ */
+void update_last_update_time(struct sched_entity *se,
+			     struct cfs_rq *prev,
+			     struct cfs_rq *next)
+{
+	if (se->avg.last_update_time) {
+		se->avg.last_update_time -= get_last_update_time(prev);
+		se->avg.last_update_time += get_last_update_time(next);
+	}
+}
+#endif
+
 /*
  * Task first catches up with cfs_rq, and then subtract
  * itself from the cfs_rq (task must be off the queue now).
@@ -8088,11 +8106,6 @@ static void task_move_group_fair(struct task_struct *p)
 {
 	detach_task_cfs_rq(p);
 	set_task_rq(p, task_cpu(p));
-
-#ifdef CONFIG_SMP
-	/* Tell se's cfs_rq has been changed */
-	p->se.avg.last_update_time = 0;
-#endif
 	attach_task_cfs_rq(p);
 }
 
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 66d0552..559f9c7 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -335,6 +335,9 @@ extern void sched_move_task(struct task_struct *tsk);
 
 #ifdef CONFIG_FAIR_GROUP_SCHED
 extern int sched_group_set_shares(struct task_group *tg, unsigned long shares);
+extern void update_last_update_time(struct sched_entity *se,
+				    struct cfs_rq *prev,
+				    struct cfs_rq *next);
 #endif
 
 #else /* CONFIG_CGROUP_SCHED */
@@ -933,6 +936,7 @@ static inline void set_task_rq(struct task_struct *p, unsigned int cpu)
 #endif
 
 #ifdef CONFIG_FAIR_GROUP_SCHED
+	update_last_update_time(&p->se, p->se.cfs_rq, tg->cfs_rq[cpu]);
 	p->se.cfs_rq = tg->cfs_rq[cpu];
 	p->se.parent = tg->se[cpu];
 #endif
-- 
1.7.9.5

Re: [PATCH v3 1/3] sched: introduce a new migration flag to task_struct

[Peter Zijlstra]

On Thu, Oct 15, 2015 at 06:01:14PM +0900, byungchul.park@lge.com wrote:
> From: Byungchul Park <byungchul.park@lge.com>
> 
> This patch removes a weird coupling between se->avg.last_update_time and
> the condition checking for migration, and introduce a new migration flag.
> Now, scheduler can use the flag instead of se->avg.last_update_time to
> check if migration already happened or not.

Was there a problem with that coupling? This does not explain.

> Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> ---
>  include/linux/sched.h |    3 +++
>  kernel/sched/core.c   |    1 +
>  kernel/sched/fair.c   |   22 ++++++++++++----------
>  kernel/sched/sched.h  |    1 +
>  4 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 699228b..a104c72 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1379,6 +1379,9 @@ struct task_struct {
>  #endif
>  	int on_rq;
>  
> +	/* For indicating if a migration has happened. */
> +	int migrated;

You just created another 4 byte hole instead of filling one.

>  	int prio, static_prio, normal_prio;
>  	unsigned int rt_priority;
>  	const struct sched_class *sched_class;

> +++ b/kernel/sched/fair.c
> @@ -2771,14 +2771,15 @@ static void detach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *s
>  
>  /* Add the load generated by se into cfs_rq's load average */
>  static inline void
> -enqueue_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
> +enqueue_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
>  {
>  	struct sched_avg *sa = &se->avg;
>  	u64 now = cfs_rq_clock_task(cfs_rq);
> -	int migrated, decayed;
> +	int decayed;
> +	int migrated = flags & ENQUEUE_MIGRATED;
> +	int created = !sa->last_update_time;
>  
> -	migrated = !sa->last_update_time;
> -	if (!migrated) {
> +	if (!migrated && !created) {
>  		__update_load_avg(now, cpu_of(rq_of(cfs_rq)), sa,
>  			se->on_rq * scale_load_down(se->load.weight),
>  			cfs_rq->curr == se, NULL);
> @@ -2789,10 +2790,10 @@ enqueue_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
>  	cfs_rq->runnable_load_avg += sa->load_avg;
>  	cfs_rq->runnable_load_sum += sa->load_sum;
>  
> -	if (migrated)
> +	if (migrated || created)
>  		attach_entity_load_avg(cfs_rq, se);
>  
> -	if (decayed || migrated)
> +	if (decayed || migrated || created)
>  		update_tg_load_avg(cfs_rq, 0);
>  }

How much extra code gets generated for this? These _are_ hot paths.

> @@ -4136,6 +4137,7 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
>  	struct cfs_rq *cfs_rq;
>  	struct sched_entity *se = &p->se;
>  
> +	flags = flags | (xchg(&p->migrated, 0) ? ENQUEUE_MIGRATED : 0);

Yeah, no way. xchg() is an absurdly expensive instruction, we do not
place that unconditionally in the enqueue path.

> @@ -5021,7 +5023,7 @@ static void migrate_task_rq_fair(struct task_struct *p, int next_cpu)
>  	remove_entity_load_avg(&p->se);
>  
>  	/* Tell new CPU we are migrated */
> -	p->se.avg.last_update_time = 0;
> +	p->migrated = 1;
>  
>  	/* We have migrated, no longer consider this task hot */
>  	p->se.exec_start = 0;
> @@ -8082,7 +8084,7 @@ static void task_move_group_fair(struct task_struct *p)
>  	set_task_rq(p, task_cpu(p));
>  
>  #ifdef CONFIG_SMP
> -	/* Tell se's cfs_rq has been changed -- migrated */
> +	/* Tell se's cfs_rq has been changed */
>  	p->se.avg.last_update_time = 0;
>  #endif
>  	attach_task_cfs_rq(p);

So my tiny little patch removed more code than it added, and simplified
a few things, like the above. Now we have 2 states to worry about.

How is this making things better?

> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index af6f252..66d0552 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -1158,6 +1158,7 @@ static const u32 prio_to_wmult[40] = {
>  #define ENQUEUE_WAKING		0
>  #endif
>  #define ENQUEUE_REPLENISH	8
> +#define ENQUEUE_MIGRATED	16

Won't actually apply that..

Re: [PATCH v3 1/3] sched: introduce a new migration flag to task_struct

[Byungchul Park]

On Thu, Oct 15, 2015 at 01:18:13PM +0200, Peter Zijlstra wrote:
> On Thu, Oct 15, 2015 at 06:01:14PM +0900, byungchul.park@lge.com wrote:
> > From: Byungchul Park <byungchul.park@lge.com>
> > 
> > This patch removes a weird coupling between se->avg.last_update_time and
> > the condition checking for migration, and introduce a new migration flag.
> > Now, scheduler can use the flag instead of se->avg.last_update_time to
> > check if migration already happened or not.
> 
> Was there a problem with that coupling? This does not explain.

The reason why i introduce the new flag is that 3/3 patch makes
se->avg.last_update_time non-zero consistently, so we cannot use the
condition "se->avg.last_update_time == 0" to check if migration has
happened.

> > +++ b/kernel/sched/fair.c
> > @@ -2771,14 +2771,15 @@ static void detach_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *s
> >  
> >  /* Add the load generated by se into cfs_rq's load average */
> >  static inline void
> > -enqueue_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
> > +enqueue_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
> >  {
> >  	struct sched_avg *sa = &se->avg;
> >  	u64 now = cfs_rq_clock_task(cfs_rq);
> > -	int migrated, decayed;
> > +	int decayed;
> > +	int migrated = flags & ENQUEUE_MIGRATED;
> > +	int created = !sa->last_update_time;
> >  
> > -	migrated = !sa->last_update_time;
> > -	if (!migrated) {
> > +	if (!migrated && !created) {
> >  		__update_load_avg(now, cpu_of(rq_of(cfs_rq)), sa,
> >  			se->on_rq * scale_load_down(se->load.weight),
> >  			cfs_rq->curr == se, NULL);
> > @@ -2789,10 +2790,10 @@ enqueue_entity_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se)
> >  	cfs_rq->runnable_load_avg += sa->load_avg;
> >  	cfs_rq->runnable_load_sum += sa->load_sum;
> >  
> > -	if (migrated)
> > +	if (migrated || created)
> >  		attach_entity_load_avg(cfs_rq, se);
> >  
> > -	if (decayed || migrated)
> > +	if (decayed || migrated || created)
> >  		update_tg_load_avg(cfs_rq, 0);
> >  }
> 
> How much extra code gets generated for this? These _are_ hot paths.

Okay, you are right. It's a hot path.

> 
> > @@ -4136,6 +4137,7 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
> >  	struct cfs_rq *cfs_rq;
> >  	struct sched_entity *se = &p->se;
> >  
> > +	flags = flags | (xchg(&p->migrated, 0) ? ENQUEUE_MIGRATED : 0);
> 
> Yeah, no way. xchg() is an absurdly expensive instruction, we do not
> place that unconditionally in the enqueue path.

Okay.

> 
> > @@ -5021,7 +5023,7 @@ static void migrate_task_rq_fair(struct task_struct *p, int next_cpu)
> >  	remove_entity_load_avg(&p->se);
> >  
> >  	/* Tell new CPU we are migrated */
> > -	p->se.avg.last_update_time = 0;
> > +	p->migrated = 1;
> >  
> >  	/* We have migrated, no longer consider this task hot */
> >  	p->se.exec_start = 0;
> > @@ -8082,7 +8084,7 @@ static void task_move_group_fair(struct task_struct *p)
> >  	set_task_rq(p, task_cpu(p));
> >  
> >  #ifdef CONFIG_SMP
> > -	/* Tell se's cfs_rq has been changed -- migrated */
> > +	/* Tell se's cfs_rq has been changed */
> >  	p->se.avg.last_update_time = 0;
> >  #endif
> >  	attach_task_cfs_rq(p);
> 
> So my tiny little patch removed more code than it added, and simplified
> a few things, like the above. Now we have 2 states to worry about.
> 
> How is this making things better?

As I said, this patch is mainly for 3/3 patch. But if you worry about
regression by this 1/3 patch, I will think more about another way.

> 
> > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > index af6f252..66d0552 100644
> > --- a/kernel/sched/sched.h
> > +++ b/kernel/sched/sched.h
> > @@ -1158,6 +1158,7 @@ static const u32 prio_to_wmult[40] = {
> >  #define ENQUEUE_WAKING		0
> >  #endif
> >  #define ENQUEUE_REPLENISH	8
> > +#define ENQUEUE_MIGRATED	16
> 
> Won't actually apply that..

Okay, I got your concern, let me think more..

By the way, what do you think about the approach of 3/3 patch?

Thanks,
Byungchul

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

Re: [PATCH v3 1/3] sched: introduce a new migration flag to task_struct

[Peter Zijlstra]

On Thu, Oct 15, 2015 at 09:26:44PM +0900, Byungchul Park wrote:
> On Thu, Oct 15, 2015 at 01:18:13PM +0200, Peter Zijlstra wrote:
> > On Thu, Oct 15, 2015 at 06:01:14PM +0900, byungchul.park@lge.com wrote:
> > > From: Byungchul Park <byungchul.park@lge.com>
> > > 
> > > This patch removes a weird coupling between se->avg.last_update_time and
> > > the condition checking for migration, and introduce a new migration flag.
> > > Now, scheduler can use the flag instead of se->avg.last_update_time to
> > > check if migration already happened or not.
> > 
> > Was there a problem with that coupling? This does not explain.
> 
> The reason why i introduce the new flag is that 3/3 patch makes
> se->avg.last_update_time non-zero consistently, so we cannot use the
> condition "se->avg.last_update_time == 0" to check if migration has
> happened.

It would have been ever so good to have had that in the Changelog.

> > > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > > index af6f252..66d0552 100644
> > > --- a/kernel/sched/sched.h
> > > +++ b/kernel/sched/sched.h
> > > @@ -1158,6 +1158,7 @@ static const u32 prio_to_wmult[40] = {
> > >  #define ENQUEUE_WAKING		0
> > >  #endif
> > >  #define ENQUEUE_REPLENISH	8
> > > +#define ENQUEUE_MIGRATED	16
> > 
> > Won't actually apply that..
> 
> Okay, I got your concern, let me think more..

Ah, so what I means was that that hunk will not apply. I see that what I
typed did not express that as clearly as I might have wanted.

Current code looks like:

#define ENQUEUE_REPLENISH       0x08
#define ENQUEUE_RESTORE		0x10

> By the way, what do you think about the approach of 3/3 patch?

Again a wee bit of worry. I like it for 64bits as there its nearly free
and does the right thing, but on 32bit we add 2 smp_rmb() invocations
(which are not free on ARM etc..). [Note that one could rewrite that
function to require but a single rmb].