All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
To: Hagen Paul Pfeifer <hagen@jauu.net>
Cc: Markus Trippelsdorf <markus@trippelsdorf.de>,
	Andrew Morton <akpm@linux-foundation.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Ingo Molnar <mingo@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>,
	"David S. Miller" <davem@davemloft.net>,
	x86@kernel.org
Subject: Re: [PATCH] enforce function inlining for hot functions
Date: Sat, 25 Apr 2015 06:51:35 -0700	[thread overview]
Message-ID: <20150425135135.GN5561@linux.vnet.ibm.com> (raw)
In-Reply-To: <CAPh34mfAhXqfd8wOuXUM_VSHOWUxh-S3o-Lz=0+sRKBzz-Pe6Q@mail.gmail.com>

On Sat, Apr 25, 2015 at 03:26:48PM +0200, Hagen Paul Pfeifer wrote:
> On 25 April 2015 at 12:31, Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> 
> > I am not arguing either way on the wisdom or lack thereof of gcc's
> > inlining decisions.  But PROVE_RCU=n and CONFIG_DEBUG_LOCK_ALLOC=n should
> > make rcu_read_lock() and rcu_read_unlock() both be empty functions in
> > a CONFIG_PREEMPT=n, which should hopefully trivialize gcc's inlining
> > decisions in that particular case.
> 
> Hey Paul,
> 
> yes, with DEBUG_LOCK_ALLOC disabled all rcu_read_lock and unlock
> functions are perfectly inlined.

Whew!!!  ;-)

>                                  So now we have the following
> situation: depending on the gcc version and the particular kernel
> configuration some hot functions are not inlined - they are duplicated
> hundred times. Which is bad no matter how you consider
> gcc/kernel-configuration. I think this should *never* happened.
> 
> With the patch we can make sure that hot functions are *always*
> inlined - no matter what gcc version and kernel configuration is used.
> 
> Furthermore, as Markus already noted: compiled with -O2 this do not
> happened. Duplicates are only generated for -Os[1]. Ok, but now the
> question: should this happened for Os? I don't think so. I think we
> can do it better and mark these few functions as always inline. For
> the remaining inlined marked function we should provide gcc the
> flexibility and do not artificially enforce inlining. The current
> situation is bad: OPTIMIZE_INLINING is default no, which defacto
> enforces inlining for *all* inlined marked functions. GCC inlining
> mechanism is defacto disabled, which is also bad. Last but not least:
> the patch do not change anything for the current user, because we will
> still disable OPTIMIZE_INLINING (resulting in __always_inline for all
> inlined marked functions). The patch effects users who enable
> OPTIMIZE_INLINING and trust the compiler.
> 
> Hagen
> 
> PS: thank you Markus for the comment.
> 
> [1] which is nonsense: the functions are not inlined yet, but are
> copied hundred times for "size optimized builds". gcc should rather
> redeclare the functions global, define it one time and call this
> function every time. But implementing such a scheme is probably a
> monster of itself and LTO is required so solve all issues with such a
> concept.

I am guessing that there is only one duplicate per compilation unit?
I would also guess that the LTO guys would have a ready solution.  ;-)

That said, if a function was invoked extremely many times, it might
make sense to duplicate it even within a single compilation unit if
doing so allowed saving more than the size of the function in the
form of call instructions with shorter address fields.  But I have
no idea whether or not gcc would do this sort of thing.

							Thanx, Paul


  reply	other threads:[~2015-04-25 13:51 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-04-23 21:40 [PATCH] enforce function inlining for hot functions Hagen Paul Pfeifer
2015-04-24 19:49 ` Andrew Morton
2015-04-24 20:13   ` Paul E. McKenney
2015-04-24 20:44     ` Hagen Paul Pfeifer
2015-04-24 23:10     ` Hagen Paul Pfeifer
2015-04-25 10:31       ` Paul E. McKenney
2015-04-25 13:26         ` Hagen Paul Pfeifer
2015-04-25 13:51           ` Paul E. McKenney [this message]
2015-04-24 20:39   ` Hagen Paul Pfeifer
2015-04-25 10:53     ` Markus Trippelsdorf

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=20150425135135.GN5561@linux.vnet.ibm.com \
    --to=paulmck@linux.vnet.ibm.com \
    --cc=akpm@linux-foundation.org \
    --cc=davem@davemloft.net \
    --cc=hagen@jauu.net \
    --cc=hpa@zytor.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=markus@trippelsdorf.de \
    --cc=mingo@redhat.com \
    --cc=x86@kernel.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.