Message-ID: <alpine.DEB.2.00.0904061529370.4317@gandalf.stny.rr.com>
Date:	Mon, 6 Apr 2009 15:30:32 -0400 (EDT)
From:	Steven Rostedt <rostedt@...dmis.org>
To:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
cc:	Tom Zanussi <tzanussi@...il.com>, Ingo Molnar <mingo@...e.hu>,
	linux-kernel <linux-kernel@...r.kernel.org>, fweisbec@...il.com
Subject: Re: [PATCH] tracing/filters: allow event filters to be set only when
 not tracing


On Mon, 6 Apr 2009, Paul E. McKenney wrote:

> On Mon, Apr 06, 2009 at 11:59:30AM -0400, Steven Rostedt wrote:
> > On Sun, 5 Apr 2009, Paul E. McKenney wrote:
> > > > 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...
> > 
> > The proble is that function tracing also traces the rcu calls. Even though 
> > the function trace protects against recursion, by adding rcu locks to the
> > function tracer, we have just doubled the overhead for it. Every function 
> > trace will call rcu_read_lock, then that would be traced too, and the 
> > function tracer would see that it is recursive and return. All this is 
> > added overhead to _every_ function!
> > 
> > I do not understand why my recommendation is not used. All tracers require 
> > preemption to be disabled. By simply removing the pred from the list, do a 
> > synchronize_sched(), then set it to NULL. The update is done by userland, 
> > synchronizing a schedule should not be that noticeable.
> 
> The only caution is that synchronize_sched() ignores preempt-disable
> sequences in the idle loop.  The reason for this is that synchronize_sched()
> maps to synchronize_rcu() for rcuclassic and rcutree.
> 
> So, if you need to make synchronize_sched() pay attention to
> preempt-disable sequences in the idle loop, something similar to the
> patch to RCU that I sent earlier (adding explicit rcu_idle() call to
> each idle loop) would be required.

Ah! OK, now I understand the issue :-)

> 
> > > > 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?
> > 
> > I think using synchronize_sched() should be good enough for what we need.
> 
> Again, as long as either (1) you are OK with synchronize_sched()
> ignoring preempt-disable sequences in the idle loop or (2) we rework RCU
> to add something like an rcu_idle() call in each idle loop.

3) add "notrace" to the idle functions ;-)

But perhaps the rcu_idle might be the best idea.

-- Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists