Re: [patch 05/61] lock validator: introduce WARN_ON_ONCE(cond)

[Steven Rostedt <rostedt@goodmis.org>]

On Sun, 2006-06-04 at 11:18 +0200, Arjan van de Ven wrote:
> On Sat, 2006-06-03 at 14:09 -0400, Steven Rostedt wrote:
> 
> > 
> > As you can see, because the whole thing is unlikely, the first condition
> > is expected to fail.  With the current WARN_ON logic, that means that
> > the __warn_once is expected to fail, but that's not the case.  So on a
> > normal system where the WARN_ON_ONCE condition would never happen, you
> > are always branching. 
> 
> which is no cost since it's consistent for the branch predictor
> 
> >   So simply reversing the order to test the
> > condition before testing the __warn_once variable should improve cache
> > performance.
> > -	if (unlikely(__warn_once && (condition))) {	\
> > +	if (unlikely((condition) && __warn_once)) {	\
> >  		__warn_once = 0;			\
> 
> I disagree with this; "condition" can be a relatively complex thing,
> such as a function call. doing the cheaper (and consistent!) test first
> will be better. 

Wrong!  It's not better, because it is pretty much ALWAYS TRUE!  So even
if you have branch prediction you will call the condition regardless!

> __warn_once will be branch predicted correctly ALWAYS,
> except the exact ONE time you turn hit the backtrace. So it's really
> really cheap to test, and if the WARN_ON_ONCE is triggering a lot after
> the first time, you now would have a flapping first condition (which
> means lots of branch mispredicts) while the original code has a perfect
> one-check-predicted-exit scenario.

Who cares?  If the WARN_ON_ONCE _is_ triggered a bunch of times, that
means the kernel is broken.  The WARN_ON is about checking for validity,
and the condition should never trigger on a proper setup.  The ONCE part
is to keep the users logs from getting full and killing performance with
printk. And even so.  If you have 100 instances of WARN_ON_ONCE in the
kernel, only one at time would probably trigger, so you save on the
other 99.  Your idea is to optimize the broken kernel while punishing
the working one.

The analysis wasn't only about the code, but also about the use of
WARN_ON_ONCE.  The condition should _not_ be too complex and slow since
the WARN_ON_ONCE is just a check, and not something that should slow the
system down too much.

One other thing that wasn't mentioned.  The __warn_once variable is
global and not setup as a read_mostly (which maybe it should).  Because
now it can be placed in the same cache line as some global variable that
is modified a lot, so every time you test __warn_once you need to do a
cache coherency  with other CPUS, thus bringing down the performance
further.

-- Steve

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/