linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "H. Peter Anvin" <hpa@zytor.com>
To: Ingo Molnar <mingo@elte.hu>
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
Date: Tue, 21 Feb 2012 23:03:39 -0800	[thread overview]
Message-ID: <4F44934B.2000808@zytor.com> (raw)
In-Reply-To: <20120222065016.GA16923@elte.hu>

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.  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 ;)

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.

	-hpa


-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.


  reply	other threads:[~2012-02-22  7:04 UTC|newest]

Thread overview: 84+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-02-21 20:02 [PATCH 00/10] jump label: introduce very_[un]likely + cleanups + docs Jason Baron
2012-02-21 20:02 ` [PATCH 01/10] jump label: Add a WARN() if jump label key count goes negative Jason Baron
2012-02-29 10:13   ` [tip:perf/core] " tip-bot for Jason Baron
2012-02-21 20:02 ` [PATCH 02/10] jump label: fix compiler warning Jason Baron
2012-02-29 10:14   ` [tip:perf/core] jump label: Fix " tip-bot for Jason Baron
2012-02-21 20:03 ` [PATCH 03/10] jump label: introduce very_unlikely() Jason Baron
2012-02-21 20:03 ` [PATCH 04/10] jump label: introduce very_likely() Jason Baron
2012-02-21 20:03 ` [PATCH 05/10] perf: update to use 'very_unlikely()' Jason Baron
2012-02-21 20:03 ` [PATCH 06/10] tracepoints: update to use very_unlikely() Jason Baron
2012-02-21 20:03 ` [PATCH 07/10] sched: update to use very_[un]likely() Jason Baron
2012-02-21 20:03 ` [PATCH 08/10] kvm: update to use very_unlikely() Jason Baron
2012-02-21 20:03 ` [PATCH 09/10] net: " Jason Baron
2012-02-21 20:03 ` [PATCH 10/10] jump label: Add docs better explaining the whole jump label mechanism Jason Baron
2012-02-29 10:15   ` [tip:perf/core] static keys: Add docs better explaining the whole 'struct static_key' mechanism tip-bot for Jason Baron
2012-02-21 20:09 ` [PATCH 00/10] jump label: introduce very_[un]likely + cleanups + docs H. Peter Anvin
2012-02-21 20:20   ` Jason Baron
2012-02-21 20:39     ` Steven Rostedt
2012-02-21 21:10       ` H. Peter Anvin
2012-02-21 21:16         ` Mathieu Desnoyers
2012-02-21 21:11       ` Mathieu Desnoyers
2012-02-22  7:32       ` Ingo Molnar
2012-02-22  7:53         ` Ingo Molnar
2012-02-22  8:01           ` H. Peter Anvin
2012-02-22  8:18             ` Ingo Molnar
2012-02-22  8:58               ` [PATCH] static keys: Introduce 'struct static_key', very_[un]likely(), static_key_slow_[inc|dec]() Ingo Molnar
2012-02-29 10:16                 ` [tip:perf/core] static keys: Introduce 'struct static_key', static_key_true()/false() and static_key_slow_[inc|dec]() tip-bot for Ingo Molnar
2012-02-22  9:03               ` [PATCH] jump labels: Explain the .config option better Ingo Molnar
2012-02-22 15:08               ` [PATCH 00/10] jump label: introduce very_[un]likely + cleanups + docs H. Peter Anvin
2012-02-22 15:51                 ` Ingo Molnar
2012-02-22 21:33               ` Paul Mackerras
2012-02-23 10:02                 ` Ingo Molnar
2012-02-23 16:21                   ` Jason Baron
2012-02-23 17:10                     ` H. Peter Anvin
2012-02-24  9:11                     ` Ingo Molnar
2012-02-23 17:18                   ` Linus Torvalds
2012-02-23 22:33                     ` Ingo Molnar
2012-02-23 22:39                       ` Ingo Molnar
2012-02-23 22:44                         ` Steven Rostedt
2012-02-23 23:18                         ` Mathieu Desnoyers
2012-02-24  2:25                           ` Jason Baron
2012-02-24  9:08                             ` Ingo Molnar
2012-02-24 15:35                               ` Jason Baron
2012-02-27  7:40                                 ` Ingo Molnar
2012-02-24 15:51                               ` Mathieu Desnoyers
2012-02-24 16:06                               ` Steven Rostedt
2012-02-24  7:52                         ` static keys: Introduce 'struct static_key', static_key_true()/false() and static_key_slow_[inc|dec]() Ingo Molnar
2012-02-24  7:59                           ` [PATCH] " Ingo Molnar
2012-02-24  7:54                         ` [PATCH] static keys: Add docs better explaining the whole 'struct static_key' mechanism Ingo Molnar
2012-02-23 22:45                       ` [PATCH 00/10] jump label: introduce very_[un]likely + cleanups + docs Linus Torvalds
2012-02-24  8:04                         ` Ingo Molnar
2012-02-24  2:42                       ` Jason Baron
2012-02-21 20:21   ` Steven Rostedt
2012-02-21 21:11     ` H. Peter Anvin
2012-02-22  6:50   ` Ingo Molnar
2012-02-22  7:03     ` H. Peter Anvin [this message]
2012-02-22  7:25       ` Ingo Molnar
2012-02-22  7:35         ` H. Peter Anvin
2012-02-22  7:48           ` Ingo Molnar
2012-02-22  7:55             ` H. Peter Anvin
2012-02-22  8:06               ` Ingo Molnar
2012-02-22 13:22                 ` Steven Rostedt
2012-02-22 13:34                   ` Ingo Molnar
2012-02-22 13:54                     ` Steven Rostedt
2012-02-22 14:20                       ` Mathieu Desnoyers
2012-02-22 14:36                         ` Steven Rostedt
2012-02-22 14:56                       ` Ingo Molnar
2012-02-22 15:11                         ` H. Peter Anvin
2012-02-22 15:11                         ` Peter Zijlstra
2012-02-22 15:47                           ` Ingo Molnar
2012-02-22 15:12                         ` Steven Rostedt
2012-02-22 15:15                           ` H. Peter Anvin
2012-02-22 15:19                           ` H. Peter Anvin
2012-02-22 15:42                           ` Jason Baron
2012-02-22 15:54                             ` Steven Rostedt
2012-02-22 15:56                               ` Jason Baron
2012-02-22 16:08                                 ` Steven Rostedt
2012-02-22 15:19                         ` Jason Baron
2012-02-22 15:40                           ` H. Peter Anvin
2012-02-22 15:13                       ` Peter Zijlstra
2012-02-22 15:32                         ` Peter Zijlstra
2012-02-22 17:14                           ` Richard Henderson
2012-02-22 18:28                             ` H. Peter Anvin
2012-02-22 18:58                             ` Jason Baron
2012-02-22 19:10                               ` H. Peter Anvin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4F44934B.2000808@zytor.com \
    --to=hpa@zytor.com \
    --cc=a.p.zijlstra@chello.nl \
    --cc=akpm@linux-foundation.org \
    --cc=davem@davemloft.net \
    --cc=ddaney.cavm@gmail.com \
    --cc=jbaron@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mathieu.desnoyers@efficios.com \
    --cc=mingo@elte.hu \
    --cc=rostedt@goodmis.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).