Skip to contentSkip to conversation

rcu: use this_cpu_ptr per-cpu helper instead of per_cpu_ptr(p, raw_smp_processor_id())

PATCH In review Shan Wei 3 participants 8 messages Oct 31, 2012 – Nov 1, 2012

lore
Share & tools
lore

Shan Wei

author
#0 lore raw

PATCH 7/9 rcu: use this_cpu_ptr per-cpu helper instead of per_cpu_ptr(p, raw_smp_processor_id())

From: Shan Wei <[email protected]>

smp_processor_id is defined as raw_smp_processor_id. replace per_cpu_ptr(p, raw_smp_processor_id()) is also ok.

Signed-off-by Shan Wei <[email protected]>
---
 kernel/rcutree.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/rcutree.c +1 -1
index 74df86b..3a21fcf 100644
@@ -1960,7 +1960,7 @@ static void force_quiescent_state(struct rcu_state *rsp)
 	struct rcu_node *rnp_old = NULL;
 
 	/* Funnel through hierarchy to reduce memory contention. */
-	rnp = per_cpu_ptr(rsp->rda, raw_smp_processor_id())->mynode;
+	rnp = this_cpu_ptr(rsp->rda)->mynode;
 	for (; rnp != NULL; rnp = rnp->parent) {
 		ret = (ACCESS_ONCE(rsp->gp_flags) & RCU_GP_FLAG_FQS) ||
 		      !raw_spin_trylock(&rnp->fqslock);
Signature · 1 line
1.7.1
#1 lore raw
Quoted 6 lines · Shan Wei

From: Shan Wei <[email protected]>

smp_processor_id is defined as raw_smp_processor_id. replace per_cpu_ptr(p, raw_smp_processor_id()) is also ok.

Signed-off-by Shan Wei <[email protected]>

Hello, Shan Wei,

There are several definitions of this_cpu_ptr():

0 percpu.h 63 #define this_cpu_ptr(ptr) SHIFT_PERCPU_PTR(ptr, my_cpu_offset) 1 percpu.h 65 #define this_cpu_ptr(ptr) __this_cpu_ptr(ptr)
2 percpu.h 85 #define this_cpu_ptr(ptr) per_cpu_ptr(ptr, 0)

The first uses my_cpu_offset, which is further defined in two ways:

0 percpu.h 33 #define my_cpu_offset per_cpu_offset(smp_processor_id()) 1 percpu.h 35 #define my_cpu_offset __my_cpu_offset

The first uses smp_processor_id(), which will complain if force_quiescent_state() is called with preemption disabled, which it sometimes is.

So what am I missing here?

Thanx, Paul

Quoted 20 lines
---
 kernel/rcutree.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/rcutree.c +1 -1
index 74df86b..3a21fcf 100644
@@ -1960,7 +1960,7 @@ static void force_quiescent_state(struct rcu_state *rsp)
 	struct rcu_node *rnp_old = NULL;
 
 	/* Funnel through hierarchy to reduce memory contention. */
-	rnp = per_cpu_ptr(rsp->rda, raw_smp_processor_id())->mynode;
+	rnp = this_cpu_ptr(rsp->rda)->mynode;
 	for (; rnp != NULL; rnp = rnp->parent) {
 		ret = (ACCESS_ONCE(rsp->gp_flags) & RCU_GP_FLAG_FQS) ||
 		      !raw_spin_trylock(&rnp->fqslock);
Signature · 1 line
1.7.1
author
#2 lore raw

Paul E. McKenney said, at 2012/10/31 19:51:

Quoted 6 lines

The first uses smp_processor_id(), which will complain if force_quiescent_state() is called with preemption disabled, which it sometimes is.

So what am I missing here?

Hi Paul

this patch is not right for CONFIG_DEBUG_PREEMPT case. __this_cpu_ptr is ok which do not check for preemption context.

Quoted 2 lines

Thanx, Paul

#3 lore raw
Quoted 6 lines · Shan Wei

Paul E. McKenney said, at 2012/10/31 19:51:

Quoted 5 lines

The first uses smp_processor_id(), which will complain if force_quiescent_state() is called with preemption disabled, which it sometimes is.

Quoted 6 lines
Quoted 1 line

So what am I missing here?

Hi Paul

this patch is not right for CONFIG_DEBUG_PREEMPT case. __this_cpu_ptr is ok which do not check for preemption context.

Ah, got it. Please feel free to submit an updated patch.

Thanx, Paul

#4 lore raw
In reply to Shan Wei
Quoted 10 lines · Shan Wei
Signed-off-by Shan Wei <[email protected]>
---
 kernel/rcutree.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/rcutree.c +0 -0
index 74df86b..3a21fcf 100644

kernel/rcutree.c static void force_quiescent_state(struct rcu_state *rsp) · line 1963

Quoted 5 lines
kernel/rcutree.c +1 -1
@@ -1960,7 +1960,7 @@ static void force_quiescent_state(struct rcu_state *rsp)
 	struct rcu_node *rnp_old = NULL;
 
 	/* Funnel through hierarchy to reduce memory contention. */
-	rnp = per_cpu_ptr(rsp->rda, raw_smp_processor_id())->mynode;
+	rnp = this_cpu_ptr(rsp->rda)->mynode;

Do

	rnp = this_cpu_read(rsp->rda->mynode);

instad.

#5 lore raw
Quoted 20 lines · Christoph Lameter
Quoted 10 lines · Shan Wei
Signed-off-by Shan Wei <[email protected]>
---
 kernel/rcutree.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
kernel/rcutree.c +0 -0
index 74df86b..3a21fcf 100644

kernel/rcutree.c static void force_quiescent_state(struct rcu_state *rsp) · line 1963

Quoted 5 lines
kernel/rcutree.c +1 -1
@@ -1960,7 +1960,7 @@ static void force_quiescent_state(struct rcu_state *rsp)
 	struct rcu_node *rnp_old = NULL;
 
 	/* Funnel through hierarchy to reduce memory contention. */
-	rnp = per_cpu_ptr(rsp->rda, raw_smp_processor_id())->mynode;
+	rnp = this_cpu_ptr(rsp->rda)->mynode;

Do

Quoted 3 lines
	rnp = this_cpu_read(rsp->rda->mynode);

instad.

One thing to keep in mind -- the only purpose of this is to diffuse memory contention. So there is no need to disable preemption.

Thanx, Paul

author
#6 lore raw

Paul E. McKenney said, at 2012/11/1 4:08:

Quoted 3 lines

One thing to keep in mind -- the only purpose of this is to diffuse memory contention. So there is no need to disable preemption.

same question to me.
Christoph, maybe __this_cpu_read is a better choice which don't disable preemption.

Quoted 4 lines

Thanx, Paul

#7 lore raw
Quoted 7 lines · Shan Wei

Paul E. McKenney said, at 2012/11/1 4:08:

Quoted 3 lines

One thing to keep in mind -- the only purpose of this is to diffuse memory contention. So there is no need to disable preemption.

same question to me.
Christoph, maybe __this_cpu_read is a better choice which don't disable preemption.

Correct. Use __this_cpu_read to reduce overhead on processor that require emulation.