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

[Pranith Kumar]

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?

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