Messages in this thread | | | Date | Thu, 23 Feb 2012 11:02:05 +0100 | From | Ingo Molnar <> | Subject | Re: [PATCH 00/10] jump label: introduce very_[un]likely + cleanups + docs |
| |
* Paul Mackerras <paulus@samba.org> wrote:
> 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.
Having to take the address gives us type safety - i.e. it will not be possible to accidentally pass in a non-jump-label key and get it misinterpreted.
If some macro magic could be used to remove the address taking I'd be in favor of such a simplification, i.e.:
if (very_unlikely(perf_sched_events.key))
which should address your observation.
Thanks,
Ingo
| |