Messages in this thread Patch in this message | | | Date | Wed, 16 Nov 2016 11:29:35 -0800 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH] rcu: Avoid unnecessary contention of rcu node lock |
| |
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();
| |