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

[Steven Rostedt]

On Wed, 26 Aug 2009, Mathieu Desnoyers wrote:

> * Mathieu Desnoyers (mathieu.desnoyers@xxxxxxxxxx) wrote:
> > * Steven Rostedt (rostedt@xxxxxxxxxxx) wrote:
> > > 
> > > This patch solves the problem that Li originally reported. If something 
> > > registers a trace point belonging to a module, then it ups the ref count 
> > > of the module. This prevents a process from registering a probe to a 
> > > tracepoint belonging to a module and then having the module disappear.
> > > 
> > > Doing the example with perf in Li's original post, now errors on the 
> > > rmmod, with "ERROR: Module trace_events_sample is in use".
> > > 
> > > Mathieu, can I have your acked-by on this?
> > > 
> > 
> > Sorry, it looks buggy.

Well, it is not that buggy. It would only prevent modules from unloading 
if another tracepoint with the same name and parameters had a probe 
registered. More of a too big of a tent deal.

> > 
> > It does not deal with the fact that tracepoints with the same name and
> > arguments can be present in more than one module, or in a combination of
> > kernel core and modules.
> > 
> > The struct tracepoint_entry is specific to a a tracepoint name, used for
> > registration, but is eventually tied to all tracepoint instrumentation
> > instances for this tracepoint name.
> > 

Anyway, this prevents your tracepoints from doing the odd things of 
loading a probe before it exists. Well you can, but then you prevent the 
unload of the module that registered it. Fine, I chucked out that patch.

> 
> Looking at the original post:
> http://lkml.org/lkml/2009/8/24/5
> 
> the problem seems to be caused by the fact that the
> trace_event_profile.c keeps some knowledge of modules in internal data
> structures, but does not get notified of module unloads. Why don't we
> fix that instead ?
> 
> A quick glance at it seems to indicate that it lazily discovers new
> modules when the tracepoints are hit. Using module load/unload notifiers
> would be more appropriate. Or maybe adding a notifier call to
> tracepoint.c, calling notification callbacks for probe modules which
> need to know when the connected tracepoints are changing
> (when they are connected/disconnected) would probably be even more
> appropriate. As a result, it would remove the dynamic verification cost
> implied by lazy data structure lookup and check each time the probe is
> fired.

Peter is correct that he should not need to worry about modules, he 
doesn't build kernels with them ;-)

Here's another patch that moves the module ref count administration to the 
trace events themselves. This should satisfy both you and Peter.

Signed-off-by: Steven Rostedt <rostedt@xxxxxxxxxxx>

diff --git a/include/trace/ftrace.h b/include/trace/ftrace.h
index 1b1f742..3f7c5dc 100644
--- a/include/trace/ftrace.h
+++ b/include/trace/ftrace.h
@@ -390,6 +390,20 @@ static inline int ftrace_get_offsets_##call(				\
  *
  */
 
+#ifdef MODULE
+# define event_trace_up_ref()					\
+	do {							\
+		if (!try_module_get(THIS_MODULE)) {		\
+			atomic_dec(&event_call->profile_count);	\
+			return -1;				\
+		}						\
+	} while (0)
+# define event_trace_down_ref() module_put(THIS_MODULE)
+#else
+# define event_trace_up_ref() do { } while (0)
+# define event_trace_down_ref() do { } while (0)
+#endif
+		
 #undef TRACE_EVENT
 #define TRACE_EVENT(call, proto, args, tstruct, assign, print)		\
 									\
@@ -399,16 +413,20 @@ static int ftrace_profile_enable_##call(struct ftrace_event_call *event_call) \
 {									\
 	int ret = 0;							\
 									\
-	if (!atomic_inc_return(&event_call->profile_count))		\
+	if (!atomic_inc_return(&event_call->profile_count)) {		\
+		event_trace_up_ref();					\
 		ret = register_trace_##call(ftrace_profile_##call);	\
+	}								\
 									\
 	return ret;							\
 }									\
 									\
 static void ftrace_profile_disable_##call(struct ftrace_event_call *event_call)\
 {									\
-	if (atomic_add_negative(-1, &event_call->profile_count))	\
+	if (atomic_add_negative(-1, &event_call->profile_count)) {	\
 		unregister_trace_##call(ftrace_profile_##call);		\
+		event_trace_down_ref();					\
+	}								\
 }
 
 #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/