Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 15 Jul 2008 15:02:12 -0400 | From | Mathieu Desnoyers <> | Subject | Re: [patch 01/15] Kernel Tracepoints |
| |
* Peter Zijlstra (peterz@infradead.org) wrote: > On Tue, 2008-07-15 at 09:25 -0400, Mathieu Desnoyers wrote: > > * Peter Zijlstra (peterz@infradead.org) wrote: > > > On Wed, 2008-07-09 at 10:59 -0400, Mathieu Desnoyers wrote: > > > > > +#define __DO_TRACE(tp, proto, args) \ > > > > + do { \ > > > > + int i; \ > > > > + void **funcs; \ > > > > + preempt_disable(); \ > > > > + funcs = (tp)->funcs; \ > > > > + smp_read_barrier_depends(); \ > > > > + if (funcs) { \ > > > > + for (i = 0; funcs[i]; i++) { \ > > > > > > Also, why is the preempt_disable needed? > > > > > > > Addition and removal of tracepoints is synchronized by RCU using the > > scheduler (and preempt_disable) as guarantees to find a quiescent state > > (this is really RCU "classic"). The update side uses rcu_barrier_sched() > > with call_rcu_sched() and the read/execute side uses > > "preempt_disable()/preempt_enable()". > > > > > +static void tracepoint_entry_free_old(struct tracepoint_entry *entry, void *old) > > > > +{ > > > > + if (!old) > > > > + return; > > > > + entry->oldptr = old; > > > > + entry->rcu_pending = 1; > > > > + /* write rcu_pending before calling the RCU callback */ > > > > + smp_wmb(); > > > > +#ifdef CONFIG_PREEMPT_RCU > > > > + synchronize_sched(); /* Until we have the call_rcu_sched() */ > > > > +#endif > > > > > > Does this have something to do with the preempt_disable above? > > > > > > > Yes, it does. We make sure the previous array containing probes, which > > has been scheduled for deletion by the rcu callback, is indeed freed > > before we proceed to the next update. It therefore limits the rate of > > modification of a single tracepoint to one update per RCU period. The > > objective here is to permit fast batch add/removal of probes on > > _different_ tracepoints. > > > > This use of "synchronize_sched()" can be changed for call_rcu_sched() in > > linux-next, I'll fix this. > > Right, I thought as much, its just that the raw preempt_disable() > without comments leaves one wondering if there is anything else going > on. > > Would it make sense to add: > > rcu_read_sched_lock() > rcu_read_sched_unlock() > > to match: > > call_rcu_sched() > rcu_barrier_sched() > synchronize_sched() > > ? >
Actually I think it's better to call them rcu_read_lock_sched() and rcu_read_unlock_sched() to match the _bh() equivalent already in rcupdate.h.
Mathieu
-- Mathieu Desnoyers OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
| ![\](/images/icornerr.gif) |