Date: Wed, 22 Feb 2012 08:25:38 +0100
From: Ingo Molnar <mingo@elte.hu>
To: "H. Peter Anvin" <hpa@zytor.com>
Cc: Jason Baron <jbaron@redhat.com>, a.p.zijlstra@chello.nl,
        rostedt@goodmis.org, mathieu.desnoyers@efficios.com,
        davem@davemloft.net, ddaney.cavm@gmail.com, akpm@linux-foundation.org,
        linux-kernel@vger.kernel.org,
        Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH 00/10] jump label: introduce very_[un]likely + cleanups +
 docs
Message-ID: <20120222072538.GA17291@elte.hu>
References: <cover.1329851692.git.jbaron@redhat.com>
 <4F43F9F0.4000605@zytor.com>
 <20120222065016.GA16923@elte.hu>
 <4F44934B.2000808@zytor.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4F44934B.2000808@zytor.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4574
Lines: 111


* H. Peter Anvin <hpa@zytor.com> wrote:

> On 02/21/2012 10:50 PM, Ingo Molnar wrote:
> > 
> > The pattern has spread beyond the niche of tracing internals and 
> > nobody seemed to have any second thoughts about the actual 
> > readability of these names. That is a major FAIL and it was my 
> > primary concern.
> > 
> > For those *reading* the code, the similarity of 
> > very_likely()/very_unlikely() to the likely()/unlikely() 
> > patterns is *INTENTIONAL*, as functionally the branch site 
> > itself is very similar to a real branch!
> > 
> > Secondly, for those *writing* such code, you cannot just 
> > 'accidentally' change a unlikely() to a very_unlikely() and get 
> > something you didn't ask for ...
> > 
> > It is the modification site that is dissimilar (and which is 
> > slow in the jump label case) - and that is very apparently 
> > dissimilar that it takes some effort to change these flags. If 
> > you write such code you have to add the whole jump label 
> > mechanism to it, which makes it very apparent what is happening 
> > and makes the costs very apparent as well.
> > 
> > Thirdly, the fact that it's a 'jump label' is an 
> > *implementational* detail. On some architectures very_unlikely() 
> > indeed maps to unlikely(), because jump labels have not been 
> > implemented yet. On some architectures very_unlikely() is 
> > implemented via jump labels. On some future architectures it 
> > might be implemented via JIT-ing the target function. (I made 
> > the last one up)
> > 
> > So it makes sense to decouple 'jump labels' from the actual high 
> > level naming of this API.
> > 
> > Anyway, I've Cc:-ed Linus and Andrew, I plan to take the renames 
> > but they can NAK me if doing that is stupid.
> > 
> 
> I have to VEHEMENTLY disagree with you here.
> 
> likely()/unlikely() are supposed to be used when it is clear 
> at the time the code is written which way the branch is biased 
> or which path is important (unlikely() is typically used for 
> performance-unimportant bailouts.)
>
> Jump labels are not that way.  The key aspect for when the 
> jump label optimization is relevant is *how often is the 
> direction changed*.  It is perfectly sane for this to be done 
> on a 50:50 branch, as long as the 50:50-ness is settled once 
> for all dring a boot. [...]

Erm, that's not at all true - jump labels have a strong build 
time bias/assymetry: the conditional block is moved totally out 
of line and the original fall-through code-path is left as 
straight a flow as possible.

Just look at the jump label usage site disassembly and check the 
historic evolution of this feature in the Git logs:

The *primary* benefit of jump labels was always to the 
fall-trough code. We hurt the tracepoint-enabled codepath 
*INTENTIONALLY*, to make sure the overwhelmingly common case of 
them being off is left alone as much as possible.

> [...]  Consider, for example, an Intel CPU versus an AMD CPU.  
> Wouldn't you agree that it would be absolutely ridiculous and 
> downright offensive to have:
> 
> 	if (very_unlikely(cpu_vendor_amd)) {
> 
> Yet this is a *fantastic* use of the jump labels, because your 
> CPU vendor isn't going to change in the middle of execution 
> (hot VM migrations excepted ;)

No, that condition perfectly validly represents what happens in 
reality: that the conditional AMD code is moved *out of line* - 
in most cases penalizing it in terms of instruction scheduling, 
etc.

There is a fundamental assymetry, and intentionally so. You 
*really* have to think what the common case is, and make sure 
the build defaults to that. It's not the end of the world to 
have it flipped over, but there's costs and those costs are 
higher even in the branch path than a regular 
likely()/unlikely().

So you are rather wrong about your expectations - I think that 
is one more piece of evidence that the naming was less than 
optimal.

> So the key aspect of this is the staticness of the 
> conditional, NOT the degree of bias of the branch.  Hence my 
> past insistence on the "static_branch" name (rather than 
> "jump_label")... the branch part can be omitted, as an 
> implementation detail, but the staticness of it is its 
> absolutely key defining characteristic.

I don't think you understand this facility as well as you think 
you do.

Thanks,

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