Messages in this thread | | | From | Pranith Kumar <> | Date | Sun, 27 Jul 2014 11:55:38 -0400 | Subject | Re: [RFC PATCH 1/1] rcu: Use rcu_gp_kthread_wake() to wake up kthreads |
| |
Hi Paul,
On Fri, Jul 25, 2014 at 6:47 PM, Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote: > On Fri, Jul 25, 2014 at 04:19:43PM -0400, Pranith Kumar wrote: >> >> I checked all the locations where gp_flags is being updated and the >> root node lock is held in all the cases. >> So I guess we can remove the comment too. > > And the accesses that matter (for some definition of "that matter") are > also similarly protected? > > An example of an access that doesn't matter is one that is followed up > by an access under the appropriate lock.
I am really new to having to think about the need for memory barriers, so please correct me if I am wrong.
So the idea here is that two consecutive accesses to ->gp_flags should not be re-ordered. If an access to ->gp_flags is followed by an access within a lock, the second access cannot be re-ordered with the first one and hence it will be safe, right?
The appropriate lock for ->gp_flags is rcu_node->lock. I see consecutive accesses to ->gp_flags without this lock only in force_quiescent_state()(we take fqslock there), but these accesses looks safe as they are in independent iterations of a loop. These cannot be rearranged by the compiler.
So all the accesses are safe from re-ordering and hence there is no need of a memory barrier for accessing ->gp_flags in rcu_gp_kthread_wake().
> > Anyway, if it is all locked properly, then yes, we should get rid of > the comment -- or replace it with a comment saying that barriers are > not needed due to locking. > > Thanx, Paul > >> >> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com> >> >> --- >> >> kernel/rcu/tree.c | 6 ++++-- >> >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> >> >> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c >> >> index 72e0b1f..d0e0d6e 100644 >> >> --- a/kernel/rcu/tree.c >> >> +++ b/kernel/rcu/tree.c >> >> @@ -1938,7 +1938,8 @@ static void rcu_report_qs_rsp(struct rcu_state *rsp, unsigned long flags) >> >> { >> >> WARN_ON_ONCE(!rcu_gp_in_progress(rsp)); >> >> raw_spin_unlock_irqrestore(&rcu_get_root(rsp)->lock, flags); >> >> - wake_up(&rsp->gp_wq); /* Memory barrier implied by wake_up() path. */ >> >> + /* Memory barrier implied by wake_up() path. */ >> >> + rcu_gp_kthread_wake(rsp); >> >> } >> >> >> >> /* >> >> @@ -2516,7 +2517,8 @@ static void force_quiescent_state(struct rcu_state *rsp) >> >> ACCESS_ONCE(rsp->gp_flags) = >> >> ACCESS_ONCE(rsp->gp_flags) | RCU_GP_FLAG_FQS; >> >> raw_spin_unlock_irqrestore(&rnp_old->lock, flags); >> >> - wake_up(&rsp->gp_wq); /* Memory barrier implied by wake_up() path. */ >> >> + /* Memory barrier implied by wake_up() path. */ >> >> + rcu_gp_kthread_wake(rsp); >> >> } >> >> >> >> /* >> >> -- >> >> 2.0.1 >> >> >> > >> >> >> >> -- >> Pranith >> >
-- Pranith
| |