Messages in this thread | | | Subject | Re: [patch 01/15] Kernel Tracepoints | From | Peter Zijlstra <> | Date | Tue, 15 Jul 2008 16:03:13 +0200 |
| |
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()
?
| |