[PATCH] rcu/tree: Add comment to describe GP done condition in fqs loop

[PATCH] rcu/tree: Add comment to describe GP done condition in fqs loop

[Neeraj Upadhyay]

Add a comment to explain why !rcu_preempt_blocked_readers_cgp() condition
is required on root rnp node, for GP completion check in rcu_gp_fqs_loop().

Signed-off-by: Neeraj Upadhyay <quic_neeraju@quicinc.com>
---
 kernel/rcu/tree.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index a93c5f4d7e09..9cd1ba512fdc 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -2083,7 +2083,17 @@ static noinline_for_stack void rcu_gp_fqs_loop(void)
 		rcu_gp_torture_wait();
 		WRITE_ONCE(rcu_state.gp_state, RCU_GP_DOING_FQS);
 		/* Locking provides needed memory barriers. */
-		/* If grace period done, leave loop. */
+		/*
+		 * If grace period done, leave loop. rcu_preempt_blocked_readers_cgp(rnp)
+		 * check is required for the case where we only have single node in the
+		 * rcu_node tree; so, root rcu node is also the leaf node, where readers
+		 * blocking current gp are queued. For multi-node tree, checking ->qsmask
+		 * on the root node is sufficient, as root rcu node's ->qsmask is only
+		 * cleared, when all leaf rcu nodes have propagated their quiescent
+		 * state to their parent node, which happens only after both ->qsmask
+		 * and rcu_preempt_blocked_readers_cgp(rnp_leaf) are cleared for those
+		 * leaf nodes.
+		 */
 		if (!READ_ONCE(rnp->qsmask) &&
 		    !rcu_preempt_blocked_readers_cgp(rnp))
 			break;
-- 
2.17.1

Re: [PATCH] rcu/tree: Add comment to describe GP done condition in fqs loop

[Paul E. McKenney]

On Thu, Jun 09, 2022 at 12:43:40PM +0530, Neeraj Upadhyay wrote:
> Add a comment to explain why !rcu_preempt_blocked_readers_cgp() condition
> is required on root rnp node, for GP completion check in rcu_gp_fqs_loop().
> 
> Signed-off-by: Neeraj Upadhyay <quic_neeraju@quicinc.com>

Thank you, Neeraj!  As usual, I could not resist the urge to wordsmith
as shown below.  Could you please check to see if I messed something up?

							Thanx, Paul

------------------------------------------------------------------------

commit bdf3a744d3ad21336a390bfcc2e41de63f193eaf
Author: Neeraj Upadhyay <quic_neeraju@quicinc.com>
Date:   Thu Jun 9 12:43:40 2022 +0530

    rcu/tree: Add comment to describe GP-done condition in fqs loop
    
    Add a comment to explain why !rcu_preempt_blocked_readers_cgp() condition
    is required on root rnp node, for GP completion check in rcu_gp_fqs_loop().
    
    Signed-off-by: Neeraj Upadhyay <quic_neeraju@quicinc.com>
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index a93c5f4d7e092..9a941e7ee6109 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -2083,7 +2083,15 @@ static noinline_for_stack void rcu_gp_fqs_loop(void)
 		rcu_gp_torture_wait();
 		WRITE_ONCE(rcu_state.gp_state, RCU_GP_DOING_FQS);
 		/* Locking provides needed memory barriers. */
-		/* If grace period done, leave loop. */
+		/*
+		 * Exit the loop if the root rcu_node structure indicates that the grace period
+		 * has ended, leave the loop.  The rcu_preempt_blocked_readers_cgp(rnp) check
+		 * is required only for single-node rcu_node trees because readers blocking
+		 * the current grace period are queued only on leaf rcu_node structures.
+		 * For multi-node trees, checking the root node's ->qsmask suffices, because a
+		 * given root node's ->qsmask bit is cleared only when all CPUs and tasks from
+		 * the corresponding leaf node have passed through their quiescent state.
+		 */
 		if (!READ_ONCE(rnp->qsmask) &&
 		    !rcu_preempt_blocked_readers_cgp(rnp))
 			break;

Re: [PATCH] rcu/tree: Add comment to describe GP done condition in fqs loop

[Neeraj Upadhyay]

On 6/10/2022 1:53 AM, Paul E. McKenney wrote:
> On Thu, Jun 09, 2022 at 12:43:40PM +0530, Neeraj Upadhyay wrote:
>> Add a comment to explain why !rcu_preempt_blocked_readers_cgp() condition
>> is required on root rnp node, for GP completion check in rcu_gp_fqs_loop().
>>
>> Signed-off-by: Neeraj Upadhyay <quic_neeraju@quicinc.com>
> 
> Thank you, Neeraj!  As usual, I could not resist the urge to wordsmith
> as shown below.  Could you please check to see if I messed something up?

