[PATCH] rcu: Avoid unnecessary contention of rcu node lock

[PATCH] rcu: Avoid unnecessary contention of rcu node lock

[Byungchul Park]

It's unnecessary to try to print stacks of blocked tasks in the case
that ndetected == 0. Furthermore, calling rcu_print_detail_task_stall()
causes to acquire rnp locks as many times as the number of leaf nodes
plus one for root node. It's unnecessary at all in the case.

Signed-off-by: Byungchul Park <byungchul.park@lge.com>
---
 kernel/rcu/tree.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 287f468..ab2f743 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -1374,6 +1374,9 @@ static void print_other_cpu_stall(struct rcu_state *rsp, unsigned long gpnum)
 	       (long)rsp->gpnum, (long)rsp->completed, totqlen);
 	if (ndetected) {
 		rcu_dump_cpu_stacks(rsp);
+
+		/* Complain about tasks blocking the grace period. */
+		rcu_print_detail_task_stall(rsp);
 	} else {
 		if (READ_ONCE(rsp->gpnum) != gpnum ||
 		    READ_ONCE(rsp->completed) == gpnum) {
@@ -1390,9 +1393,6 @@ static void print_other_cpu_stall(struct rcu_state *rsp, unsigned long gpnum)
 		}
 	}
 
-	/* Complain about tasks blocking the grace period. */
-	rcu_print_detail_task_stall(rsp);
-
 	rcu_check_gp_kthread_starvation(rsp);
 
 	panic_on_rcu_stall();
-- 
1.9.1

Re: [PATCH] rcu: Avoid unnecessary contention of rcu node lock

[Byungchul Park]

On Wed, Nov 09, 2016 at 05:57:13PM +0900, Byungchul Park wrote:
> It's unnecessary to try to print stacks of blocked tasks in the case
> that ndetected == 0. Furthermore, calling rcu_print_detail_task_stall()
> causes to acquire rnp locks as many times as the number of leaf nodes
> plus one for root node. It's unnecessary at all in the case.

Hello,

I have two questions. Could you answer them?

1. What do you think about this patch?
2. Is there a tree where patches about rcu are pulled into, before
   being pulled into mainline tree?
   For example, tip tree in case of scheduler patches.

It would be appriciated if you answer them.

Thank you in advance,
Byungchul

> 
> Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> ---
>  kernel/rcu/tree.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 287f468..ab2f743 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -1374,6 +1374,9 @@ static void print_other_cpu_stall(struct rcu_state *rsp, unsigned long gpnum)
>  	       (long)rsp->gpnum, (long)rsp->completed, totqlen);
>  	if (ndetected) {
>  		rcu_dump_cpu_stacks(rsp);
> +
> +		/* Complain about tasks blocking the grace period. */
> +		rcu_print_detail_task_stall(rsp);
>  	} else {
>  		if (READ_ONCE(rsp->gpnum) != gpnum ||
>  		    READ_ONCE(rsp->completed) == gpnum) {
> @@ -1390,9 +1393,6 @@ static void print_other_cpu_stall(struct rcu_state *rsp, unsigned long gpnum)
>  		}
>  	}
>  
> -	/* Complain about tasks blocking the grace period. */
> -	rcu_print_detail_task_stall(rsp);
> -
>  	rcu_check_gp_kthread_starvation(rsp);
>  
>  	panic_on_rcu_stall();
> -- 
> 1.9.1

Re: [PATCH] rcu: Avoid unnecessary contention of rcu node lock

[Steven Rostedt]

On Wed, 16 Nov 2016 13:49:31 +0900
Byungchul Park <byungchul.park@lge.com> wrote:

> On Wed, Nov 09, 2016 at 05:57:13PM +0900, Byungchul Park wrote:
> > It's unnecessary to try to print stacks of blocked tasks in the case
> > that ndetected == 0. Furthermore, calling rcu_print_detail_task_stall()
> > causes to acquire rnp locks as many times as the number of leaf nodes
> > plus one for root node. It's unnecessary at all in the case.  
> 
> Hello,
> 
> I have two questions. Could you answer them?
> 
> 1. What do you think about this patch?
> 2. Is there a tree where patches about rcu are pulled into, before
>    being pulled into mainline tree?
>    For example, tip tree in case of scheduler patches.

I believe Paul sends to the tip tree too.

