@@ -75,12 +75,12 @@ static inline __sum16 csum16_add(__sum16 csum, __be16 addend)return(__force__sum16)(res+(res<(__forceu16)addend));}-staticinline__sum16csum16_sub(__sum16csum,__be16addend)+static__always_inline__sum16csum16_sub(__sum16csum,__be16addend){returncsum16_add(csum,~addend);}-staticinline__wsumcsum_shift(__wsumsum,intoffset)+static__always_inline__wsumcsum_shift(__wsumsum,intoffset){/* rotate sum to align it with a 16b boundary */if(offset&1)
From: David Laight <hidden> Date: 2022-02-17 13:36:15
From: Christophe Leroy
Sent: 17 February 2022 12:19
All functions defined as static inline in net/checksum.h are
meant to be inlined for performance reason.
But since commit ac7c3e4ff401 ("compiler: enable
CONFIG_OPTIMIZE_INLINING forcibly") the compiler is allowed to
uninline functions when it wants.
Fair enough in the general case, but for tiny performance critical
checksum helpers that's counter-productive.
There isn't a real justification for allowing the compiler
to 'not inline' functions in that commit.
It rather seems backwards.
The kernel sources don't really have anything marked 'inline'
that shouldn't always be inlined.
If there are any such functions they are few and far between.
I've had enough trouble (elsewhere) getting gcc to inline
static functions that are only called once.
I ended up using 'always_inline'.
(That is 4k of embedded object code that will be too slow
if it ever spills a register to stack.)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Adding Ingo, Andrew and Nick as they were involved in the subjet,
Le 17/02/2022 à 14:36, David Laight a écrit :
From: Christophe Leroy
quoted
Sent: 17 February 2022 12:19
All functions defined as static inline in net/checksum.h are
meant to be inlined for performance reason.
But since commit ac7c3e4ff401 ("compiler: enable
CONFIG_OPTIMIZE_INLINING forcibly") the compiler is allowed to
uninline functions when it wants.
Fair enough in the general case, but for tiny performance critical
checksum helpers that's counter-productive.
There isn't a real justification for allowing the compiler
to 'not inline' functions in that commit.
Do you mean that the two following commits should be reverted:
- 889b3c1245de ("compiler: remove CONFIG_OPTIMIZE_INLINING entirely")
- 4c4e276f6491 ("net: Force inlining of checksum functions in
net/checksum.h")
It rather seems backwards.
The kernel sources don't really have anything marked 'inline'
that shouldn't always be inlined.
If there are any such functions they are few and far between.
I've had enough trouble (elsewhere) getting gcc to inline
static functions that are only called once.
I ended up using 'always_inline'.
(That is 4k of embedded object code that will be too slow
if it ever spills a register to stack.)
I agree with you that that change is a nightmare with many small
functions that we really want inlined, and when we force inlining we
most of the time get a smaller binary.
And it becomes even more problematic when we start adding
instrumentation like stack protector.
According to the original commits however this was supposed to provide
real benefit:
- 60a3cdd06394 ("x86: add optimized inlining")
- 9012d011660e ("compiler: allow all arches to enable
CONFIG_OPTIMIZE_INLINING")
But when I build ppc64le_defconfig + CONFIG_CC_OPTIMISE_FOR_SIZE I get:
112 times queued_spin_unlock()
122 times mmiowb_spin_unlock()
151 times cpu_online()
225 times __raw_spin_unlock()
So I was wondering, would we have a way to force inlining of functions
marked inline in header files while leaving GCC handling the ones in C
files the way it wants ?
Christophe
Adding Ingo, Andrew and Nick as they were involved in the subjet,
Le 17/02/2022 à 14:36, David Laight a écrit :
quoted
From: Christophe Leroy
quoted
Sent: 17 February 2022 12:19
All functions defined as static inline in net/checksum.h are
meant to be inlined for performance reason.
But since commit ac7c3e4ff401 ("compiler: enable
CONFIG_OPTIMIZE_INLINING forcibly") the compiler is allowed to
uninline functions when it wants.
Fair enough in the general case, but for tiny performance critical
checksum helpers that's counter-productive.
There isn't a real justification for allowing the compiler
to 'not inline' functions in that commit.
Do you mean that the two following commits should be reverted:
- 889b3c1245de ("compiler: remove CONFIG_OPTIMIZE_INLINING entirely")
- 4c4e276f6491 ("net: Force inlining of checksum functions in
net/checksum.h")
Of course not the above one (copy/paste error), but:
- ac7c3e4ff401 ("compiler: enable CONFIG_OPTIMIZE_INLINING forcibly")
quoted
It rather seems backwards.
The kernel sources don't really have anything marked 'inline'
that shouldn't always be inlined.
If there are any such functions they are few and far between.
I've had enough trouble (elsewhere) getting gcc to inline
static functions that are only called once.
I ended up using 'always_inline'.
(That is 4k of embedded object code that will be too slow
if it ever spills a register to stack.)
I agree with you that that change is a nightmare with many small
functions that we really want inlined, and when we force inlining we
most of the time get a smaller binary.
And it becomes even more problematic when we start adding
instrumentation like stack protector.
According to the original commits however this was supposed to provide
real benefit:
- 60a3cdd06394 ("x86: add optimized inlining")
- 9012d011660e ("compiler: allow all arches to enable
CONFIG_OPTIMIZE_INLINING")
But when I build ppc64le_defconfig + CONFIG_CC_OPTIMISE_FOR_SIZE I get:
112 times queued_spin_unlock()
122 times mmiowb_spin_unlock()
151 times cpu_online()
225 times __raw_spin_unlock()
So I was wondering, would we have a way to force inlining of functions
marked inline in header files while leaving GCC handling the ones in C
files the way it wants ?
Christophe
From: David Laight <hidden> Date: 2022-02-17 15:15:21
From: Christophe Leroy
Sent: 17 February 2022 14:55
Le 17/02/2022 à 15:50, Christophe Leroy a écrit :
quoted
Adding Ingo, Andrew and Nick as they were involved in the subjet,
Le 17/02/2022 à 14:36, David Laight a écrit :
quoted
From: Christophe Leroy
quoted
Sent: 17 February 2022 12:19
All functions defined as static inline in net/checksum.h are
meant to be inlined for performance reason.
But since commit ac7c3e4ff401 ("compiler: enable
CONFIG_OPTIMIZE_INLINING forcibly") the compiler is allowed to
uninline functions when it wants.
Fair enough in the general case, but for tiny performance critical
checksum helpers that's counter-productive.
There isn't a real justification for allowing the compiler
to 'not inline' functions in that commit.
Do you mean that the two following commits should be reverted:
- 889b3c1245de ("compiler: remove CONFIG_OPTIMIZE_INLINING entirely")
- 4c4e276f6491 ("net: Force inlining of checksum functions in
net/checksum.h")
Of course not the above one (copy/paste error), but:
- ac7c3e4ff401 ("compiler: enable CONFIG_OPTIMIZE_INLINING forcibly")
That's the one I looked at.
quoted
quoted
It rather seems backwards.
The kernel sources don't really have anything marked 'inline'
that shouldn't always be inlined.
If there are any such functions they are few and far between.
I've had enough trouble (elsewhere) getting gcc to inline
static functions that are only called once.
I ended up using 'always_inline'.
(That is 4k of embedded object code that will be too slow
if it ever spills a register to stack.)
I agree with you that that change is a nightmare with many small
functions that we really want inlined, and when we force inlining we
most of the time get a smaller binary.
And it becomes even more problematic when we start adding
instrumentation like stack protector.
According to the original commits however this was supposed to provide
real benefit:
- 60a3cdd06394 ("x86: add optimized inlining")
- 9012d011660e ("compiler: allow all arches to enable
CONFIG_OPTIMIZE_INLINING")
But when I build ppc64le_defconfig + CONFIG_CC_OPTIMISE_FOR_SIZE I get:
112 times queued_spin_unlock()
122 times mmiowb_spin_unlock()
151 times cpu_online()
225 times __raw_spin_unlock()
Yes, you either want them inlined, or a single copy of the real function.
I have seen a linker de-duplicate functions with identical bodies.
But I don't that gld does that for the kernel.
(Was confusing because both did structure->member = 0 but for entirely
different types.)
quoted
So I was wondering, would we have a way to force inlining of functions
marked inline in header files while leaving GCC handling the ones in C
files the way it wants ?
The view for those (in netdev at least) is just not to mark the inline
and let the compiler decide.
Although, IMHO, it tends to get it wrong quite often.
Often because it decides not to inline before the optimiser
removes all the constant conditionals.
Kernel developers ought to be clever enough to not inline
functions that are big.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Fri, Feb 18, 2022 at 12:15 AM David Laight [off-list ref] wrote:
From: Christophe Leroy
quoted
Sent: 17 February 2022 14:55
Le 17/02/2022 à 15:50, Christophe Leroy a écrit :
quoted
Adding Ingo, Andrew and Nick as they were involved in the subjet,
Le 17/02/2022 à 14:36, David Laight a écrit :
quoted
From: Christophe Leroy
quoted
Sent: 17 February 2022 12:19
All functions defined as static inline in net/checksum.h are
meant to be inlined for performance reason.
But since commit ac7c3e4ff401 ("compiler: enable
CONFIG_OPTIMIZE_INLINING forcibly") the compiler is allowed to
uninline functions when it wants.
Fair enough in the general case, but for tiny performance critical
checksum helpers that's counter-productive.
There isn't a real justification for allowing the compiler
to 'not inline' functions in that commit.
Do you mean that the two following commits should be reverted:
- 889b3c1245de ("compiler: remove CONFIG_OPTIMIZE_INLINING entirely")
- 4c4e276f6491 ("net: Force inlining of checksum functions in
net/checksum.h")
Of course not the above one (copy/paste error), but:
- ac7c3e4ff401 ("compiler: enable CONFIG_OPTIMIZE_INLINING forcibly")
That's the one I looked at.
No. Not that one.
The commit you presumably want to revert is:
a771f2b82aa2 ("[PATCH] Add a section about inlining to
Documentation/CodingStyle")
This is now referred to as "__always_inline disease", though.
CONFIG_OPTIMIZE_INLINING has 14 years of history for x86.
See commit 60a3cdd06394 ("x86: add optimized inlining").
We always give gcc freedom to not inline functions marked as inline.
--
Best Regards
Masahiro Yamada
From: David Laight <hidden> Date: 2022-02-17 16:49:57
From: Masahiro Yamada
Sent: 17 February 2022 16:17
...
No. Not that one.
The commit you presumably want to revert is:
a771f2b82aa2 ("[PATCH] Add a section about inlining to
Documentation/CodingStyle")
This is now referred to as "__always_inline disease", though.
That description is largely fine.
Inappropriate 'inline' ought to be removed.
Then 'inline' means - 'really do inline this'.
Anyone remember massive 100+ line #defines being
used to get code inlined 'to make it faster'.
Sometimes being expanded several times in succession.
May have helped a 68020, but likely to be a loss on
modern cpu with large I-cache and slow memory.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Fri, Feb 18, 2022 at 1:49 AM David Laight [off-list ref] wrote:
From: Masahiro Yamada
quoted
Sent: 17 February 2022 16:17
...
quoted
No. Not that one.
The commit you presumably want to revert is:
a771f2b82aa2 ("[PATCH] Add a section about inlining to
Documentation/CodingStyle")
This is now referred to as "__always_inline disease", though.
That description is largely fine.
Inappropriate 'inline' ought to be removed.
Then 'inline' means - 'really do inline this'.
You cannot change "static inline" to "static"
in header files.
If "static inline" meant __always_inline,
there would be no way to negate it.
That's why we need both inline and __always_inline.
Anyone remember massive 100+ line #defines being
used to get code inlined 'to make it faster'.
Sometimes being expanded several times in succession.
May have helped a 68020, but likely to be a loss on
modern cpu with large I-cache and slow memory.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Fri, Feb 18, 2022 at 3:10 AM Segher Boessenkool
[off-list ref] wrote:
On Fri, Feb 18, 2022 at 02:27:16AM +0900, Masahiro Yamada wrote:
quoted
On Fri, Feb 18, 2022 at 1:49 AM David Laight [off-list ref] wrote:
quoted
That description is largely fine.
Inappropriate 'inline' ought to be removed.
Then 'inline' means - 'really do inline this'.
You cannot change "static inline" to "static"
in header files.
Why not? Those two have identical semantics!
e.g.)
[1] Open include/linux/device.h with your favorite editor,
then edit
static inline void *devm_kcalloc(struct device *dev,
to
static void *devm_kcalloc(struct device *dev,
[2] Build the kernel
On Fri, Feb 18, 2022 at 10:35:48AM +0900, Masahiro Yamada wrote:
On Fri, Feb 18, 2022 at 3:10 AM Segher Boessenkool
[off-list ref] wrote:
quoted
On Fri, Feb 18, 2022 at 02:27:16AM +0900, Masahiro Yamada wrote:
quoted
On Fri, Feb 18, 2022 at 1:49 AM David Laight [off-list ref] wrote:
quoted
That description is largely fine.
Inappropriate 'inline' ought to be removed.
Then 'inline' means - 'really do inline this'.
You cannot change "static inline" to "static"
in header files.
Why not? Those two have identical semantics!
e.g.)
[1] Open include/linux/device.h with your favorite editor,
then edit
static inline void *devm_kcalloc(struct device *dev,
to
static void *devm_kcalloc(struct device *dev,
[2] Build the kernel
You get some "defined but not used" warnings that are shushed for
inlines. Do you see something else?
The semantics are the same. Warnings are just warnings. It builds
fine.
Segher
From: Stephen Hemminger <stephen@networkplumber.org> Date: 2022-02-18 16:29:28
On Fri, 18 Feb 2022 06:12:37 -0600
Segher Boessenkool [off-list ref] wrote:
On Fri, Feb 18, 2022 at 10:35:48AM +0900, Masahiro Yamada wrote:
quoted
On Fri, Feb 18, 2022 at 3:10 AM Segher Boessenkool
[off-list ref] wrote:
quoted
On Fri, Feb 18, 2022 at 02:27:16AM +0900, Masahiro Yamada wrote:
quoted
On Fri, Feb 18, 2022 at 1:49 AM David Laight [off-list ref] wrote:
quoted
That description is largely fine.
Inappropriate 'inline' ought to be removed.
Then 'inline' means - 'really do inline this'.
You cannot change "static inline" to "static"
in header files.
Why not? Those two have identical semantics!
e.g.)
[1] Open include/linux/device.h with your favorite editor,
then edit
static inline void *devm_kcalloc(struct device *dev,
to
static void *devm_kcalloc(struct device *dev,
[2] Build the kernel
You get some "defined but not used" warnings that are shushed for
inlines. Do you see something else?
The semantics are the same. Warnings are just warnings. It builds
fine.
Kernel code should build with zero warnings, the compiler is telling you
something.
On Fri, Feb 18, 2022 at 08:29:20AM -0800, Stephen Hemminger wrote:
On Fri, 18 Feb 2022 06:12:37 -0600
Segher Boessenkool [off-list ref] wrote:
quoted
On Fri, Feb 18, 2022 at 10:35:48AM +0900, Masahiro Yamada wrote:
quoted
On Fri, Feb 18, 2022 at 3:10 AM Segher Boessenkool
[off-list ref] wrote:
quoted
On Fri, Feb 18, 2022 at 02:27:16AM +0900, Masahiro Yamada wrote:
quoted
On Fri, Feb 18, 2022 at 1:49 AM David Laight [off-list ref] wrote:
quoted
That description is largely fine.
Inappropriate 'inline' ought to be removed.
Then 'inline' means - 'really do inline this'.
You cannot change "static inline" to "static"
in header files.
Why not? Those two have identical semantics!
e.g.)
[1] Open include/linux/device.h with your favorite editor,
then edit
static inline void *devm_kcalloc(struct device *dev,
to
static void *devm_kcalloc(struct device *dev,
[2] Build the kernel
You get some "defined but not used" warnings that are shushed for
inlines. Do you see something else?
The semantics are the same. Warnings are just warnings. It builds
fine.
Kernel code should build with zero warnings, the compiler is telling you
something.
The second part is of course true. The first part less so, and is in
fact not true at all from some points of view:
$ ./build --kernel x86_64
Building x86_64... (target x86_64-linux)
kernel: configure [00:06] build [02:12] 1949 warnings OK
(This is with a development version of GCC.)
There are simple ways to shut up specific warnings for specific code.
That is useful, certainly. And so is having a warning-free build. It
is obvious that we do survive without either of that though!
And none of this detracts from the point that the semantics of "static"
and "static inline" are identical.
Segher
From: David Laight <hidden> Date: 2022-02-18 08:41:47
From: Masahiro Yamada
Sent: 17 February 2022 17:27
On Fri, Feb 18, 2022 at 1:49 AM David Laight [off-list ref] wrote:
quoted
From: Masahiro Yamada
quoted
Sent: 17 February 2022 16:17
...
quoted
No. Not that one.
The commit you presumably want to revert is:
a771f2b82aa2 ("[PATCH] Add a section about inlining to
Documentation/CodingStyle")
This is now referred to as "__always_inline disease", though.
That description is largely fine.
Inappropriate 'inline' ought to be removed.
Then 'inline' means - 'really do inline this'.
You cannot change "static inline" to "static"
in header files.
You'd need some 'magicary' to get an extern except for a special
include that generated the visible function.
It has been done.
If "static inline" meant __always_inline,
there would be no way to negate it.
That's why we need both inline and __always_inline.
I'd go the other way, 'inline' and 'inline_for_code_bloat'
(or maybe inline_for_speed).
Much the same as the noinline's to stop stack bloat.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: Joe Perches <joe@perches.com> Date: 2022-02-17 15:42:15
On Thu, 2022-02-17 at 13:19 +0100, Christophe Leroy wrote:
All functions defined as static inline in net/checksum.h are
meant to be inlined for performance reason.
But since commit ac7c3e4ff401 ("compiler: enable
CONFIG_OPTIMIZE_INLINING forcibly") the compiler is allowed to
uninline functions when it wants.
Fair enough in the general case, but for tiny performance critical
checksum helpers that's counter-productive.