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(-)
diff --git a/kernel/rcutree.c b/kernel/rcutree.c
index 74df86b..3a21fcf 100644
--- a/kernel/rcutree.c
+++ b/kernel/rcutree.c
@@ -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);
--
1.7.1
On Wed, Oct 31, 2012 at 07:23:03PM +0800, Shan Wei wrote:
> 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
> ---
> kernel/rcutree.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> index 74df86b..3a21fcf 100644
> --- a/kernel/rcutree.c
> +++ b/kernel/rcutree.c
> @@ -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);
> --
> 1.7.1
>
Paul E. McKenney said, at 2012/10/31 19:51:
>
> 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.
>
> Thanx, Paul
On Wed, Oct 31, 2012 at 09:20:19PM +0800, Shan Wei wrote:
> Paul E. McKenney said, at 2012/10/31 19:51:
> >
> > 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.
Ah, got it. Please feel free to submit an updated patch.
Thanx, Paul
On Wed, 31 Oct 2012, Shan Wei wrote:
> Signed-off-by: Shan Wei <[email protected]>
> ---
> kernel/rcutree.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> index 74df86b..3a21fcf 100644
> --- a/kernel/rcutree.c
> +++ b/kernel/rcutree.c
> @@ -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.
On Wed, Oct 31, 2012 at 05:47:04PM +0000, Christoph Lameter wrote:
> On Wed, 31 Oct 2012, Shan Wei wrote:
>
> > Signed-off-by: Shan Wei <[email protected]>
> > ---
> > kernel/rcutree.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/kernel/rcutree.c b/kernel/rcutree.c
> > index 74df86b..3a21fcf 100644
> > --- a/kernel/rcutree.c
> > +++ b/kernel/rcutree.c
> > @@ -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.
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
Paul E. McKenney said, at 2012/11/1 4:08:
>
> 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.
>
> Thanx, Paul
>
>
On Thu, 1 Nov 2012, Shan Wei wrote:
> Paul E. McKenney said, at 2012/11/1 4:08:
> >
> > 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.