> 
> It would be appriciated if you answer them.
> 
> Thank you in advance,
> Byungchul
> 
> > 
> > Signed-off-by: Byungchul Park <byungchul.park@lge.com>
> > ---
> >  kernel/rcu/tree.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 287f468..ab2f743 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -1374,6 +1374,9 @@ static void print_other_cpu_stall(struct rcu_state *rsp, unsigned long gpnum)
> >  	       (long)rsp->gpnum, (long)rsp->completed, totqlen);
> >  	if (ndetected) {
> >  		rcu_dump_cpu_stacks(rsp);
> > +
> > +		/* Complain about tasks blocking the grace period. */
> > +		rcu_print_detail_task_stall(rsp);
> >  	} else {
> >  		if (READ_ONCE(rsp->gpnum) != gpnum ||
> >  		    READ_ONCE(rsp->completed) == gpnum) {
> > @@ -1390,9 +1393,6 @@ static void print_other_cpu_stall(struct rcu_state *rsp, unsigned long gpnum)
> >  		}

What about showing it in else statement of the above if as well?

	sched_show_task(current);
	rcu_print_detail_task_stall(rsp);

-- Steve

> >  	}
> >  
> > -	/* Complain about tasks blocking the grace period. */
> > -	rcu_print_detail_task_stall(rsp);
> > -
> >  	rcu_check_gp_kthread_starvation(rsp);
> >  
> >  	panic_on_rcu_stall();
> > -- 
> > 1.9.1  

Re: [PATCH] rcu: Avoid unnecessary contention of rcu node lock

[Paul E. McKenney]

On Wed, Nov 16, 2016 at 01:49:31PM +0900, Byungchul Park wrote:
> On Wed, Nov 09, 2016 at 05:57:13PM +0900, Byungchul Park wrote:
> > It's unnecessary to try to print stacks of blocked tasks in the case
> > that ndetected == 0. Furthermore, calling rcu_print_detail_task_stall()
> > causes to acquire rnp locks as many times as the number of leaf nodes
> > plus one for root node. It's unnecessary at all in the case.

Please accept my apologies for the delay -- the last couple of weeks
were quite busy, and I needed to give this the attention that it
deserves.

> Hello,
> 
> I have two questions. Could you answer them?
> 
> 1. What do you think about this patch?

This patch would be a performance optimization if ndetected were often
zero at the end of the loop in print_other_cpu_stall().  However, for
this to happen, the stall would have to be almost exactly 21 seconds
in duration, which seems unlikely and which also proves to be unlikely
in actual practice.

If there was any performance or readability downside whatsoever for
this patch, I would of course need to reject it.  However, it appears
to be free of any performance degradation and could be said to slightly
increase readability.

I took the patch and reworked the commit log as shown below.

That said, it is quite rare for me to accept a patch with such a low
probability of reducing overhead.

> 2. Is there a tree where patches about rcu are pulled into, before
>    being pulled into mainline tree?
>    For example, tip tree in case of scheduler patches.

git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git

This is pulled into -tip, as Steven said.

							Thanx, Paul

> It would be appriciated if you answer them.
> 
> Thank you in advance,
> Byungchul

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

commit 9183b76a762e0e73fd362cf2563f6492ae7fc193
Author: Byungchul Park <byungchul.park@lge.com>
Date:   Wed Nov 9 17:57:13 2016 +0900

    rcu: Only dump stalled-tasks stacks if there was a real stall
    
    The print_other_cpu_stall() function currently unconditionally invokes
    rcu_print_detail_task_stall().  This is OK because if there was a stall
    sufficient to cause print_other_cpu_stall() to be invoked, that stall
    is very likely to persist through the entire print_other_cpu_stall()
    execution.  However, if the stall did not persist, the variable ndetected
    will be zero, and that variable is already tested in an "if" statement.
    Therefore, this commit moves the call to rcu_print_detail_task_stall()
    under that pre-existing "if" to improve readability, with a very rare
    reduction in overhead.
    
    Signed-off-by: Byungchul Park <byungchul.park@lge.com>
    [ paulmck: Reworked commit log. ]
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 2c399db6df6e..b11d00ad1213 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -1504,6 +1504,9 @@ static void print_other_cpu_stall(struct rcu_state *rsp, unsigned long gpnum)
 	       (long)rsp->gpnum, (long)rsp->completed, totqlen);
 	if (ndetected) {
 		rcu_dump_cpu_stacks(rsp);
+
+		/* Complain about tasks blocking the grace period. */
+		rcu_print_detail_task_stall(rsp);
 	} else {
 		if (READ_ONCE(rsp->gpnum) != gpnum ||
 		    READ_ONCE(rsp->completed) == gpnum) {
@@ -1520,9 +1523,6 @@ static void print_other_cpu_stall(struct rcu_state *rsp, unsigned long gpnum)
 		}
 	}
 
