Messages in this thread | | | Date | Thu, 27 Aug 2009 11:59:39 -0400 | From | Mathieu Desnoyers <> | Subject | Re: [PATCH] tracing/profile: Fix profile_disable vs module_unload |
| |
* 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
| |