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

[Steven Rostedt]

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@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;				\
> 
> 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.

-- 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)
> > 
> > 
> 
--
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/