LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Arjan van de Ven <arjan@infradead.org>
To: Geoff Levand <geoffrey.levand@am.sony.com>
Cc: Willy Tarreau <w@1wt.eu>, Roel Kluin <12o3l@tiscali.nl>,
	linuxppc-dev@ozlabs.org, cbe-oss-dev@ozlabs.org,
	lkml <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/3] Fix Unlikely(x) == y
Date: Sat, 16 Feb 2008 10:39:27 -0800	[thread overview]
Message-ID: <20080216103927.2a02352b@laptopd505.fenrus.org> (raw)
In-Reply-To: <47B72BFE.9060302@am.sony.com>

On Sat, 16 Feb 2008 10:31:26 -0800
Geoff Levand <geoffrey.levand@am.sony.com> wrote:

> On 02/16/2008 09:42 AM, Arjan van de Ven wrote:
> > On Sat, 16 Feb 2008 18:33:16 +0100
> > Willy Tarreau <w@1wt.eu> wrote:
> > 
> >> On Sat, Feb 16, 2008 at 09:25:52AM -0800, Arjan van de Ven wrote:
> >> > On Sat, 16 Feb 2008 17:08:01 +0100
> >> > Roel Kluin <12o3l@tiscali.nl> wrote:
> >> > 
> >> > > The patch below was not yet tested. If it's correct as it is,
> >> > > please comment. ---
> >> > > Fix Unlikely(x) == y
> >> > > 
> >> > 
> >> > you found a great set of bugs..
> >> > but to be honest... I suspect it's just best to remove unlikely
> >> > altogether for these cases; unlikely() is almost a
> >> > go-faster-stripes thing, and if you don't know how to use it you
> >> > shouldn't be using it... so just removing it for all wrong cases
> >> > is actually the best thing to do imo.
> >> 
> >> Well, eventhough the author may not know how to use it, "unlikely"
> >> at least indicates the intention of the author, or his knowledge
> >> of what should happen here. I'd suggest leaving it where it is
> >> because the authot of this code is in best position to know that
> >> this branch is unlikely to happen, eventhough he does not
> >> correctly use the macro.
> >>
> > 
> > you have more faith in the authors knowledge of how his code
> > actually behaves than I think is warranted  :) Or faith in that he
> > knows what "unlikely" means. I should write docs about this; but
> > unlikely() means: 1) It happens less than 0.01% of the cases.
> > 2) The compiler couldn't have figured this out by itself
> >    (NULL pointer checks are compiler done already, same for some
> > other conditions) 3) It's a hot codepath where shaving 0.5 cycles
> > (less even on x86) matters (and the author is ok with taking a 500
> > cycles hit if he's wrong)
> > 
> > If you think unlikely() means something else, we should fix what it
> > maps to towards gcc ;) (to.. be empty ;)
> 
> Well, I didn't consider what today's compiler does, but used it as a
> general indicator, because I think that code will be around a long
> time.  If you show me some test results that prove it causes harm I
> might consider removing it. 

for mordern (last 10 years) x86 cpus... the cpu branchpredictor is better
than the coder in general. Same for most other architectures.

unlikely() creates bigger code as well.

Now... we're talking about your super duper hotpath function here right?
One where you care about 0.5 cycle speed improvement? (less on modern
systems ;)

Because if not, the bigger code and general "compiler second guessing" is just
more harmful than good, shown already here by the fact that the code was just incorrect
as a virtue of having the unlikely() in.

-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

  reply	other threads:[~2008-02-16 18:39 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-16 16:08 Roel Kluin
2008-02-16 17:25 ` Arjan van de Ven
2008-02-16 17:33   ` Willy Tarreau
2008-02-16 17:42     ` Arjan van de Ven
2008-02-16 17:58       ` Willy Tarreau
2008-02-16 18:29         ` Arjan van de Ven
2008-02-17  9:45         ` [Cbe-oss-dev] " Andrew Pinski
2008-02-17 10:08           ` Willy Tarreau
2008-02-16 18:31       ` Geoff Levand
2008-02-16 18:39         ` Arjan van de Ven [this message]
2008-02-17 11:50           ` Michael Ellerman
2008-02-18 13:56             ` Adrian Bunk
2008-02-18 14:01               ` Geert Uytterhoeven
2008-02-18 14:13                 ` Adrian Bunk
2008-02-18 21:46                   ` Michael Ellerman
2008-02-19  7:43                     ` Adrian Bunk
2008-02-18 19:22                 ` [Cbe-oss-dev] " Andrew Pinski
2008-02-18 14:27               ` David Howells
2008-02-18 14:59                 ` Roel Kluin
2008-02-18 18:11                 ` Valdis.Kletnieks
2008-02-18 18:33                   ` Arjan van de Ven
2008-02-18 14:39       ` Andi Kleen
2008-02-19  2:33         ` Nick Piggin
2008-02-19  2:40           ` Arjan van de Ven
2008-02-19  4:41             ` Nick Piggin
2008-02-19  5:58           ` Willy Tarreau
2008-02-19  6:20             ` Nick Piggin
2008-02-19  9:28             ` Andi Kleen
2008-02-20  7:32               ` Willy Tarreau
2008-02-19  9:25           ` Andi Kleen
2008-02-19  9:46             ` Nick Piggin
2008-02-19  9:57               ` Andi Kleen
2008-02-19 22:25                 ` Nick Piggin
2008-02-16 18:41 ` Geoff Levand

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=20080216103927.2a02352b@laptopd505.fenrus.org \
    --to=arjan@infradead.org \
    --cc=12o3l@tiscali.nl \
    --cc=cbe-oss-dev@ozlabs.org \
    --cc=geoffrey.levand@am.sony.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@ozlabs.org \
    --cc=w@1wt.eu \
    --subject='Re: [PATCH 1/3] Fix Unlikely(x) == y' \
    /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

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