Re: [PATCH v3 3/4] rcu: Use better variable names in funnel locking loop

[Paul E. McKenney]

On Mon, May 21, 2018 at 05:19:51PM -0700, Joel Fernandes wrote:
> On Mon, May 21, 2018 at 05:19:25PM -0700, Paul E. McKenney wrote:
> > On Mon, May 21, 2018 at 05:00:16PM -0700, Joel Fernandes wrote:
> > > On Mon, May 21, 2018 at 04:13:57PM -0700, Paul E. McKenney wrote:
> > > > > ---
> > > > >  kernel/rcu/tree.c | 48 
> > > > > ---
> > > > >  1 file changed, 25 insertions(+), 23 deletions(-)
> > > > > 
> > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > > index 0ffd41ba304f..879c67a31116 100644
> > > > > --- a/kernel/rcu/tree.c
> > > > > +++ b/kernel/rcu/tree.c
> > > > > @@ -1526,7 +1526,7 @@ static void trace_rcu_this_gp(struct rcu_node 
> > > > > *rnp, struct rcu_data *rdp,
> > > > > 
> > > > >  /*
> > > > >   * rcu_start_this_gp - Request the start of a particular grace period
> > > > > - * @rnp: The leaf node of the CPU from which to start.
> > > > > + * @rnp_start: The leaf node of the CPU from which to start.
> > > > >   * @rdp: The rcu_data corresponding to the CPU from which to start.
> > > > >   * @gp_seq_req: The gp_seq of the grace period to start.
> > > > >   *
> > > > > @@ -1540,12 +1540,12 @@ static void trace_rcu_this_gp(struct rcu_node 
> > > > > *rnp, struct rcu_data *rdp,
> > > > >   *
> > > > >   * Returns true if the GP thread needs to be awakened else false.
> > > > >   */
> > > > > -static bool rcu_start_this_gp(struct rcu_node *rnp, struct rcu_data 
> > > > > *rdp,
> > > > > +static bool rcu_start_this_gp(struct rcu_node *rnp_start, struct 
> > > > > rcu_data *rdp,
> > > > > unsigned long gp_seq_req)
> > > > >  {
> > > > >   bool ret = false;
> > > > >   struct rcu_state *rsp = rdp->rsp;
> > > > > - struct rcu_node *rnp_root;
> > > > > + struct rcu_node *rnp, *rnp_root = NULL;
> > > > 
> > > > Unless I am going blind, this patch really isn't using rnp_root.  It
> > > > could be removed.
> > > 
> > > Its just limitation of the diff tools. Your eyes are just fine and doing
> > > great based on your review comments ;)
> > > 
> > > The rnp_root is used after we break out of the loop.
> > > 
> > > > > 
> > > > >   /*
> > > > >* Use funnel locking to either acquire the root rcu_node
> > > > > @@ -1556,34 +1556,36 @@ static bool rcu_start_this_gp(struct rcu_node 
> > > > > *rnp, struct rcu_data *rdp,
> > > > >* scan the leaf rcu_node structures.  Note that rnp->lock must
> > > > >* not be released.
> > > > >*/
> > > > > - raw_lockdep_assert_held_rcu_node(rnp);
> > > > > - trace_rcu_this_gp(rnp, rdp, gp_seq_req, TPS("Startleaf"));
> > > > > - for (rnp_root = rnp; 1; rnp_root = rnp_root->parent) {
> > > > > - if (rnp_root != rnp)
> > > > > - raw_spin_lock_rcu_node(rnp_root);
> > > > > - if (ULONG_CMP_GE(rnp_root->gp_seq_needed, gp_seq_req) ||
> > > > > - rcu_seq_started(_root->gp_seq, gp_seq_req) ||
> > > > > - (rnp != rnp_root &&
> > > > > -  
> > > > > rcu_seq_state(rcu_seq_current(_root->gp_seq {
> > > > > - trace_rcu_this_gp(rnp_root, rdp, gp_seq_req,
> > > > > + raw_lockdep_assert_held_rcu_node(rnp_start);
> > > > > + trace_rcu_this_gp(rnp_start, rdp, gp_seq_req, TPS("Startleaf"));
> > > > > + for (rnp = rnp_start; 1; rnp = rnp->parent) {
> > > > > + if (rnp != rnp_start)
> > > > > + raw_spin_lock_rcu_node(rnp);
> > > > > + if (ULONG_CMP_GE(rnp->gp_seq_needed, gp_seq_req) ||
> > > > > + rcu_seq_started(>gp_seq, gp_seq_req) ||
> > > > > + (rnp != rnp_start &&
> > > > > +  rcu_seq_state(rcu_seq_current(>gp_seq {
> > > > > + trace_rcu_this_gp(rnp, rdp, gp_seq_req,
> > > > > TPS("Prestarted"));
> > > > >   goto unlock_out;
> > > > >   }
> > > > > - rnp_root->gp_seq_needed = gp_seq_req;
> > > > > - if (rcu_seq_state(rcu_seq_current(>gp_seq))) {
> > > > > + rnp->gp_seq_needed = gp_seq_req;
> > > > > + if (rcu_seq_state(rcu_seq_current(_start->gp_seq))) 
> > > > > {
> > > > 
> > > > The original had a performance bug, which is quite a bit more obvious
> > > > given the new names, so thank you for that!  The above statement should
> > > > instead be as follows:
> > > > 
> > > > if (rcu_seq_state(rcu_seq_current(>gp_seq))) {
> > > > 
> > > > It does not make sense to keep checking the starting rcu_node because
> > > > changes to ->gp_seq happen first at the top of the tree.  So we might
> > > > take an earlier exit by checking the current rnp instead of rechecking
> > > > rnp_start over and over.
> > > > 
> > > > Please feel free to make this change, which is probably best as a 
> > > > separate
> > > > patch.  That way this rename patch can remain a straightforward 

Re: [PATCH v3 3/4] rcu: Use better variable names in funnel locking loop

[Paul E. McKenney]

On Mon, May 21, 2018 at 05:19:51PM -0700, Joel Fernandes wrote:
> On Mon, May 21, 2018 at 05:19:25PM -0700, Paul E. McKenney wrote:
> > On Mon, May 21, 2018 at 05:00:16PM -0700, Joel Fernandes wrote:
> > > On Mon, May 21, 2018 at 04:13:57PM -0700, Paul E. McKenney wrote:
> > > > > ---
> > > > >  kernel/rcu/tree.c | 48 
> > > > > ---
> > > > >  1 file changed, 25 insertions(+), 23 deletions(-)
> > > > > 
> > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > > index 0ffd41ba304f..879c67a31116 100644
> > > > > --- a/kernel/rcu/tree.c
> > > > > +++ b/kernel/rcu/tree.c
> > > > > @@ -1526,7 +1526,7 @@ static void trace_rcu_this_gp(struct rcu_node 
> > > > > *rnp, struct rcu_data *rdp,
> > > > > 
> > > > >  /*
> > > > >   * rcu_start_this_gp - Request the start of a particular grace period
> > > > > - * @rnp: The leaf node of the CPU from which to start.
> > > > > + * @rnp_start: The leaf node of the CPU from which to start.
> > > > >   * @rdp: The rcu_data corresponding to the CPU from which to start.
> > > > >   * @gp_seq_req: The gp_seq of the grace period to start.
> > > > >   *
> > > > > @@ -1540,12 +1540,12 @@ static void trace_rcu_this_gp(struct rcu_node 
> > > > > *rnp, struct rcu_data *rdp,
> > > > >   *
> > > > >   * Returns true if the GP thread needs to be awakened else false.
> > > > >   */
> > > > > -static bool rcu_start_this_gp(struct rcu_node *rnp, struct rcu_data 
> > > > > *rdp,
> > > > > +static bool rcu_start_this_gp(struct rcu_node *rnp_start, struct 
> > > > > rcu_data *rdp,
> > > > > unsigned long gp_seq_req)
> > > > >  {
> > > > >   bool ret = false;
> > > > >   struct rcu_state *rsp = rdp->rsp;
> > > > > - struct rcu_node *rnp_root;
> > > > > + struct rcu_node *rnp, *rnp_root = NULL;
> > > > 
> > > > Unless I am going blind, this patch really isn't using rnp_root.  It
> > > > could be removed.
> > > 
> > > Its just limitation of the diff tools. Your eyes are just fine and doing
> > > great based on your review comments ;)
> > > 
> > > The rnp_root is used after we break out of the loop.
> > > 
> > > > > 
> > > > >   /*
> > > > >* Use funnel locking to either acquire the root rcu_node
> > > > > @@ -1556,34 +1556,36 @@ static bool rcu_start_this_gp(struct rcu_node 
> > > > > *rnp, struct rcu_data *rdp,
> > > > >* scan the leaf rcu_node structures.  Note that rnp->lock must
> > > > >* not be released.
> > > > >*/
> > > > > - raw_lockdep_assert_held_rcu_node(rnp);
> > > > > - trace_rcu_this_gp(rnp, rdp, gp_seq_req, TPS("Startleaf"));
> > > > > - for (rnp_root = rnp; 1; rnp_root = rnp_root->parent) {
> > > > > - if (rnp_root != rnp)
> > > > > - raw_spin_lock_rcu_node(rnp_root);
> > > > > - if (ULONG_CMP_GE(rnp_root->gp_seq_needed, gp_seq_req) ||
> > > > > - rcu_seq_started(_root->gp_seq, gp_seq_req) ||
> > > > > - (rnp != rnp_root &&
> > > > > -  
> > > > > rcu_seq_state(rcu_seq_current(_root->gp_seq {
> > > > > - trace_rcu_this_gp(rnp_root, rdp, gp_seq_req,
> > > > > + raw_lockdep_assert_held_rcu_node(rnp_start);
> > > > > + trace_rcu_this_gp(rnp_start, rdp, gp_seq_req, TPS("Startleaf"));
> > > > > + for (rnp = rnp_start; 1; rnp = rnp->parent) {
> > > > > + if (rnp != rnp_start)
> > > > > + raw_spin_lock_rcu_node(rnp);
> > > > > + if (ULONG_CMP_GE(rnp->gp_seq_needed, gp_seq_req) ||
> > > > > + rcu_seq_started(>gp_seq, gp_seq_req) ||
> > > > > + (rnp != rnp_start &&
> > > > > +  rcu_seq_state(rcu_seq_current(>gp_seq {
> > > > > + trace_rcu_this_gp(rnp, rdp, gp_seq_req,
> > > > > TPS("Prestarted"));
> > > > >   goto unlock_out;
> > > > >   }
> > > > > - rnp_root->gp_seq_needed = gp_seq_req;
> > > > > - if (rcu_seq_state(rcu_seq_current(>gp_seq))) {
> > > > > + rnp->gp_seq_needed = gp_seq_req;
> > > > > + if (rcu_seq_state(rcu_seq_current(_start->gp_seq))) 
> > > > > {
> > > > 
> > > > The original had a performance bug, which is quite a bit more obvious
> > > > given the new names, so thank you for that!  The above statement should
> > > > instead be as follows:
> > > > 
> > > > if (rcu_seq_state(rcu_seq_current(>gp_seq))) {
> > > > 
> > > > It does not make sense to keep checking the starting rcu_node because
> > > > changes to ->gp_seq happen first at the top of the tree.  So we might
> > > > take an earlier exit by checking the current rnp instead of rechecking
> > > > rnp_start over and over.
> > > > 
> > > > Please feel free to make this change, which is probably best as a 
> > > > separate
> > > > patch.  That way this rename patch can remain a straightforward 

Re: [PATCH v3 3/4] rcu: Use better variable names in funnel locking loop

[Joel Fernandes]

On Mon, May 21, 2018 at 05:19:25PM -0700, Paul E. McKenney wrote:
> On Mon, May 21, 2018 at 05:00:16PM -0700, Joel Fernandes wrote:
> > On Mon, May 21, 2018 at 04:13:57PM -0700, Paul E. McKenney wrote:
> > > > ---
> > > >  kernel/rcu/tree.c | 48 ---
> > > >  1 file changed, 25 insertions(+), 23 deletions(-)
> > > > 
> > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > index 0ffd41ba304f..879c67a31116 100644
> > > > --- a/kernel/rcu/tree.c
> > > > +++ b/kernel/rcu/tree.c
> > > > @@ -1526,7 +1526,7 @@ static void trace_rcu_this_gp(struct rcu_node 
> > > > *rnp, struct rcu_data *rdp,
> > > > 
> > > >  /*
> > > >   * rcu_start_this_gp - Request the start of a particular grace period
> > > > - * @rnp: The leaf node of the CPU from which to start.
> > > > + * @rnp_start: The leaf node of the CPU from which to start.
> > > >   * @rdp: The rcu_data corresponding to the CPU from which to start.
> > > >   * @gp_seq_req: The gp_seq of the grace period to start.
> > > >   *
> > > > @@ -1540,12 +1540,12 @@ static void trace_rcu_this_gp(struct rcu_node 
> > > > *rnp, struct rcu_data *rdp,
> > > >   *
> > > >   * Returns true if the GP thread needs to be awakened else false.
> > > >   */
> > > > -static bool rcu_start_this_gp(struct rcu_node *rnp, struct rcu_data 
> > > > *rdp,
> > > > +static bool rcu_start_this_gp(struct rcu_node *rnp_start, struct 
> > > > rcu_data *rdp,
> > > >   unsigned long gp_seq_req)
> > > >  {
> > > > bool ret = false;
> > > > struct rcu_state *rsp = rdp->rsp;
> > > > -   struct rcu_node *rnp_root;
> > > > +   struct rcu_node *rnp, *rnp_root = NULL;
> > > 
> > > Unless I am going blind, this patch really isn't using rnp_root.  It
> > > could be removed.
> > 
> > Its just limitation of the diff tools. Your eyes are just fine and doing
> > great based on your review comments ;)
> > 
> > The rnp_root is used after we break out of the loop.
> > 
> > > > 
> > > > /*
> > > >  * Use funnel locking to either acquire the root rcu_node
> > > > @@ -1556,34 +1556,36 @@ static bool rcu_start_this_gp(struct rcu_node 
> > > > *rnp, struct rcu_data *rdp,
> > > >  * scan the leaf rcu_node structures.  Note that rnp->lock must
> > > >  * not be released.
> > > >  */
> > > > -   raw_lockdep_assert_held_rcu_node(rnp);
> > > > -   trace_rcu_this_gp(rnp, rdp, gp_seq_req, TPS("Startleaf"));
> > > > -   for (rnp_root = rnp; 1; rnp_root = rnp_root->parent) {
> > > > -   if (rnp_root != rnp)
> > > > -   raw_spin_lock_rcu_node(rnp_root);
> > > > -   if (ULONG_CMP_GE(rnp_root->gp_seq_needed, gp_seq_req) ||
> > > > -   rcu_seq_started(_root->gp_seq, gp_seq_req) ||
> > > > -   (rnp != rnp_root &&
> > > > -
> > > > rcu_seq_state(rcu_seq_current(_root->gp_seq {
> > > > -   trace_rcu_this_gp(rnp_root, rdp, gp_seq_req,
> > > > +   raw_lockdep_assert_held_rcu_node(rnp_start);
> > > > +   trace_rcu_this_gp(rnp_start, rdp, gp_seq_req, TPS("Startleaf"));
> > > > +   for (rnp = rnp_start; 1; rnp = rnp->parent) {
> > > > +   if (rnp != rnp_start)
> > > > +   raw_spin_lock_rcu_node(rnp);
> > > > +   if (ULONG_CMP_GE(rnp->gp_seq_needed, gp_seq_req) ||
> > > > +   rcu_seq_started(>gp_seq, gp_seq_req) ||
> > > > +   (rnp != rnp_start &&
> > > > +rcu_seq_state(rcu_seq_current(>gp_seq {
> > > > +   trace_rcu_this_gp(rnp, rdp, gp_seq_req,
> > > >   TPS("Prestarted"));
> > > > goto unlock_out;
> > > > }
> > > > -   rnp_root->gp_seq_needed = gp_seq_req;
> > > > -   if (rcu_seq_state(rcu_seq_current(>gp_seq))) {
> > > > +   rnp->gp_seq_needed = gp_seq_req;
> > > > +   if (rcu_seq_state(rcu_seq_current(_start->gp_seq))) 
> > > > {
> > > 
> > > The original had a performance bug, which is quite a bit more obvious
> > > given the new names, so thank you for that!  The above statement should
> > > instead be as follows:
> > > 
> > >   if (rcu_seq_state(rcu_seq_current(>gp_seq))) {
> > > 
> > > It does not make sense to keep checking the starting rcu_node because
> > > changes to ->gp_seq happen first at the top of the tree.  So we might
> > > take an earlier exit by checking the current rnp instead of rechecking
> > > rnp_start over and over.
> > > 
> > > Please feel free to make this change, which is probably best as a separate
> > > patch.  That way this rename patch can remain a straightforward rename 
> > > patch.
> > 
> > Yes, sounds like a nice optimization and I'm glad my variable renaming 
> > helped
> > ;) I feel I should have seen it too. I can make this change and send out
> > with my next series 

Re: [PATCH v3 3/4] rcu: Use better variable names in funnel locking loop

[Joel Fernandes]

On Mon, May 21, 2018 at 05:19:25PM -0700, Paul E. McKenney wrote:
> On Mon, May 21, 2018 at 05:00:16PM -0700, Joel Fernandes wrote:
> > On Mon, May 21, 2018 at 04:13:57PM -0700, Paul E. McKenney wrote:
> > > > ---
> > > >  kernel/rcu/tree.c | 48 ---
> > > >  1 file changed, 25 insertions(+), 23 deletions(-)
> > > > 
> > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > index 0ffd41ba304f..879c67a31116 100644
> > > > --- a/kernel/rcu/tree.c
> > > > +++ b/kernel/rcu/tree.c
> > > > @@ -1526,7 +1526,7 @@ static void trace_rcu_this_gp(struct rcu_node 
> > > > *rnp, struct rcu_data *rdp,
> > > > 
> > > >  /*
> > > >   * rcu_start_this_gp - Request the start of a particular grace period
> > > > - * @rnp: The leaf node of the CPU from which to start.
> > > > + * @rnp_start: The leaf node of the CPU from which to start.
> > > >   * @rdp: The rcu_data corresponding to the CPU from which to start.
> > > >   * @gp_seq_req: The gp_seq of the grace period to start.
> > > >   *
> > > > @@ -1540,12 +1540,12 @@ static void trace_rcu_this_gp(struct rcu_node 
> > > > *rnp, struct rcu_data *rdp,
> > > >   *
> > > >   * Returns true if the GP thread needs to be awakened else false.
> > > >   */
> > > > -static bool rcu_start_this_gp(struct rcu_node *rnp, struct rcu_data 
> > > > *rdp,
> > > > +static bool rcu_start_this_gp(struct rcu_node *rnp_start, struct 
> > > > rcu_data *rdp,
> > > >   unsigned long gp_seq_req)
> > > >  {
> > > > bool ret = false;
> > > > struct rcu_state *rsp = rdp->rsp;
> > > > -   struct rcu_node *rnp_root;
> > > > +   struct rcu_node *rnp, *rnp_root = NULL;
> > > 
> > > Unless I am going blind, this patch really isn't using rnp_root.  It
> > > could be removed.
> > 
> > Its just limitation of the diff tools. Your eyes are just fine and doing
> > great based on your review comments ;)
> > 
> > The rnp_root is used after we break out of the loop.
> > 
> > > > 
> > > > /*
> > > >  * Use funnel locking to either acquire the root rcu_node
> > > > @@ -1556,34 +1556,36 @@ static bool rcu_start_this_gp(struct rcu_node 
> > > > *rnp, struct rcu_data *rdp,
> > > >  * scan the leaf rcu_node structures.  Note that rnp->lock must
> > > >  * not be released.
> > > >  */
> > > > -   raw_lockdep_assert_held_rcu_node(rnp);
> > > > -   trace_rcu_this_gp(rnp, rdp, gp_seq_req, TPS("Startleaf"));
> > > > -   for (rnp_root = rnp; 1; rnp_root = rnp_root->parent) {
> > > > -   if (rnp_root != rnp)
> > > > -   raw_spin_lock_rcu_node(rnp_root);
> > > > -   if (ULONG_CMP_GE(rnp_root->gp_seq_needed, gp_seq_req) ||
> > > > -   rcu_seq_started(_root->gp_seq, gp_seq_req) ||
> > > > -   (rnp != rnp_root &&
> > > > -
> > > > rcu_seq_state(rcu_seq_current(_root->gp_seq {
> > > > -   trace_rcu_this_gp(rnp_root, rdp, gp_seq_req,
> > > > +   raw_lockdep_assert_held_rcu_node(rnp_start);
> > > > +   trace_rcu_this_gp(rnp_start, rdp, gp_seq_req, TPS("Startleaf"));
> > > > +   for (rnp = rnp_start; 1; rnp = rnp->parent) {
> > > > +   if (rnp != rnp_start)
> > > > +   raw_spin_lock_rcu_node(rnp);
> > > > +   if (ULONG_CMP_GE(rnp->gp_seq_needed, gp_seq_req) ||
> > > > +   rcu_seq_started(>gp_seq, gp_seq_req) ||
> > > > +   (rnp != rnp_start &&
> > > > +rcu_seq_state(rcu_seq_current(>gp_seq {
> > > > +   trace_rcu_this_gp(rnp, rdp, gp_seq_req,
> > > >   TPS("Prestarted"));
> > > > goto unlock_out;
> > > > }
> > > > -   rnp_root->gp_seq_needed = gp_seq_req;
> > > > -   if (rcu_seq_state(rcu_seq_current(>gp_seq))) {
> > > > +   rnp->gp_seq_needed = gp_seq_req;
> > > > +   if (rcu_seq_state(rcu_seq_current(_start->gp_seq))) 
> > > > {
> > > 
> > > The original had a performance bug, which is quite a bit more obvious
> > > given the new names, so thank you for that!  The above statement should
> > > instead be as follows:
> > > 
> > >   if (rcu_seq_state(rcu_seq_current(>gp_seq))) {
> > > 
> > > It does not make sense to keep checking the starting rcu_node because
> > > changes to ->gp_seq happen first at the top of the tree.  So we might
> > > take an earlier exit by checking the current rnp instead of rechecking
> > > rnp_start over and over.
> > > 
> > > Please feel free to make this change, which is probably best as a separate
> > > patch.  That way this rename patch can remain a straightforward rename 
> > > patch.
> > 
> > Yes, sounds like a nice optimization and I'm glad my variable renaming 
> > helped
> > ;) I feel I should have seen it too. I can make this change and send out
> > with my next series 

Re: [PATCH v3 3/4] rcu: Use better variable names in funnel locking loop

[Paul E. McKenney]

On Mon, May 21, 2018 at 05:00:16PM -0700, Joel Fernandes wrote:
> On Mon, May 21, 2018 at 04:13:57PM -0700, Paul E. McKenney wrote:
> > > ---
> > >  kernel/rcu/tree.c | 48 ---
> > >  1 file changed, 25 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index 0ffd41ba304f..879c67a31116 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -1526,7 +1526,7 @@ static void trace_rcu_this_gp(struct rcu_node *rnp, 
> > > struct rcu_data *rdp,
> > > 
> > >  /*
> > >   * rcu_start_this_gp - Request the start of a particular grace period
> > > - * @rnp: The leaf node of the CPU from which to start.
> > > + * @rnp_start: The leaf node of the CPU from which to start.
> > >   * @rdp: The rcu_data corresponding to the CPU from which to start.
> > >   * @gp_seq_req: The gp_seq of the grace period to start.
> > >   *
> > > @@ -1540,12 +1540,12 @@ static void trace_rcu_this_gp(struct rcu_node 
> > > *rnp, struct rcu_data *rdp,
> > >   *
> > >   * Returns true if the GP thread needs to be awakened else false.
> > >   */
> > > -static bool rcu_start_this_gp(struct rcu_node *rnp, struct rcu_data *rdp,
> > > +static bool rcu_start_this_gp(struct rcu_node *rnp_start, struct 
> > > rcu_data *rdp,
> > > unsigned long gp_seq_req)
> > >  {
> > >   bool ret = false;
> > >   struct rcu_state *rsp = rdp->rsp;
> > > - struct rcu_node *rnp_root;
> > > + struct rcu_node *rnp, *rnp_root = NULL;
> > 
> > Unless I am going blind, this patch really isn't using rnp_root.  It
> > could be removed.
> 
> Its just limitation of the diff tools. Your eyes are just fine and doing
> great based on your review comments ;)
> 
> The rnp_root is used after we break out of the loop.
> 
> > > 
> > >   /*
> > >* Use funnel locking to either acquire the root rcu_node
> > > @@ -1556,34 +1556,36 @@ static bool rcu_start_this_gp(struct rcu_node 
> > > *rnp, struct rcu_data *rdp,
> > >* scan the leaf rcu_node structures.  Note that rnp->lock must
> > >* not be released.
> > >*/
> > > - raw_lockdep_assert_held_rcu_node(rnp);
> > > - trace_rcu_this_gp(rnp, rdp, gp_seq_req, TPS("Startleaf"));
> > > - for (rnp_root = rnp; 1; rnp_root = rnp_root->parent) {
> > > - if (rnp_root != rnp)
> > > - raw_spin_lock_rcu_node(rnp_root);
> > > - if (ULONG_CMP_GE(rnp_root->gp_seq_needed, gp_seq_req) ||
> > > - rcu_seq_started(_root->gp_seq, gp_seq_req) ||
> > > - (rnp != rnp_root &&
> > > -  rcu_seq_state(rcu_seq_current(_root->gp_seq {
> > > - trace_rcu_this_gp(rnp_root, rdp, gp_seq_req,
> > > + raw_lockdep_assert_held_rcu_node(rnp_start);
> > > + trace_rcu_this_gp(rnp_start, rdp, gp_seq_req, TPS("Startleaf"));
> > > + for (rnp = rnp_start; 1; rnp = rnp->parent) {
> > > + if (rnp != rnp_start)
> > > + raw_spin_lock_rcu_node(rnp);
> > > + if (ULONG_CMP_GE(rnp->gp_seq_needed, gp_seq_req) ||
> > > + rcu_seq_started(>gp_seq, gp_seq_req) ||
> > > + (rnp != rnp_start &&
> > > +  rcu_seq_state(rcu_seq_current(>gp_seq {
> > > + trace_rcu_this_gp(rnp, rdp, gp_seq_req,
> > > TPS("Prestarted"));
> > >   goto unlock_out;
> > >   }
> > > - rnp_root->gp_seq_needed = gp_seq_req;
> > > - if (rcu_seq_state(rcu_seq_current(>gp_seq))) {
> > > + rnp->gp_seq_needed = gp_seq_req;
> > > + if (rcu_seq_state(rcu_seq_current(_start->gp_seq))) {
> > 
> > The original had a performance bug, which is quite a bit more obvious
> > given the new names, so thank you for that!  The above statement should
> > instead be as follows:
> > 
> > if (rcu_seq_state(rcu_seq_current(>gp_seq))) {
> > 
> > It does not make sense to keep checking the starting rcu_node because
> > changes to ->gp_seq happen first at the top of the tree.  So we might
> > take an earlier exit by checking the current rnp instead of rechecking
> > rnp_start over and over.
> > 
> > Please feel free to make this change, which is probably best as a separate
> > patch.  That way this rename patch can remain a straightforward rename 
> > patch.
> 
> Yes, sounds like a nice optimization and I'm glad my variable renaming helped
> ;) I feel I should have seen it too. I can make this change and send out
> with my next series as you suggest.
> 
> > >   /*
> > >* We just marked the leaf, and a grace period
> > >* is in progress, which means that rcu_gp_cleanup()
> > >* will see the marking.  Bail to reduce contention.
> > >*/
> > > - trace_rcu_this_gp(rnp, rdp, gp_seq_req,
> > > + trace_rcu_this_gp(rnp_start, rdp, gp_seq_req,
> > > TPS("Startedleaf"));
> > >   

Re: [PATCH v3 3/4] rcu: Use better variable names in funnel locking loop

[Paul E. McKenney]

On Mon, May 21, 2018 at 05:00:16PM -0700, Joel Fernandes wrote:
> On Mon, May 21, 2018 at 04:13:57PM -0700, Paul E. McKenney wrote:
> > > ---
> > >  kernel/rcu/tree.c | 48 ---
> > >  1 file changed, 25 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index 0ffd41ba304f..879c67a31116 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -1526,7 +1526,7 @@ static void trace_rcu_this_gp(struct rcu_node *rnp, 
> > > struct rcu_data *rdp,
> > > 
> > >  /*
> > >   * rcu_start_this_gp - Request the start of a particular grace period
> > > - * @rnp: The leaf node of the CPU from which to start.
> > > + * @rnp_start: The leaf node of the CPU from which to start.
> > >   * @rdp: The rcu_data corresponding to the CPU from which to start.
> > >   * @gp_seq_req: The gp_seq of the grace period to start.
> > >   *
> > > @@ -1540,12 +1540,12 @@ static void trace_rcu_this_gp(struct rcu_node 
> > > *rnp, struct rcu_data *rdp,
> > >   *
> > >   * Returns true if the GP thread needs to be awakened else false.
> > >   */
> > > -static bool rcu_start_this_gp(struct rcu_node *rnp, struct rcu_data *rdp,
> > > +static bool rcu_start_this_gp(struct rcu_node *rnp_start, struct 
> > > rcu_data *rdp,
> > > unsigned long gp_seq_req)
> > >  {
> > >   bool ret = false;
> > >   struct rcu_state *rsp = rdp->rsp;
> > > - struct rcu_node *rnp_root;
> > > + struct rcu_node *rnp, *rnp_root = NULL;
> > 
> > Unless I am going blind, this patch really isn't using rnp_root.  It
> > could be removed.
> 
> Its just limitation of the diff tools. Your eyes are just fine and doing
> great based on your review comments ;)
> 
> The rnp_root is used after we break out of the loop.
> 
> > > 
> > >   /*
> > >* Use funnel locking to either acquire the root rcu_node
> > > @@ -1556,34 +1556,36 @@ static bool rcu_start_this_gp(struct rcu_node 
> > > *rnp, struct rcu_data *rdp,
> > >* scan the leaf rcu_node structures.  Note that rnp->lock must
> > >* not be released.
> > >*/
> > > - raw_lockdep_assert_held_rcu_node(rnp);
> > > - trace_rcu_this_gp(rnp, rdp, gp_seq_req, TPS("Startleaf"));
> > > - for (rnp_root = rnp; 1; rnp_root = rnp_root->parent) {
> > > - if (rnp_root != rnp)
> > > - raw_spin_lock_rcu_node(rnp_root);
> > > - if (ULONG_CMP_GE(rnp_root->gp_seq_needed, gp_seq_req) ||
> > > - rcu_seq_started(_root->gp_seq, gp_seq_req) ||
> > > - (rnp != rnp_root &&
> > > -  rcu_seq_state(rcu_seq_current(_root->gp_seq {
> > > - trace_rcu_this_gp(rnp_root, rdp, gp_seq_req,
> > > + raw_lockdep_assert_held_rcu_node(rnp_start);
> > > + trace_rcu_this_gp(rnp_start, rdp, gp_seq_req, TPS("Startleaf"));
> > > + for (rnp = rnp_start; 1; rnp = rnp->parent) {
> > > + if (rnp != rnp_start)
> > > + raw_spin_lock_rcu_node(rnp);
> > > + if (ULONG_CMP_GE(rnp->gp_seq_needed, gp_seq_req) ||
> > > + rcu_seq_started(>gp_seq, gp_seq_req) ||
> > > + (rnp != rnp_start &&
> > > +  rcu_seq_state(rcu_seq_current(>gp_seq {
> > > + trace_rcu_this_gp(rnp, rdp, gp_seq_req,
> > > TPS("Prestarted"));
> > >   goto unlock_out;
> > >   }
> > > - rnp_root->gp_seq_needed = gp_seq_req;
> > > - if (rcu_seq_state(rcu_seq_current(>gp_seq))) {
> > > + rnp->gp_seq_needed = gp_seq_req;
> > > + if (rcu_seq_state(rcu_seq_current(_start->gp_seq))) {
> > 
> > The original had a performance bug, which is quite a bit more obvious
> > given the new names, so thank you for that!  The above statement should
> > instead be as follows:
> > 
> > if (rcu_seq_state(rcu_seq_current(>gp_seq))) {
> > 
> > It does not make sense to keep checking the starting rcu_node because
> > changes to ->gp_seq happen first at the top of the tree.  So we might
> > take an earlier exit by checking the current rnp instead of rechecking
> > rnp_start over and over.
> > 
> > Please feel free to make this change, which is probably best as a separate
> > patch.  That way this rename patch can remain a straightforward rename 
> > patch.
> 
> Yes, sounds like a nice optimization and I'm glad my variable renaming helped
> ;) I feel I should have seen it too. I can make this change and send out
> with my next series as you suggest.
> 
> > >   /*
> > >* We just marked the leaf, and a grace period
> > >* is in progress, which means that rcu_gp_cleanup()
> > >* will see the marking.  Bail to reduce contention.
> > >*/
> > > - trace_rcu_this_gp(rnp, rdp, gp_seq_req,
> > > + trace_rcu_this_gp(rnp_start, rdp, gp_seq_req,
> > > TPS("Startedleaf"));
> > >   

Re: [PATCH v3 3/4] rcu: Use better variable names in funnel locking loop

[Joel Fernandes]

On Mon, May 21, 2018 at 04:13:57PM -0700, Paul E. McKenney wrote:
> > ---
> >  kernel/rcu/tree.c | 48 ---
> >  1 file changed, 25 insertions(+), 23 deletions(-)
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 0ffd41ba304f..879c67a31116 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -1526,7 +1526,7 @@ static void trace_rcu_this_gp(struct rcu_node *rnp, 
> > struct rcu_data *rdp,
> > 
> >  /*
> >   * rcu_start_this_gp - Request the start of a particular grace period
> > - * @rnp: The leaf node of the CPU from which to start.
> > + * @rnp_start: The leaf node of the CPU from which to start.
> >   * @rdp: The rcu_data corresponding to the CPU from which to start.
> >   * @gp_seq_req: The gp_seq of the grace period to start.
> >   *
> > @@ -1540,12 +1540,12 @@ static void trace_rcu_this_gp(struct rcu_node *rnp, 
> > struct rcu_data *rdp,
> >   *
> >   * Returns true if the GP thread needs to be awakened else false.
> >   */
> > -static bool rcu_start_this_gp(struct rcu_node *rnp, struct rcu_data *rdp,
> > +static bool rcu_start_this_gp(struct rcu_node *rnp_start, struct rcu_data 
> > *rdp,
> >   unsigned long gp_seq_req)
> >  {
> > bool ret = false;
> > struct rcu_state *rsp = rdp->rsp;
> > -   struct rcu_node *rnp_root;
> > +   struct rcu_node *rnp, *rnp_root = NULL;
> 
> Unless I am going blind, this patch really isn't using rnp_root.  It
> could be removed.

Its just limitation of the diff tools. Your eyes are just fine and doing
great based on your review comments ;)

The rnp_root is used after we break out of the loop.

> > 
> > /*
> >  * Use funnel locking to either acquire the root rcu_node
> > @@ -1556,34 +1556,36 @@ static bool rcu_start_this_gp(struct rcu_node *rnp, 
> > struct rcu_data *rdp,
> >  * scan the leaf rcu_node structures.  Note that rnp->lock must
> >  * not be released.
> >  */
> > -   raw_lockdep_assert_held_rcu_node(rnp);
> > -   trace_rcu_this_gp(rnp, rdp, gp_seq_req, TPS("Startleaf"));
> > -   for (rnp_root = rnp; 1; rnp_root = rnp_root->parent) {
> > -   if (rnp_root != rnp)
> > -   raw_spin_lock_rcu_node(rnp_root);
> > -   if (ULONG_CMP_GE(rnp_root->gp_seq_needed, gp_seq_req) ||
> > -   rcu_seq_started(_root->gp_seq, gp_seq_req) ||
> > -   (rnp != rnp_root &&
> > -rcu_seq_state(rcu_seq_current(_root->gp_seq {
> > -   trace_rcu_this_gp(rnp_root, rdp, gp_seq_req,
> > +   raw_lockdep_assert_held_rcu_node(rnp_start);
> > +   trace_rcu_this_gp(rnp_start, rdp, gp_seq_req, TPS("Startleaf"));
> > +   for (rnp = rnp_start; 1; rnp = rnp->parent) {
> > +   if (rnp != rnp_start)
> > +   raw_spin_lock_rcu_node(rnp);
> > +   if (ULONG_CMP_GE(rnp->gp_seq_needed, gp_seq_req) ||
> > +   rcu_seq_started(>gp_seq, gp_seq_req) ||
> > +   (rnp != rnp_start &&
> > +rcu_seq_state(rcu_seq_current(>gp_seq {
> > +   trace_rcu_this_gp(rnp, rdp, gp_seq_req,
> >   TPS("Prestarted"));
> > goto unlock_out;
> > }
> > -   rnp_root->gp_seq_needed = gp_seq_req;
> > -   if (rcu_seq_state(rcu_seq_current(>gp_seq))) {
> > +   rnp->gp_seq_needed = gp_seq_req;
> > +   if (rcu_seq_state(rcu_seq_current(_start->gp_seq))) {
> 
> The original had a performance bug, which is quite a bit more obvious
> given the new names, so thank you for that!  The above statement should
> instead be as follows:
> 
>   if (rcu_seq_state(rcu_seq_current(>gp_seq))) {
> 
> It does not make sense to keep checking the starting rcu_node because
> changes to ->gp_seq happen first at the top of the tree.  So we might
> take an earlier exit by checking the current rnp instead of rechecking
> rnp_start over and over.
> 
> Please feel free to make this change, which is probably best as a separate
> patch.  That way this rename patch can remain a straightforward rename patch.

Yes, sounds like a nice optimization and I'm glad my variable renaming helped
;) I feel I should have seen it too. I can make this change and send out
with my next series as you suggest.

> > /*
> >  * We just marked the leaf, and a grace period
> >  * is in progress, which means that rcu_gp_cleanup()
> >  * will see the marking.  Bail to reduce contention.
> >  */
> > -   trace_rcu_this_gp(rnp, rdp, gp_seq_req,
> > +   trace_rcu_this_gp(rnp_start, rdp, gp_seq_req,
> >   TPS("Startedleaf"));
> > goto unlock_out;
> > }
> > -   if (rnp_root != rnp && rnp_root->parent != NULL)
> > -   raw_spin_unlock_rcu_node(rnp_root);
> > -   

Re: [PATCH v3 3/4] rcu: Use better variable names in funnel locking loop

[Joel Fernandes]

On Mon, May 21, 2018 at 04:13:57PM -0700, Paul E. McKenney wrote:
> > ---
> >  kernel/rcu/tree.c | 48 ---
> >  1 file changed, 25 insertions(+), 23 deletions(-)
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 0ffd41ba304f..879c67a31116 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -1526,7 +1526,7 @@ static void trace_rcu_this_gp(struct rcu_node *rnp, 
> > struct rcu_data *rdp,
> > 
> >  /*
> >   * rcu_start_this_gp - Request the start of a particular grace period
> > - * @rnp: The leaf node of the CPU from which to start.
> > + * @rnp_start: The leaf node of the CPU from which to start.
> >   * @rdp: The rcu_data corresponding to the CPU from which to start.
> >   * @gp_seq_req: The gp_seq of the grace period to start.
> >   *
> > @@ -1540,12 +1540,12 @@ static void trace_rcu_this_gp(struct rcu_node *rnp, 
> > struct rcu_data *rdp,
> >   *
> >   * Returns true if the GP thread needs to be awakened else false.
> >   */
> > -static bool rcu_start_this_gp(struct rcu_node *rnp, struct rcu_data *rdp,
> > +static bool rcu_start_this_gp(struct rcu_node *rnp_start, struct rcu_data 
> > *rdp,
> >   unsigned long gp_seq_req)
> >  {
> > bool ret = false;
> > struct rcu_state *rsp = rdp->rsp;
> > -   struct rcu_node *rnp_root;
> > +   struct rcu_node *rnp, *rnp_root = NULL;
> 
> Unless I am going blind, this patch really isn't using rnp_root.  It
> could be removed.

Its just limitation of the diff tools. Your eyes are just fine and doing
great based on your review comments ;)

The rnp_root is used after we break out of the loop.

> > 
> > /*
> >  * Use funnel locking to either acquire the root rcu_node
> > @@ -1556,34 +1556,36 @@ static bool rcu_start_this_gp(struct rcu_node *rnp, 
> > struct rcu_data *rdp,
> >  * scan the leaf rcu_node structures.  Note that rnp->lock must
> >  * not be released.
> >  */
> > -   raw_lockdep_assert_held_rcu_node(rnp);
> > -   trace_rcu_this_gp(rnp, rdp, gp_seq_req, TPS("Startleaf"));
> > -   for (rnp_root = rnp; 1; rnp_root = rnp_root->parent) {
> > -   if (rnp_root != rnp)
> > -   raw_spin_lock_rcu_node(rnp_root);
> > -   if (ULONG_CMP_GE(rnp_root->gp_seq_needed, gp_seq_req) ||
> > -   rcu_seq_started(_root->gp_seq, gp_seq_req) ||
> > -   (rnp != rnp_root &&
> > -rcu_seq_state(rcu_seq_current(_root->gp_seq {
> > -   trace_rcu_this_gp(rnp_root, rdp, gp_seq_req,
> > +   raw_lockdep_assert_held_rcu_node(rnp_start);
> > +   trace_rcu_this_gp(rnp_start, rdp, gp_seq_req, TPS("Startleaf"));
> > +   for (rnp = rnp_start; 1; rnp = rnp->parent) {
> > +   if (rnp != rnp_start)
> > +   raw_spin_lock_rcu_node(rnp);
> > +   if (ULONG_CMP_GE(rnp->gp_seq_needed, gp_seq_req) ||
> > +   rcu_seq_started(>gp_seq, gp_seq_req) ||
> > +   (rnp != rnp_start &&
> > +rcu_seq_state(rcu_seq_current(>gp_seq {
> > +   trace_rcu_this_gp(rnp, rdp, gp_seq_req,
> >   TPS("Prestarted"));
> > goto unlock_out;
> > }
> > -   rnp_root->gp_seq_needed = gp_seq_req;
> > -   if (rcu_seq_state(rcu_seq_current(>gp_seq))) {
> > +   rnp->gp_seq_needed = gp_seq_req;
> > +   if (rcu_seq_state(rcu_seq_current(_start->gp_seq))) {
> 
> The original had a performance bug, which is quite a bit more obvious
> given the new names, so thank you for that!  The above statement should
> instead be as follows:
> 
>   if (rcu_seq_state(rcu_seq_current(>gp_seq))) {
> 
> It does not make sense to keep checking the starting rcu_node because
> changes to ->gp_seq happen first at the top of the tree.  So we might
> take an earlier exit by checking the current rnp instead of rechecking
> rnp_start over and over.
> 
> Please feel free to make this change, which is probably best as a separate
> patch.  That way this rename patch can remain a straightforward rename patch.

Yes, sounds like a nice optimization and I'm glad my variable renaming helped
;) I feel I should have seen it too. I can make this change and send out
with my next series as you suggest.

> > /*
> >  * We just marked the leaf, and a grace period
> >  * is in progress, which means that rcu_gp_cleanup()
> >  * will see the marking.  Bail to reduce contention.
> >  */
> > -   trace_rcu_this_gp(rnp, rdp, gp_seq_req,
> > +   trace_rcu_this_gp(rnp_start, rdp, gp_seq_req,
> >   TPS("Startedleaf"));
> > goto unlock_out;
> > }
> > -   if (rnp_root != rnp && rnp_root->parent != NULL)
> > -   raw_spin_unlock_rcu_node(rnp_root);
> > -   

Re: [PATCH v3 3/4] rcu: Use better variable names in funnel locking loop

[Paul E. McKenney]

On Sun, May 20, 2018 at 09:42:19PM -0700, Joel Fernandes wrote:
> The funnel locking loop in rcu_start_this_gp uses rcu_root as a
> temporary variable while walking the combining tree. This causes a
> tiresome exercise of a code reader reminding themselves that rcu_root
> may not be root. Lets just call it rnp, and rename other variables as
> well to be more appropriate.
> 
> Original patch: https://patchwork.kernel.org/patch/10396577/
> 
> Signed-off-by: Joel Fernandes 

Nice!

Please see feedback interspersed below.

Thanx, Paul

> ---
>  kernel/rcu/tree.c | 48 ---
>  1 file changed, 25 insertions(+), 23 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 0ffd41ba304f..879c67a31116 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -1526,7 +1526,7 @@ static void trace_rcu_this_gp(struct rcu_node *rnp, 
> struct rcu_data *rdp,
> 
>  /*
>   * rcu_start_this_gp - Request the start of a particular grace period
> - * @rnp: The leaf node of the CPU from which to start.
> + * @rnp_start: The leaf node of the CPU from which to start.
>   * @rdp: The rcu_data corresponding to the CPU from which to start.
>   * @gp_seq_req: The gp_seq of the grace period to start.
>   *
> @@ -1540,12 +1540,12 @@ static void trace_rcu_this_gp(struct rcu_node *rnp, 
> struct rcu_data *rdp,
>   *
>   * Returns true if the GP thread needs to be awakened else false.
>   */
> -static bool rcu_start_this_gp(struct rcu_node *rnp, struct rcu_data *rdp,
> +static bool rcu_start_this_gp(struct rcu_node *rnp_start, struct rcu_data 
> *rdp,
> unsigned long gp_seq_req)
>  {
>   bool ret = false;
>   struct rcu_state *rsp = rdp->rsp;
> - struct rcu_node *rnp_root;
> + struct rcu_node *rnp, *rnp_root = NULL;

Unless I am going blind, this patch really isn't using rnp_root.  It
could be removed.

> 
>   /*
>* Use funnel locking to either acquire the root rcu_node
> @@ -1556,34 +1556,36 @@ static bool rcu_start_this_gp(struct rcu_node *rnp, 
> struct rcu_data *rdp,
>* scan the leaf rcu_node structures.  Note that rnp->lock must
>* not be released.
>*/
> - raw_lockdep_assert_held_rcu_node(rnp);
> - trace_rcu_this_gp(rnp, rdp, gp_seq_req, TPS("Startleaf"));
> - for (rnp_root = rnp; 1; rnp_root = rnp_root->parent) {
> - if (rnp_root != rnp)
> - raw_spin_lock_rcu_node(rnp_root);
> - if (ULONG_CMP_GE(rnp_root->gp_seq_needed, gp_seq_req) ||
> - rcu_seq_started(_root->gp_seq, gp_seq_req) ||
> - (rnp != rnp_root &&
> -  rcu_seq_state(rcu_seq_current(_root->gp_seq {
> - trace_rcu_this_gp(rnp_root, rdp, gp_seq_req,
> + raw_lockdep_assert_held_rcu_node(rnp_start);
> + trace_rcu_this_gp(rnp_start, rdp, gp_seq_req, TPS("Startleaf"));
> + for (rnp = rnp_start; 1; rnp = rnp->parent) {
> + if (rnp != rnp_start)
> + raw_spin_lock_rcu_node(rnp);
> + if (ULONG_CMP_GE(rnp->gp_seq_needed, gp_seq_req) ||
> + rcu_seq_started(>gp_seq, gp_seq_req) ||
> + (rnp != rnp_start &&
> +  rcu_seq_state(rcu_seq_current(>gp_seq {
> + trace_rcu_this_gp(rnp, rdp, gp_seq_req,
> TPS("Prestarted"));
>   goto unlock_out;
>   }
> - rnp_root->gp_seq_needed = gp_seq_req;
> - if (rcu_seq_state(rcu_seq_current(>gp_seq))) {
> + rnp->gp_seq_needed = gp_seq_req;
> + if (rcu_seq_state(rcu_seq_current(_start->gp_seq))) {

The original had a performance bug, which is quite a bit more obvious
given the new names, so thank you for that!  The above statement should
instead be as follows:

if (rcu_seq_state(rcu_seq_current(>gp_seq))) {

It does not make sense to keep checking the starting rcu_node because
changes to ->gp_seq happen first at the top of the tree.  So we might
take an earlier exit by checking the current rnp instead of rechecking
rnp_start over and over.

Please feel free to make this change, which is probably best as a separate
patch.  That way this rename patch can remain a straightforward rename patch.

>   /*
>* We just marked the leaf, and a grace period
>* is in progress, which means that rcu_gp_cleanup()
>* will see the marking.  Bail to reduce contention.
>*/
> - trace_rcu_this_gp(rnp, rdp, gp_seq_req,
> + trace_rcu_this_gp(rnp_start, rdp, gp_seq_req,
> TPS("Startedleaf"));
>   goto unlock_out;
>   }
> - if (rnp_root != rnp && 

Re: [PATCH v3 3/4] rcu: Use better variable names in funnel locking loop

[Paul E. McKenney]

On Sun, May 20, 2018 at 09:42:19PM -0700, Joel Fernandes wrote:
> The funnel locking loop in rcu_start_this_gp uses rcu_root as a
> temporary variable while walking the combining tree. This causes a
> tiresome exercise of a code reader reminding themselves that rcu_root
> may not be root. Lets just call it rnp, and rename other variables as
> well to be more appropriate.
> 
> Original patch: https://patchwork.kernel.org/patch/10396577/
> 
> Signed-off-by: Joel Fernandes 

Nice!

Please see feedback interspersed below.

Thanx, Paul

> ---
>  kernel/rcu/tree.c | 48 ---
>  1 file changed, 25 insertions(+), 23 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 0ffd41ba304f..879c67a31116 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -1526,7 +1526,7 @@ static void trace_rcu_this_gp(struct rcu_node *rnp, 
> struct rcu_data *rdp,
> 
>  /*
>   * rcu_start_this_gp - Request the start of a particular grace period
> - * @rnp: The leaf node of the CPU from which to start.
> + * @rnp_start: The leaf node of the CPU from which to start.
>   * @rdp: The rcu_data corresponding to the CPU from which to start.
>   * @gp_seq_req: The gp_seq of the grace period to start.
>   *
> @@ -1540,12 +1540,12 @@ static void trace_rcu_this_gp(struct rcu_node *rnp, 
> struct rcu_data *rdp,
>   *
>   * Returns true if the GP thread needs to be awakened else false.
>   */
> -static bool rcu_start_this_gp(struct rcu_node *rnp, struct rcu_data *rdp,
> +static bool rcu_start_this_gp(struct rcu_node *rnp_start, struct rcu_data 
> *rdp,
> unsigned long gp_seq_req)
>  {
>   bool ret = false;
>   struct rcu_state *rsp = rdp->rsp;
> - struct rcu_node *rnp_root;
> + struct rcu_node *rnp, *rnp_root = NULL;

Unless I am going blind, this patch really isn't using rnp_root.  It
could be removed.

> 
>   /*
>* Use funnel locking to either acquire the root rcu_node
> @@ -1556,34 +1556,36 @@ static bool rcu_start_this_gp(struct rcu_node *rnp, 
> struct rcu_data *rdp,
>* scan the leaf rcu_node structures.  Note that rnp->lock must
>* not be released.
>*/
> - raw_lockdep_assert_held_rcu_node(rnp);
> - trace_rcu_this_gp(rnp, rdp, gp_seq_req, TPS("Startleaf"));
> - for (rnp_root = rnp; 1; rnp_root = rnp_root->parent) {
> - if (rnp_root != rnp)
> - raw_spin_lock_rcu_node(rnp_root);
> - if (ULONG_CMP_GE(rnp_root->gp_seq_needed, gp_seq_req) ||
> - rcu_seq_started(_root->gp_seq, gp_seq_req) ||
> - (rnp != rnp_root &&
> -  rcu_seq_state(rcu_seq_current(_root->gp_seq {
> - trace_rcu_this_gp(rnp_root, rdp, gp_seq_req,
> + raw_lockdep_assert_held_rcu_node(rnp_start);
> + trace_rcu_this_gp(rnp_start, rdp, gp_seq_req, TPS("Startleaf"));
> + for (rnp = rnp_start; 1; rnp = rnp->parent) {
> + if (rnp != rnp_start)
> + raw_spin_lock_rcu_node(rnp);
> + if (ULONG_CMP_GE(rnp->gp_seq_needed, gp_seq_req) ||
> + rcu_seq_started(>gp_seq, gp_seq_req) ||
> + (rnp != rnp_start &&
> +  rcu_seq_state(rcu_seq_current(>gp_seq {
> + trace_rcu_this_gp(rnp, rdp, gp_seq_req,
> TPS("Prestarted"));
>   goto unlock_out;
>   }
> - rnp_root->gp_seq_needed = gp_seq_req;
> - if (rcu_seq_state(rcu_seq_current(>gp_seq))) {
> + rnp->gp_seq_needed = gp_seq_req;
> + if (rcu_seq_state(rcu_seq_current(_start->gp_seq))) {

The original had a performance bug, which is quite a bit more obvious
given the new names, so thank you for that!  The above statement should
instead be as follows:

if (rcu_seq_state(rcu_seq_current(>gp_seq))) {

It does not make sense to keep checking the starting rcu_node because
changes to ->gp_seq happen first at the top of the tree.  So we might
take an earlier exit by checking the current rnp instead of rechecking
rnp_start over and over.

Please feel free to make this change, which is probably best as a separate
patch.  That way this rename patch can remain a straightforward rename patch.

>   /*
>* We just marked the leaf, and a grace period
>* is in progress, which means that rcu_gp_cleanup()
>* will see the marking.  Bail to reduce contention.
>*/
> - trace_rcu_this_gp(rnp, rdp, gp_seq_req,
> + trace_rcu_this_gp(rnp_start, rdp, gp_seq_req,
> TPS("Startedleaf"));
>   goto unlock_out;
>   }
> - if (rnp_root != rnp && rnp_root->parent != NULL)
> - 

[PATCH v3 3/4] rcu: Use better variable names in funnel locking loop

[Joel Fernandes]

The funnel locking loop in rcu_start_this_gp uses rcu_root as a
temporary variable while walking the combining tree. This causes a
tiresome exercise of a code reader reminding themselves that rcu_root
may not be root. Lets just call it rnp, and rename other variables as
well to be more appropriate.

Original patch: https://patchwork.kernel.org/patch/10396577/

Signed-off-by: Joel Fernandes 
---
 kernel/rcu/tree.c | 48 ---
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 0ffd41ba304f..879c67a31116 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -1526,7 +1526,7 @@ static void trace_rcu_this_gp(struct rcu_node *rnp, 
struct rcu_data *rdp,
 
 /*
  * rcu_start_this_gp - Request the start of a particular grace period
- * @rnp: The leaf node of the CPU from which to start.
+ * @rnp_start: The leaf node of the CPU from which to start.
  * @rdp: The rcu_data corresponding to the CPU from which to start.
  * @gp_seq_req: The gp_seq of the grace period to start.
  *
@@ -1540,12 +1540,12 @@ static void trace_rcu_this_gp(struct rcu_node *rnp, 
struct rcu_data *rdp,
  *
  * Returns true if the GP thread needs to be awakened else false.
  */
-static bool rcu_start_this_gp(struct rcu_node *rnp, struct rcu_data *rdp,
+static bool rcu_start_this_gp(struct rcu_node *rnp_start, struct rcu_data *rdp,
  unsigned long gp_seq_req)
 {
bool ret = false;
struct rcu_state *rsp = rdp->rsp;
-   struct rcu_node *rnp_root;
+   struct rcu_node *rnp, *rnp_root = NULL;
 
/*
 * Use funnel locking to either acquire the root rcu_node
@@ -1556,34 +1556,36 @@ static bool rcu_start_this_gp(struct rcu_node *rnp, 
struct rcu_data *rdp,
 * scan the leaf rcu_node structures.  Note that rnp->lock must
 * not be released.
 */
-   raw_lockdep_assert_held_rcu_node(rnp);
-   trace_rcu_this_gp(rnp, rdp, gp_seq_req, TPS("Startleaf"));
-   for (rnp_root = rnp; 1; rnp_root = rnp_root->parent) {
-   if (rnp_root != rnp)
-   raw_spin_lock_rcu_node(rnp_root);
-   if (ULONG_CMP_GE(rnp_root->gp_seq_needed, gp_seq_req) ||
-   rcu_seq_started(_root->gp_seq, gp_seq_req) ||
-   (rnp != rnp_root &&
-rcu_seq_state(rcu_seq_current(_root->gp_seq {
-   trace_rcu_this_gp(rnp_root, rdp, gp_seq_req,
+   raw_lockdep_assert_held_rcu_node(rnp_start);
+   trace_rcu_this_gp(rnp_start, rdp, gp_seq_req, TPS("Startleaf"));
+   for (rnp = rnp_start; 1; rnp = rnp->parent) {
+   if (rnp != rnp_start)
+   raw_spin_lock_rcu_node(rnp);
+   if (ULONG_CMP_GE(rnp->gp_seq_needed, gp_seq_req) ||
+   rcu_seq_started(>gp_seq, gp_seq_req) ||
+   (rnp != rnp_start &&
+rcu_seq_state(rcu_seq_current(>gp_seq {
+   trace_rcu_this_gp(rnp, rdp, gp_seq_req,
  TPS("Prestarted"));
goto unlock_out;
}
-   rnp_root->gp_seq_needed = gp_seq_req;
-   if (rcu_seq_state(rcu_seq_current(>gp_seq))) {
+   rnp->gp_seq_needed = gp_seq_req;
+   if (rcu_seq_state(rcu_seq_current(_start->gp_seq))) {
/*
 * We just marked the leaf, and a grace period
 * is in progress, which means that rcu_gp_cleanup()
 * will see the marking.  Bail to reduce contention.
 */
-   trace_rcu_this_gp(rnp, rdp, gp_seq_req,
+   trace_rcu_this_gp(rnp_start, rdp, gp_seq_req,
  TPS("Startedleaf"));
goto unlock_out;
}
-   if (rnp_root != rnp && rnp_root->parent != NULL)
-   raw_spin_unlock_rcu_node(rnp_root);
-   if (!rnp_root->parent)
+   if (rnp != rnp_start && rnp->parent != NULL)
+   raw_spin_unlock_rcu_node(rnp);
+   if (!rnp->parent) {
+   rnp_root = rnp;
break;  /* At root, and perhaps also leaf. */
+   }
}
 
/* If GP already in progress, just leave, otherwise start one. */
@@ -1601,11 +1603,11 @@ static bool rcu_start_this_gp(struct rcu_node *rnp, 
struct rcu_data *rdp,
trace_rcu_grace_period(rsp->name, READ_ONCE(rsp->gp_seq), 
TPS("newreq"));
ret = true;  /* Caller must wake GP kthread. */
 unlock_out:
-   if (rnp != rnp_root)
-   raw_spin_unlock_rcu_node(rnp_root);
+   if (rnp != rnp_start)
+   raw_spin_unlock_rcu_node(rnp);
/* Push furthest requested GP to leaf node and rcu_data 

[PATCH v3 3/4] rcu: Use better variable names in funnel locking loop

[Joel Fernandes]

The funnel locking loop in rcu_start_this_gp uses rcu_root as a
temporary variable while walking the combining tree. This causes a
tiresome exercise of a code reader reminding themselves that rcu_root
may not be root. Lets just call it rnp, and rename other variables as
well to be more appropriate.

Original patch: https://patchwork.kernel.org/patch/10396577/

Signed-off-by: Joel Fernandes 
---
 kernel/rcu/tree.c | 48 ---
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 0ffd41ba304f..879c67a31116 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -1526,7 +1526,7 @@ static void trace_rcu_this_gp(struct rcu_node *rnp, 
struct rcu_data *rdp,
 
 /*
  * rcu_start_this_gp - Request the start of a particular grace period
- * @rnp: The leaf node of the CPU from which to start.
+ * @rnp_start: The leaf node of the CPU from which to start.
  * @rdp: The rcu_data corresponding to the CPU from which to start.
  * @gp_seq_req: The gp_seq of the grace period to start.
  *
@@ -1540,12 +1540,12 @@ static void trace_rcu_this_gp(struct rcu_node *rnp, 
struct rcu_data *rdp,
  *
  * Returns true if the GP thread needs to be awakened else false.
  */
-static bool rcu_start_this_gp(struct rcu_node *rnp, struct rcu_data *rdp,
+static bool rcu_start_this_gp(struct rcu_node *rnp_start, struct rcu_data *rdp,
  unsigned long gp_seq_req)
 {
bool ret = false;
struct rcu_state *rsp = rdp->rsp;
-   struct rcu_node *rnp_root;
+   struct rcu_node *rnp, *rnp_root = NULL;
 
/*
 * Use funnel locking to either acquire the root rcu_node
@@ -1556,34 +1556,36 @@ static bool rcu_start_this_gp(struct rcu_node *rnp, 
struct rcu_data *rdp,
 * scan the leaf rcu_node structures.  Note that rnp->lock must
 * not be released.
 */
-   raw_lockdep_assert_held_rcu_node(rnp);
-   trace_rcu_this_gp(rnp, rdp, gp_seq_req, TPS("Startleaf"));
-   for (rnp_root = rnp; 1; rnp_root = rnp_root->parent) {
-   if (rnp_root != rnp)
-   raw_spin_lock_rcu_node(rnp_root);
-   if (ULONG_CMP_GE(rnp_root->gp_seq_needed, gp_seq_req) ||
-   rcu_seq_started(_root->gp_seq, gp_seq_req) ||
-   (rnp != rnp_root &&
-rcu_seq_state(rcu_seq_current(_root->gp_seq {
-   trace_rcu_this_gp(rnp_root, rdp, gp_seq_req,
+   raw_lockdep_assert_held_rcu_node(rnp_start);
+   trace_rcu_this_gp(rnp_start, rdp, gp_seq_req, TPS("Startleaf"));
+   for (rnp = rnp_start; 1; rnp = rnp->parent) {
+   if (rnp != rnp_start)
+   raw_spin_lock_rcu_node(rnp);
+   if (ULONG_CMP_GE(rnp->gp_seq_needed, gp_seq_req) ||
+   rcu_seq_started(>gp_seq, gp_seq_req) ||
+   (rnp != rnp_start &&
+rcu_seq_state(rcu_seq_current(>gp_seq {
+   trace_rcu_this_gp(rnp, rdp, gp_seq_req,
  TPS("Prestarted"));
goto unlock_out;
}
-   rnp_root->gp_seq_needed = gp_seq_req;
-   if (rcu_seq_state(rcu_seq_current(>gp_seq))) {
+   rnp->gp_seq_needed = gp_seq_req;
+   if (rcu_seq_state(rcu_seq_current(_start->gp_seq))) {
/*
 * We just marked the leaf, and a grace period
 * is in progress, which means that rcu_gp_cleanup()
 * will see the marking.  Bail to reduce contention.
 */
-   trace_rcu_this_gp(rnp, rdp, gp_seq_req,
+   trace_rcu_this_gp(rnp_start, rdp, gp_seq_req,
  TPS("Startedleaf"));
goto unlock_out;
}
-   if (rnp_root != rnp && rnp_root->parent != NULL)
-   raw_spin_unlock_rcu_node(rnp_root);
-   if (!rnp_root->parent)
+   if (rnp != rnp_start && rnp->parent != NULL)
+   raw_spin_unlock_rcu_node(rnp);
+   if (!rnp->parent) {
+   rnp_root = rnp;
break;  /* At root, and perhaps also leaf. */
+   }
}
 
/* If GP already in progress, just leave, otherwise start one. */
@@ -1601,11 +1603,11 @@ static bool rcu_start_this_gp(struct rcu_node *rnp, 
struct rcu_data *rdp,
trace_rcu_grace_period(rsp->name, READ_ONCE(rsp->gp_seq), 
TPS("newreq"));
ret = true;  /* Caller must wake GP kthread. */
 unlock_out:
-   if (rnp != rnp_root)
-   raw_spin_unlock_rcu_node(rnp_root);
+   if (rnp != rnp_start)
+   raw_spin_unlock_rcu_node(rnp);
/* Push furthest requested GP to leaf node and rcu_data structure. */
-   if