Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload

[Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> 
> On Thu, 27 Aug 2009, Mathieu Desnoyers wrote:
> 
> > * Steven Rostedt (rostedt@goodmis.org) wrote:
> > > 
> > > On Thu, 27 Aug 2009, Mathieu Desnoyers wrote:
> > > 
> > > > Looks good. Just don't forget to eventually add the "synchronize" calls
> > > > between tracepoint unregistration and the removal of their module. There
> > > > is a race condition in the way you do it currently.
> > > 
> > > I'm trying to figure out the race here. What will disappear in the 
> > > tracepoint? Could you give a brief example of the issue.
> > 
> > Sure,
> > 
> > Let's say we have a tracepoint in module instrumented.c, and a probe in
> > module probe.c. The probe is registered by module probe.c init through
> > the tracepoint infrastructure to connect to the tracepoint in
> > instrumented.c. Unregistration is done in probe.c module exit.
> > 
> > As the instrumented code get executed (let's say periodically), it calls
> > the connected probe. Preemption is disabled around the call.
> > 
> > If you unload the probe.c module, the module exit will unregister the
> > probe, but the probe code can still be in use by another CPU. You have
> > to wait for quiescent state with the tracepoint synchronize (which is
> > just a wrapper over synchronize_sched() before you are allowed to
> > complete module unload. Otherwise, you will end up reclaiming module
> > memory that is still used by probe execution.
> > 
> > A test-case for this would be to create a probe with a delay in it, and
> > an instrumented module calling the instrumentation in a loop. On a SMP
> > system, running the instrumented code and probe load/unload in loops
> > should trigger this race.
> 
> Thanks for the explanation. So let me see if I get this correct.
> 
> For this race to occur, the probe (code that hooks to the tracepoint) must 
> be in module that does not contain the tracepoint. We don't even need more 
> than one module, this could occur even with a core tracepoint. If a module 
> registers it, if it unregisters before unloading, the tracepoint may be 
> hit before the unregister and executing while the module is unloading.
> 
> I don't think we need to worry about this with the case of TRACE_EVENT and 
> ftrace.h. The reason is that the trace point and probes are always in the 
> same location. The MACROS set up the probe code with the modules. Thus, to 
> remove the module, you must also remove the tracepoint itself along with 
> the probe. If you can be executing in the probe, then you must have hit 
> the trace point. If you hit the trace point, then you are executing code 
> inside the module you are removing, which is a bug in the module code 
> itself.
> 
> Using the ftrace.h MACROS limits the use of tracepoints and this race 
> does not exist. I feel we are safe not needing to have the 
> tracepoint_synchronize_unregister within the ftrace.h code.
> 

Looks right. If you can guarantee that the probe is only called from
tracepoints located within the same module as the probe, you should be
safe without tracepoint_synchronize_unregister. It's worth adding a
comment in ftrace.h explaining that though.

Mathieu

> -- Steve
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68