Thanks!
> 
> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> commit bdf3a744d3ad21336a390bfcc2e41de63f193eaf
> Author: Neeraj Upadhyay <quic_neeraju@quicinc.com>
> Date:   Thu Jun 9 12:43:40 2022 +0530
> 
>      rcu/tree: Add comment to describe GP-done condition in fqs loop
>      
>      Add a comment to explain why !rcu_preempt_blocked_readers_cgp() condition
>      is required on root rnp node, for GP completion check in rcu_gp_fqs_loop().
>      
>      Signed-off-by: Neeraj Upadhyay <quic_neeraju@quicinc.com>
>      Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index a93c5f4d7e092..9a941e7ee6109 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -2083,7 +2083,15 @@ static noinline_for_stack void rcu_gp_fqs_loop(void)
>   		rcu_gp_torture_wait();
>   		WRITE_ONCE(rcu_state.gp_state, RCU_GP_DOING_FQS);
>   		/* Locking provides needed memory barriers. */
> -		/* If grace period done, leave loop. */
> +		/*
> +		 * Exit the loop if the root rcu_node structure indicates that the grace period
> +		 * has ended, leave the loop.  The rcu_preempt_blocked_readers_cgp(rnp) check

We can remove ", leave the loop" ?

> +		 * is required only for single-node rcu_node trees because readers blocking
> +		 * the current grace period are queued only on leaf rcu_node structures.
> +		 * For multi-node trees, checking the root node's ->qsmask suffices, because a
> +		 * given root node's ->qsmask bit is cleared only when all CPUs and tasks from

Do we need to say "a given root node's" , we have only single RCU node 
in the system, so we can just say, "because root node's ->qsmask bit is 
cleared..." ?

> +		 * the corresponding leaf node have passed through their quiescent state.

Change "the corresponding leaf node" to "their corresponding leaf nodes" 
or "the corresponding leaf nodes"?


Thanks
Neeraj

> +		 */
>   		if (!READ_ONCE(rnp->qsmask) &&
>   		    !rcu_preempt_blocked_readers_cgp(rnp))
>   			break;

Re: [PATCH] rcu/tree: Add comment to describe GP done condition in fqs loop

[Paul E. McKenney]

On Fri, Jun 10, 2022 at 08:45:42AM +0530, Neeraj Upadhyay wrote:
> 
> 
> On 6/10/2022 1:53 AM, Paul E. McKenney wrote:
> > On Thu, Jun 09, 2022 at 12:43:40PM +0530, Neeraj Upadhyay wrote:
> > > Add a comment to explain why !rcu_preempt_blocked_readers_cgp() condition
> > > is required on root rnp node, for GP completion check in rcu_gp_fqs_loop().
> > > 
> > > Signed-off-by: Neeraj Upadhyay <quic_neeraju@quicinc.com>
> > 
> > Thank you, Neeraj!  As usual, I could not resist the urge to wordsmith
> > as shown below.  Could you please check to see if I messed something up?
> 
> Thanks!
> > 
> > 							Thanx, Paul
> > 
> > ------------------------------------------------------------------------
> > 
> > commit bdf3a744d3ad21336a390bfcc2e41de63f193eaf
> > Author: Neeraj Upadhyay <quic_neeraju@quicinc.com>
> > Date:   Thu Jun 9 12:43:40 2022 +0530
> > 
> >      rcu/tree: Add comment to describe GP-done condition in fqs loop
> >      Add a comment to explain why !rcu_preempt_blocked_readers_cgp() condition
> >      is required on root rnp node, for GP completion check in rcu_gp_fqs_loop().
> >      Signed-off-by: Neeraj Upadhyay <quic_neeraju@quicinc.com>
> >      Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index a93c5f4d7e092..9a941e7ee6109 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -2083,7 +2083,15 @@ static noinline_for_stack void rcu_gp_fqs_loop(void)
> >   		rcu_gp_torture_wait();
> >   		WRITE_ONCE(rcu_state.gp_state, RCU_GP_DOING_FQS);
> >   		/* Locking provides needed memory barriers. */
> > -		/* If grace period done, leave loop. */
> > +		/*
> > +		 * Exit the loop if the root rcu_node structure indicates that the grace period
> > +		 * has ended, leave the loop.  The rcu_preempt_blocked_readers_cgp(rnp) check
> 
> We can remove ", leave the loop" ?
> 
> > +		 * is required only for single-node rcu_node trees because readers blocking
> > +		 * the current grace period are queued only on leaf rcu_node structures.
> > +		 * For multi-node trees, checking the root node's ->qsmask suffices, because a
> > +		 * given root node's ->qsmask bit is cleared only when all CPUs and tasks from
> 
> Do we need to say "a given root node's" , we have only single RCU node in
> the system, so we can just say, "because root node's ->qsmask bit is
> cleared..." ?
> 
> > +		 * the corresponding leaf node have passed through their quiescent state.
> 
> Change "the corresponding leaf node" to "their corresponding leaf nodes" or
> "the corresponding leaf nodes"?

And the winner is "the corresponding leaf nodes"!  Good catch, and
thank you!

							Thanx, Paul

> Thanks
> Neeraj
> 
> > +		 */
> >   		if (!READ_ONCE(rnp->qsmask) &&
> >   		    !rcu_preempt_blocked_readers_cgp(rnp))
> >   			break;

