Re: [RFC PATCH 1/1] rcu: Use rcu_gp_kthread_wake() to wake up kthreads

[Paul E. McKenney]

On Sun, Jul 27, 2014 at 11:55:38AM -0400, Pranith Kumar wrote:
> Hi Paul,
> 
> On Fri, Jul 25, 2014 at 6:47 PM, Paul E. McKenney
> <paulmck@xxxxxxxxxxxxxxxxxx> 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?

No, in that case they actually can be re-ordered.

If two accesses are made while holding a given lock, then they cannot
be reordered, but only from the viewpoint of another access made while
holding that same lock.

This gets really involved really fast.  You therefore need to read
Documentation/memory-barriers.txt.

> The appropriate lock for ->gp_flags is rcu_node->lock.

Specifically, the root rcu_node structure's ->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.

Almost...  They are ordered because the accesses are to the exact same
variable -and- because they are protected by ACCESS_ONCE().  If there
was no ACCESS_ONCE(), both the CPU and the compiler could rearrange the
accesses.  (On many, but not all, architectures, the unlock-lock
pairs would act as full barriers.)

> 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().

Your answer does in fact appear to be correct, but the reasoning leading
to it is not completely sound.  Which is not bad, given that you appear
not to have read the documentation.  Therefore, once again, please read
Documentation/memory-barriers.txt.

							Thanx, Paul

> > 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@xxxxxxxxx>
> >> >> ---
> >> >>  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
> 

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