Messages in this thread | | | Date | Thu, 23 Feb 2012 08:33:44 +1100 | From | Paul Mackerras <> | Subject | Re: [PATCH 00/10] jump label: introduce very_[un]likely + cleanups + docs |
| |
On Wed, Feb 22, 2012 at 09:18:55AM +0100, Ingo Molnar wrote:
> The problem with static_branch_def_false/def_true was that the > very intuitively visible bias that we see with > likely()/unlikely() is confused in jump label constructs through > two layers of modifiers. And the fix is so easy, a simple rename > in most cases ;-) > > So instead of that, in this series we have: > > + if (very_unlikely(&perf_sched_events.key)) > > which is a heck of an improvement IMO. I'd still up its > readability a notch, by also signalling the overhead of the > update path by making it: > > + if (very_unlikely(&perf_sched_events.slow_flag)) > > ... but I don't want to be that much of a readability nazi ;-)
I have to say I don't like the "very_unlikely" name. It's confusing because the condition being evaluated appears to be the address of something, i.e. &perf_sched_events.key in your example, and that looks to me to be very very likely to be true, i.e. non-zero. But the code is telling me that's very *un*likely, which is confusing.
In other words I can't think about "very_unlikely" as being similar to "unlikely". Clearly very_unlikely isn't just passing through the true/falseness of its argument to the surrounding if, the way that unlikely does; it (very_unlikely) is a real function that is doing something more complicated with its argument -- dereferencing it, conceptually at least, for a start.
Just from a readability perspective, I like the static_branch name, and if we want to add information about the execution bias, I'd suggest we do:
if (likely(static_branch(&x))) ...
if (unlikely(static_branch(&x))) ...
if that can be made to work, or else have variants like this:
if (likely_static_branch(&x)) ...
if (unlikely_static_branch(&x)) ...
Paul.
| |