Re: [PATCH] rcu/tree: Add comment to describe GP done condition in fqs loop

[Joel Fernandes]

On Thu, Jun 09, 2022 at 12:43:40PM +0530, Neeraj Upadhyay wrote:
> Add a comment to explain why !rcu_preempt_blocked_readers_cgp() condition
> is required on root rnp node, for GP completion check in rcu_gp_fqs_loop().
> 
> Signed-off-by: Neeraj Upadhyay <quic_neeraju@quicinc.com>
> ---
>  kernel/rcu/tree.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index a93c5f4d7e09..9cd1ba512fdc 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -2083,7 +2083,17 @@ static noinline_for_stack void rcu_gp_fqs_loop(void)
>  		rcu_gp_torture_wait();
>  		WRITE_ONCE(rcu_state.gp_state, RCU_GP_DOING_FQS);
>  		/* Locking provides needed memory barriers. */
> -		/* If grace period done, leave loop. */
> +		/*
> +		 * If grace period done, leave loop. rcu_preempt_blocked_readers_cgp(rnp)
> +		 * check is required for the case where we only have single node in the
> +		 * rcu_node tree; so, root rcu node is also the leaf node, where readers
> +		 * blocking current gp are queued. For multi-node tree, checking ->qsmask
> +		 * on the root node is sufficient, as root rcu node's ->qsmask is only
> +		 * cleared, when all leaf rcu nodes have propagated their quiescent
> +		 * state to their parent node, which happens only after both ->qsmask
> +		 * and rcu_preempt_blocked_readers_cgp(rnp_leaf) are cleared for those
> +		 * leaf nodes.
> +		 */
>  		if (!READ_ONCE(rnp->qsmask) &&
>  		    !rcu_preempt_blocked_readers_cgp(rnp))
>  			break;

Paul's wording changes are OK with me.

For the patch:
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

thanks,

 - Joel

Re: [PATCH] rcu/tree: Add comment to describe GP done condition in fqs loop

[Paul E. McKenney]

On Fri, Jun 10, 2022 at 04:37:39PM +0000, Joel Fernandes wrote:
> On Thu, Jun 09, 2022 at 12:43:40PM +0530, Neeraj Upadhyay wrote:
> > Add a comment to explain why !rcu_preempt_blocked_readers_cgp() condition
> > is required on root rnp node, for GP completion check in rcu_gp_fqs_loop().
> > 
> > Signed-off-by: Neeraj Upadhyay <quic_neeraju@quicinc.com>
> > ---
> >  kernel/rcu/tree.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index a93c5f4d7e09..9cd1ba512fdc 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -2083,7 +2083,17 @@ static noinline_for_stack void rcu_gp_fqs_loop(void)
> >  		rcu_gp_torture_wait();
> >  		WRITE_ONCE(rcu_state.gp_state, RCU_GP_DOING_FQS);
> >  		/* Locking provides needed memory barriers. */
> > -		/* If grace period done, leave loop. */
> > +		/*
> > +		 * If grace period done, leave loop. rcu_preempt_blocked_readers_cgp(rnp)
> > +		 * check is required for the case where we only have single node in the
> > +		 * rcu_node tree; so, root rcu node is also the leaf node, where readers
> > +		 * blocking current gp are queued. For multi-node tree, checking ->qsmask
> > +		 * on the root node is sufficient, as root rcu node's ->qsmask is only
> > +		 * cleared, when all leaf rcu nodes have propagated their quiescent
> > +		 * state to their parent node, which happens only after both ->qsmask
> > +		 * and rcu_preempt_blocked_readers_cgp(rnp_leaf) are cleared for those
> > +		 * leaf nodes.
> > +		 */
> >  		if (!READ_ONCE(rnp->qsmask) &&
> >  		    !rcu_preempt_blocked_readers_cgp(rnp))
> >  			break;
> 
> Paul's wording changes are OK with me.
> 
> For the patch:
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Applied, thank you!

							Thanx, Paul