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

[Byungchul Park <byungchul.park@lge.com>]

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