Re: [PATCH 3/8] ftrace: Add enable/disable ftrace_ops controlinterface

[Jiri Olsa]

On Wed, Dec 21, 2011 at 11:01:33AM -0500, Steven Rostedt wrote:
> On Wed, 2011-12-21 at 12:48 +0100, Jiri Olsa wrote:

SNIP

> > +/**
> > + * ftrace_function_enable - enable controlled ftrace_ops on given cpu
> > + *
> > + * This function enables tracing on given cpu by decreasing
> > + * the per cpu control variable.
> > + * It must be called with preemption disabled and only on
> > + * ftrace_ops registered with FTRACE_OPS_FL_CONTROL.
> > + */
> > +static inline void ftrace_function_enable(struct ftrace_ops *ops, int cpu)
> > +{
> > +	atomic_t *disabled;
> > +
> > +	if (WARN_ON_ONCE(!(ops->flags & FTRACE_OPS_FL_CONTROL)) ||
> > +			 !preempt_count())
> 
> The WARN_ON_ONCE() should also include the !preempt_count().
> 

ouch, that was initial intention.. need eye doctor ;)

> 
> > +		return;
> > +
> > +	disabled = per_cpu_ptr(ops->disabled, cpu);
> > +	atomic_dec(disabled);
> > +}
> > +

SNIP

> > +
> > +static int control_ops_is_disabled(struct ftrace_ops *ops)
> > +{
> > +	atomic_t *disabled = this_cpu_ptr(ops->disabled);
> 
> Again, the use of "this_cpu_ptr" is wrong. Gah! We should nuke all of
> that crap.

will nuke this one..

> 
> 

> >  static int __register_ftrace_function(struct ftrace_ops *ops)
> >  {
> >  	if (ftrace_disabled)
> > @@ -268,15 +324,19 @@ static int __register_ftrace_function(struct ftrace_ops *ops)
> >  	if (WARN_ON(ops->flags & FTRACE_OPS_FL_ENABLED))
> >  		return -EBUSY;
> >  
> > +	if ((ops->flags & FL_GLOBAL_CONTROL) == FL_GLOBAL_CONTROL)
> 
> No biggy, but I usually find:
> 
> 	if (ops->flags & FL_GLOBAL_CONTROL)
> 
> more readable. With what you have, I looked at that condition three
> times to figure out what was different between what was '&'d with the
> flags and what was being equal too. Usually the ((flags & X) == Y) is
> done to check if a subset of bits are set within a mask of bits.

Well, thats what I need to do here. Bail out if both bits are set,
since we dont support both global and control flags set at the same
time.. I'll add some comment to it.

thanks,
jirka
--
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/