lkml.org 
[lkml]   [2014]   [Jul]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH 1/1] rcu: Use rcu_gp_kthread_wake() to wake up kthreads
On Sun, Jul 27, 2014 at 12:44:30PM -0400, Pranith Kumar wrote:
> On Sun, Jul 27, 2014 at 12:29 PM, Paul E. McKenney
> <paulmck@linux.vnet.ibm.com> wrote:
> > 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@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?
> >
> > 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.
>
> OK, I think I will do that now and for the next few days :)
>
> >
> >> 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.)
>
> I was under the impression that ACCESS_ONCE() only defeated the
> compiler re-orderings. I did not know that it could defeat the CPU
> reordering too. I will look for the details in the documentation.

Search for "reorder successive loads to the same location" in
Documentation/memory-barriers.txt. More details are available in the
CPU reference manual.

> >> 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.
>
> When the answer is yes/no, I think I luckily landed on the correct
> side of 50% fence :)

I know that feeling! ;-)

Thanx, Paul

> --
> Pranith.
>
> >
> > 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@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
> >>
> >
>
>
>
> --
> Pranith
>



\
 
 \ /
  Last update: 2014-07-27 20:01    [W:0.081 / U:1.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site