-	/* Complain about tasks blocking the grace period. */
-	rcu_print_detail_task_stall(rsp);
-
 	rcu_check_gp_kthread_starvation(rsp);
 
 	panic_on_rcu_stall();

Re: [PATCH] rcu: Avoid unnecessary contention of rcu node lock

[Byungchul Park]

On Wed, Nov 16, 2016 at 11:29:35AM -0800, Paul E. McKenney wrote:
> On Wed, Nov 16, 2016 at 01:49:31PM +0900, Byungchul Park wrote:
> > On Wed, Nov 09, 2016 at 05:57:13PM +0900, Byungchul Park wrote:
> > > It's unnecessary to try to print stacks of blocked tasks in the case
> > > that ndetected == 0. Furthermore, calling rcu_print_detail_task_stall()
> > > causes to acquire rnp locks as many times as the number of leaf nodes
> > > plus one for root node. It's unnecessary at all in the case.
> 
> Please accept my apologies for the delay -- the last couple of weeks
> were quite busy, and I needed to give this the attention that it
> deserves.
> 
> > Hello,
> > 
> > I have two questions. Could you answer them?
> > 
> > 1. What do you think about this patch?
> 
> This patch would be a performance optimization if ndetected were often
> zero at the end of the loop in print_other_cpu_stall().  However, for
> this to happen, the stall would have to be almost exactly 21 seconds
> in duration, which seems unlikely and which also proves to be unlikely
> in actual practice.

Hello paul,

Yes, it's true with current code.

> 
> If there was any performance or readability downside whatsoever for
> this patch, I would of course need to reject it.  However, it appears
> to be free of any performance degradation and could be said to slightly
> increase readability.
> 
> I took the patch and reworked the commit log as shown below.
> 
> That said, it is quite rare for me to accept a patch with such a low
> probability of reducing overhead.

Thank you very much ;)

Thanks,
Byungchul

> 
> > 2. Is there a tree where patches about rcu are pulled into, before
> >    being pulled into mainline tree?
> >    For example, tip tree in case of scheduler patches.
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git
> 
> This is pulled into -tip, as Steven said.
> 
> 							Thanx, Paul
> 
> > It would be appriciated if you answer them.
> > 
> > Thank you in advance,
> > Byungchul
> 
> ------------------------------------------------------------------------
> 
> commit 9183b76a762e0e73fd362cf2563f6492ae7fc193
> Author: Byungchul Park <byungchul.park@lge.com>
> Date:   Wed Nov 9 17:57:13 2016 +0900
> 
>     rcu: Only dump stalled-tasks stacks if there was a real stall
>     
>     The print_other_cpu_stall() function currently unconditionally invokes
>     rcu_print_detail_task_stall().  This is OK because if there was a stall
>     sufficient to cause print_other_cpu_stall() to be invoked, that stall
>     is very likely to persist through the entire print_other_cpu_stall()
>     execution.  However, if the stall did not persist, the variable ndetected
>     will be zero, and that variable is already tested in an "if" statement.
>     Therefore, this commit moves the call to rcu_print_detail_task_stall()
>     under that pre-existing "if" to improve readability, with a very rare
>     reduction in overhead.
>     
>     Signed-off-by: Byungchul Park <byungchul.park@lge.com>
>     [ paulmck: Reworked commit log. ]
>     Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 2c399db6df6e..b11d00ad1213 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -1504,6 +1504,9 @@ static void print_other_cpu_stall(struct rcu_state *rsp, unsigned long gpnum)
>  	       (long)rsp->gpnum, (long)rsp->completed, totqlen);
>  	if (ndetected) {
>  		rcu_dump_cpu_stacks(rsp);
> +
> +		/* Complain about tasks blocking the grace period. */
> +		rcu_print_detail_task_stall(rsp);
>  	} else {
>  		if (READ_ONCE(rsp->gpnum) != gpnum ||
>  		    READ_ONCE(rsp->completed) == gpnum) {
> @@ -1520,9 +1523,6 @@ static void print_other_cpu_stall(struct rcu_state *rsp, unsigned long gpnum)
>  		}
>  	}
>  
> -	/* Complain about tasks blocking the grace period. */
> -	rcu_print_detail_task_stall(rsp);
> -
>  	rcu_check_gp_kthread_starvation(rsp);
>  
>  	panic_on_rcu_stall();