[Top] [All Lists]

likely and unlikely was: Re: [PATCH] split xfs_ioc_xattr

To: Andi Kleen <andi@xxxxxxxxxxxxxx>
Subject: likely and unlikely was: Re: [PATCH] split xfs_ioc_xattr
From: Timothy Shimmin <tes@xxxxxxx>
Date: Fri, 18 Apr 2008 17:06:50 +1000
Cc: David Chinner <dgc@xxxxxxx>, Niv Sardi <xaiki@xxxxxxxxxxxxx>, Christoph Hellwig <hch@xxxxxx>, xfs@xxxxxxxxxxx
In-reply-to: <87ve2i5kbs.fsf@xxxxxxxxxxxxxxxxx>
References: <20080319204014.GA23644@xxxxxx> <ncciqylf7q0.fsf@xxxxxxx> <20080414032940.GA10579@xxxxxx> <ncclk3ejwam.fsf@xxxxxxx> <20080416063712.GN108924158@xxxxxxx> <4805A589.7080906@xxxxxxx> <87ve2i5kbs.fsf@xxxxxxxxxxxxxxxxx>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Thunderbird (Macintosh/20080213)

Thanks for the explanation, Andi.
So I guess the upshot is, that it can make a difference but
in many cases (where the perf difference isn't an issue)
it is probably not worth the ugliness.
And in performance cases, it would be best to test the hypothesis
with the unlikely profiler patch
=> it will be _unlikely_ we will bother ;-)
So I don't think I'll be bothering with them then unless
an issue comes up :)


Andi Kleen wrote:
Timothy Shimmin <tes@xxxxxxx> writes:
I'm still wondering if likely() and unlikely() should ever be used or not?

It's more than just branch predictors. unlikely also moves unlikely
code out of line and keeps it out of the icache (the obvious
drawback is that it makes the asm code much harder to read
during debugging though -- that is why it used to be turned off on x86-64)
Then CPUs have two types of branch predictors: dynamic ones with
history and static ones. Dynamic branch predictors tend to only work
when the code has been recently executed several times and is still
cached in their history buffers

Now the nature of the kernel is that it is a library serving much more
code running in user space. This leads to often the user space
clearing out the history buffers and caches so kernel code has to often
deal with running cache cold and also branch predictor cold.

Then there are the static branch predictors in the CPU and unlikely()
actually rearranges code to make them predict correctly.

Personally I would say the cache effects (moving code out of line) are more important than the branch prediction because cache misses
are more costly than branch misprediction.

That all said it might make sense in some really performance critical code,
especially if it's a in a loop and the gcc static branch predictors
(gcc has a large range of builtin heuristics that say e.g. (x < 0) or
(x == NULL) is unlikely). Most code is probably not performance critical
enough to justify the ugliness of the code annotations. And again
for many situations the builtin predictors of gcc (and the CPU) do
fine without help anyways.

Also if you add them you should at some point run with the unlikely
profiler patch from mm just to make sure that your guesses about
which paths are likely are actually correct. Humans are unfortunately
often wrong on such guesses.

Ideally (but that might ask for too much for normal code writing) you
would only add them to code where you have oprofile data for branch
mispredictions or icache misses.


<Prev in Thread] Current Thread [Next in Thread>