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, Li Zefan wrote:
> 
> > > 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.
> > > 
> > 
> > In fact I had this patch in my mind, but I thought Peter insist
> > on fixing it in tracepoint. So I'm fine with this. :)
> 
> Tracepoints I consider a more low level API. Using them takes more hand 
> work and the user needs to know what they are doing and thus, must take 
> into account modules.
> 
> This code is automatic, and a much higher level API. The user shoud not 
> need to worry about modules, thus the protection belongs here.
> 
> If we try to make it so a module can not have register itself, then that 
> will just complicate the TRACE_EVENT macros even more. And those are 
> complex enough.
> 
> > 
> > > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > > 
> > > 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;				\
> > 
> > Should return -1 or -errno like -ENOENT?
> 
> Thanks, I was being lazy and really did not know what to have it return. 
> I'll commit it with a -ENOENT.
> 

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.

Thanks,

Mathieu

> -- Steve
> 
> > 
> > > +		}						\
> > > +	} 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)
> > > 
> > > 
> > 

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