Re: [PATCH][RFC] trace: profile likely and unlikely annotations

[Steven Rostedt]

On Tue, 28 Oct 2008, Theodore Tso wrote:

> On Tue, Oct 28, 2008 at 12:12:48AM -0400, Steven Rostedt wrote:
> > 
> > Andrew Morton recently suggested having an in-kernel way to profile
> > likely and unlikely macros. This patch achieves that goal.
> 
> Maybe I'm confused, but when I read through the patch, it looks like
> that 'hit' is incremented whenever the condition is true, and 'missed'
> is incremented whenever the condition is false, correct?

Correct.

> 
> Is that what you intended?  So for profile_unlikely, "missed" is good,
> and "hit" is bad, and for profile_likely, "hit" is good, and "missed"
> is bad.  That seems horribly confusing.

Correct. Yeah, I figured I'd get complaints about this (hence the RFC).
If you look at my awk example, you will also notice that I switched the
$1 and $2 around when reading the other file.

This can be confusing either way. I did this to reuse the code for both 
outputs.

> 
> If that wasn't what you intended, the meaning of "hit" and "missed"
> seems to be highly confusing, either way.  Can we perhaps use some
> other terminology?  Simply using "True" and "False" would be better,
> since there's no possible confusion what the labels mean.   

So renaming 'hit' and 'miss' to 'True' and 'False' would be good enough?
That is, it will still mean that a 'True' is bad for unlikely but good for 
a likely?

> 
> > +#define unlikely(x) ({							\
> > +			int ______r;					\
> > +			static struct ftrace_likely_data ______f	\
> > +				__attribute__((__aligned__(4)))		\
> > +				__attribute__((section("_ftrace_unlikely"))); \
> > +			if (unlikely_notrace(!______f.ip))		\
> > +				______f.ip = __THIS_IP__;		\
> > +			______r = unlikely_notrace(x);			\
> > +			ftrace_likely_update(&______f, ______r);	\
> > +			______r;					\
> > +		})
> 
> Note that unlikely(x) calls ftrace_likely_update(), which does this:
> 
> > +void ftrace_likely_update(struct ftrace_likely_data *f, int val)
> > +{
> > +	/* FIXME: Make this atomic! */
> > +	if (val)
> > +		f->hit++;
> > +	else
> > +		f->missed++;
> > +}
> > +EXPORT_SYMBOL(ftrace_likely_update);
> 
> 
> So that seems to mean that if unlikely(x) is false, then _____r is 0,
> which means we increment f->missed.   Or am I missing something?
> 
> I would have thought that if unlikely(x) is false, that's *good*,
> since it means the unlikely label was correct.  And normally, when
> people think about cache hits vs cache misses, hits are good and
> misses are bad.  Which is why I think the terminology is highly
> confusing...

OK, I'm fine with changing the terminology. v2 will do:

  s/hit/True/
  s/missed/False/

Fine with you?

-- Steve

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