Messages in this thread | | | Date | Thu, 01 Apr 2010 15:40:00 +0800 | From | Lai Jiangshan <> | Subject | Re: [PATCH] rcu: only raise softirq when need |
| |
Paul E. McKenney wrote: > On Wed, Mar 31, 2010 at 10:10:55AM +0800, Lai Jiangshan wrote: >> Paul E. McKenney wrote: >>> On Tue, Mar 30, 2010 at 06:11:55PM +0800, Lai Jiangshan wrote: >>>> I found something RCU_SOFTIRQ are called without do any thing. >>>> (use function_graph to find it: >>>> 1) | rcu_process_callbacks() { >>>> 1) | __rcu_process_callbacks() { >>>> 1) 0.634 us | rcu_process_gp_end(); >>>> 1) 0.487 us | check_for_new_grace_period(); >>>> 1) 2.672 us | } >>>> 1) | __rcu_process_callbacks() { >>>> 1) 0.633 us | rcu_process_gp_end(); >>>> 1) 0.491 us | check_for_new_grace_period(); >>>> 1) 2.672 us | } >>>> ) >>>> >>>> This patch make RCU_SOFTIRQ raised when need. >>> So this seems to have two effects: >>> >>> 1. Avoid checking for a quiescent state if RCU doesn't need one >>> from this CPU. >>> >>> 2. Avoid RCU_SOFTIRQ if RCU did need a quiescent state from >>> this CPU, and if rcu_check_callbacks() saw a quiescent state. >> This RCU_SOFTIRQ is not avoided. >> >> + if (rdp->qs_pending && rdp->passed_quiesc) { >> + rdp->n_rp_report_qs++; >> return 1; >> } >> >> Old: raise RCU_SOFTIRQ when rdp->qs_pending is not zero >> New: raise RCU_SOFTIRQ when rdp->qs_pending && rdp->passed_quiesc >> >> So the different effects only happen when this state: >> rdp->qs_pending == 1 && rdp->passed_quiesc == 0, >> But this state will be changed after next rcu_sched_qs() or families. >> So it will not hang up. > > You are quite correct. I clearly need to give myself a day to think > about patches to RCU functionality before replying. On the other hand, > I might have been just as confused after thinking on it, so airing my > confusion immediately might well have been the optimal approach. ;-) > > So this patch looks like it would work, so the next question is how much > it helps and/or hurts. > >>> Except that if rcu_check_callbacks() did see a quiescent state, then we >>> -need- RCU_SOFTIRQ to propagate this up the tree. So I don't see how >>> this patch helps, and unless I am missing something, it can result in >>> grace-period hangs. (This CPU is the last one to pass through a >>> quiescent state, and this call to rcu_check_callbacks() finds one, >>> and we fail to report it up the tree.) >>> >>> Please note that there are other possible causes for empty calls to >>> rcu_process_callbacks(): >>> >>> 1. RCU needs a call to force_quiescent_state(), but some other >>> CPU beats us to it. We raise RCU_SOFTIRQ, but by the time >>> we get there, our work is done. >>> >>> 2. RCU needs to check for CPU stalls, but some other CPU beats >>> us to it. >>> >>> 3. RCU is idle, and this CPU needs another grace period, but >>> some other CPU starts up a new grace period before our >>> softirq gets started. >> These may happen, but I have not seen any empty call after patch applied. > > Before the patch, what fraction of the calls were empty calls? > >>> So I do not believe that this patch is worthwhile even if it does turn >>> out to be safe. >> I accept that this patch is not worthwhile. >> >> Raising empty call is harmless, and it is a chance >> to progress RCU or detect problems. > > If you say "this patch has not been yet proven to be worthwhile" instead > of "this patch is not worthwhile", I will agree with you. Here is a > quick rundown of my thoughts on it, which cannot be considered to be > fully formed: > > 1. It cannot improve real-time scheduling latency, since there > can be RCU_SOFTIRQ actions that really do something. This > patch therefore does not improve the worst-case code path > (though it might -- or might not -- help the average path). > > 2. It seems to increase path length a bit, but I doubt that this > is measurable, so not a valid objection. > > 3. It -might- improve throughput, but this would need to be > proven. This is the motivation for my question about the > fraction of empty calls above. > > Kernels that force all softirqs to execute in process > context (e.g., CONFIG_PREEMPT_RT) are the most likely to > see benefit from this patch, as a pair of context switches > are saved in that case. Of course, CONFIG_PREEMPT_RT is > more about scheduling latency than maximum throughput. > > 4. It does complicate the code a bit, but could be refactored > to reduce the complication. (I haven't thought this through, > but my guess is that the initial check should be factored > across the "if" statement.) > > All that aside, I don't believe the priority of this patch is particularly > high, although it did do a good job of showing that your understanding > of the treercu implementation is increasing. Which is a very good thing! > > Just out of curiosity, what is your test procedure? > > Thanx, Paul > >
1------------------result (CONFIG_HZ=1000 and CONFIG_PREEMPT=y) Before patch applied: 10% empty calls (something 30+ percent) after patch applied: <0.5% empty calls
2------------------test procedure I think you can add kernel trace code to detect it. (I think it's simpler) The next is my fast test:
ensure CONFIG_FUNCTION_GRAPH_TRACER = y
mount -t debugfs xx /debugfs/ echo rcu_process_callbacks > /debugfs/tracing/set_graph_function echo function_graph > /debugfs/tracing/current_tracer
# make all cpu busy, example: complier the kernel with 'make -j XXX' cat /debugfs/tracing/trace_pipe > trace.tmp # CTRL-C after 20 seconds ./parser.py trace.tmp
3----------------------test code and .... (Because compiler may or may not inline a static function, So:) Your 'empty_call_graph' is not deferent from mine very likely, you should change it.
Q: why 'rcu_process_gp_end();' is empty call? A: _raw_spin_trylock() is not inline-ed, we will see a '{' and '_raw_spin_trylock()' in the graph when it is not empty call: 0) | rcu_process_gp_end() { 0) 0.350 us | _raw_spin_trylock(); 0) 0.250 us | _raw_spin_unlock_irqrestore(); 0) 1.326 us | }
Q: Is this test trusted. A: No, but I believe it.
------------------parser.py---------- #!/bin/env python
import sys
empty_call_graph = tuple( ''' rcu_process_callbacks() { __rcu_process_callbacks() { rcu_process_gp_end(); check_for_new_grace_period(); } __rcu_process_callbacks() { rcu_process_gp_end(); check_for_new_grace_period(); } } '''.splitlines())
total=0 empty=0 match=0 empty_block=[]
for line in file(sys.argv[1]): empty_block.append(line) if line.find('force_quiescent_state();') >= 0: continue
if line.find(empty_call_graph[match]) < 0: match = 0 empty_block=[] continue
match = match + 1 if match == 1: total = total + 1 elif match == len(empty_call_graph): empty = empty + 1 for emptyline in empty_block: print emptyline, empty_block=[] match = 0
print('empty call/total call: %d/%d' % (empty, total)) print('fraction: %f%%' % (100.0 * empty / total))
| |