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

[Tom Zanussi <tzanussi@gmail.com>]

On Fri, 2009-04-03 at 10:12 -0400, Steven Rostedt wrote:
> On Fri, 3 Apr 2009, Ingo Molnar wrote:
> 
> > 
> > * Tom Zanussi <tzanussi@gmail.com> wrote:
> > 
> > > On Wed, 2009-04-01 at 14:24 +0200, Ingo Molnar wrote:
> > > > * Tom Zanussi <tzanussi@gmail.com> wrote:
> > > > 
> > > > > This patch adds code allowing the event filter to be set only if 
> > > > > there's no active tracing going on.
> > > > 
> > > > > --- a/kernel/trace/trace_events.c
> > > > > +++ b/kernel/trace/trace_events.c
> > > > > @@ -498,6 +498,9 @@ event_filter_write(struct file *filp, const char __user *ubuf, size_t cnt,
> > > > >  	struct filter_pred *pred;
> > > > >  	int err;
> > > > >  
> > > > > +	if (tracing_is_enabled() && (!tracer_is_nop() || call->enabled))
> > > > > +		return -EBUSY;
> > > > 
> > > > hm, but it would be the normal use-case to set filters on the fly. 
> > > > To experiment around with them and shape them until the output is 
> > > > just right. Having to turn the tracer on/off during that seems quite 
> > > > counterproductive to that use-case.
> > > > 
> > > 
> > > I didn't see anything that could be used to temporarily disable 
> > > tracing (tracing_stop() and tracing_start() are 'quick' versions 
> > > that mostly just disable recording), so did it this way to avoid 
> > > adding any overhead to the filter-checking code.
> > > 
> > > But anyway, I'll post a new patch shortly that uses rcu and does 
> > > allow the filters to be set on the fly.
> > 
> > that's a very intelligent way to do it!
> > 
> > There's a theoretical problem though: what if we put a filtered 
> > tracepoint into the RCU code? Especially if that tracepoint is in 
> > the common function-tracer callback affecting all kernel functions. 
> > I've Cc:-ed Paul. I think the quiescent state logic should handle 
> > this just fine, but i'm not 100% sure.
> 
> I commented about this too. I feel the safest way is to simply use 
> preempt_disable, and instead of synchronize_rcu, we can use 
> synchronize_sched, which should have the same effect.
> 

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()).

I was thinking it would be something like:

stop_tracing();
current_tracer->stop(); /* unregister tracepoints, etc */

remove filter

current_tracer->start(); /* reregister tracepoints, etc */
start_tracing();

The struct tracer comments suggest that the stop()/start()
ops are meant for pausing, I'd guess for things like this, but some of
the tracers don't implement them.

For the events in the event tracer, it would be something like:

stop_tracing();
call->unregfunc(); /* unregister tracepoint */

remove filter

call->regfunc(); /* reregister tracepoint */
start_tracing();

If that makes sense, I can try it that way instead.

Tom