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@xxxxxxx>
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@xxxxxxx>
    [ paulmck: Reworked commit log. ]
    Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>

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();