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

[Pranith Kumar]

On Sun, Jul 27, 2014 at 12:29 PM, Paul E. McKenney
<paulmck@xxxxxxxxxxxxxxxxxx> 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@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.

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.


>
>> 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 :)

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



-- 
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/