Re: [PATCH] tracing/filters: allow event filters to be set onlywhen not tracing

[Paul E. McKenney]

On Sun, Apr 05, 2009 at 02:34:25AM -0500, Tom Zanussi wrote:
> On Sat, 2009-04-04 at 11:49 -0400, Steven Rostedt wrote:
> > On Sat, 4 Apr 2009, Tom Zanussi wrote:
> > > 
> > > Hmm, after reading Paul's replies, it sounds like this approach might be
> > > more trouble than it's worth.  Maybe going back to the idea of
> > > temporarily stopping/starting tracing would be a better idea, but with a
> > > little more heavyweight version of the current 'quick' tracing
> > > start/stop (that would prevent entering the tracing functions (and ththe
> > > filter_check_discard()).
> > 
> > 
> > Actually, I forgot what the general problem we are avoiding here with the
> > RCU locks. Could you explain that again. Just so that I can get a better
> > idea without having to read between the lines of the previous messages in 
> > this thread.
> > 
> 
> Basically the problem is that the tracing functions call
> filter_match_preds(call,...) where call->preds is an array of predicates
> that get checked to determine whether the current event matches or not.
> When an existing filter is deleted (or an old one replaced), the
> call->preds array is freed and set to NULL (which happens only via a
> write to the 'filter' debugfs file).  So without any protection, while
> one cpu is freeing the preds array, the others may still be using it,
> and if so, it will crash the box.  You can easily see the problem with
> e.g. the function tracer:
> 
> # echo function > /debug/tracing/current_tracer
> 
> Function tracing is now live
> 
> # echo 'common_pid == 0' > /debug/tracing/events/ftrace/function/filter
> 
> No problem, no preds are freed the first time
> 
> # echo 0 > /debug/tracing/events/ftrace/function/filter
> 
> Crash.
> 
> My first patch took the safe route and completely disallowed filters
> from being set when any tracing was live i.e. you had to for example
> echo 0 > tracing_enabled or echo 0 > enable for a particular event, etc.
> 
> This wasn't great for usability, though - it would be much nicer to be
> able to remove or set new filters on the fly, while tracing is active,
> which rcu seemed perfect for - the preds wouldn't actually be destroyed
> until all the current users were finished with them.  My second patch
> implemented that and it seemed to nicely fix the problem, but it
> apparently can cause other problems...
> 
> So assuming we can't use rcu for this, it would be nice to have a way to
> 'pause' tracing so the current filter can be removed i.e. some version
> of stop_trace()/start_trace() that make sure nothing is still executing
> or can enter filter_match_preds() while the current call->preds is being
> destroyed.  Seems like it would be straightforward to implement for the
> event tracer, since each event maps to a tracepoint that could be
> temporarily unregistered/reregistered, but maybe not so easy for the
> ftrace tracers...

In principle, it would be possible to rework RCU so that instead of the
whole idle loop being a quiescent state, there is a single quiescent state
at one point in each idle loop.  The reason that I have been avoiding this
is that there are a lot of idle loops out there, and it would be a bit
annoying to (1) find them all and update them and (2) keep track of all of
them to ensure that new ones cannot slip in without the quiescent state.

But it could be done if the need is there.  Simple enough change.
The following patch shows the general approach, assuming that CPUs
are never put to sleep without entering nohz mode.

Thoughts?

							Thanx, Paul