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 <[email protected]> > 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 <[email protected]> > [ paulmck: Reworked commit log. ] > Signed-off-by: Paul E. McKenney <[email protected]> > > 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();
