[PATCH net v3] net: Force inlining of checksum functions in net/checksum.h

[PATCH net v3] net: Force inlining of checksum functions in net/checksum.h

[Christophe Leroy]

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.

The problem mainly arises when selecting CONFIG_CC_OPTIMISE_FOR_SIZE,
Those helpers being 'static inline' in header files you suddenly find
them duplicated many times in the resulting vmlinux.

Here is a typical exemple when building powerpc pmac32_defconfig
with CONFIG_CC_OPTIMISE_FOR_SIZE. csum_sub() appears 4 times:

	c04a23cc <csum_sub>:
	c04a23cc:	7c 84 20 f8 	not     r4,r4
	c04a23d0:	7c 63 20 14 	addc    r3,r3,r4
	c04a23d4:	7c 63 01 94 	addze   r3,r3
	c04a23d8:	4e 80 00 20 	blr
		...
	c04a2ce8:	4b ff f6 e5 	bl      c04a23cc <csum_sub>
		...
	c04a2d2c:	4b ff f6 a1 	bl      c04a23cc <csum_sub>
		...
	c04a2d54:	4b ff f6 79 	bl      c04a23cc <csum_sub>
		...
	c04a754c <csum_sub>:
	c04a754c:	7c 84 20 f8 	not     r4,r4
	c04a7550:	7c 63 20 14 	addc    r3,r3,r4
	c04a7554:	7c 63 01 94 	addze   r3,r3
	c04a7558:	4e 80 00 20 	blr
		...
	c04ac930:	4b ff ac 1d 	bl      c04a754c <csum_sub>
		...
	c04ad264:	4b ff a2 e9 	bl      c04a754c <csum_sub>
		...
	c04e3b08 <csum_sub>:
	c04e3b08:	7c 84 20 f8 	not     r4,r4
	c04e3b0c:	7c 63 20 14 	addc    r3,r3,r4
	c04e3b10:	7c 63 01 94 	addze   r3,r3
	c04e3b14:	4e 80 00 20 	blr
		...
	c04e5788:	4b ff e3 81 	bl      c04e3b08 <csum_sub>
		...
	c04e65c8:	4b ff d5 41 	bl      c04e3b08 <csum_sub>
		...
	c0512d34 <csum_sub>:
	c0512d34:	7c 84 20 f8 	not     r4,r4
	c0512d38:	7c 63 20 14 	addc    r3,r3,r4
	c0512d3c:	7c 63 01 94 	addze   r3,r3
	c0512d40:	4e 80 00 20 	blr
		...
	c0512dfc:	4b ff ff 39 	bl      c0512d34 <csum_sub>
		...
	c05138bc:	4b ff f4 79 	bl      c0512d34 <csum_sub>
		...

Restore the expected behaviour by using __always_inline for all
functions defined in net/checksum.h

vmlinux size is even reduced by 256 bytes with this patch:

	   text	   data	    bss	    dec	    hex	filename
	6980022	2515362	 194384	9689768	 93daa8	vmlinux.before
	6979862	2515266	 194384	9689512	 93d9a8	vmlinux.now

Fixes: ac7c3e4ff401 ("compiler: enable CONFIG_OPTIMIZE_INLINING forcibly")
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v3: Added fixes tag and handled checkpatch warning about length of lines.

v2: Rebased on net tree
---
 include/net/checksum.h | 45 +++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/include/net/checksum.h b/include/net/checksum.h
index 5218041e5c8f..712b554a23be 100644
--- a/include/net/checksum.h
+++ b/include/net/checksum.h
@@ -22,7 +22,7 @@
 #include <asm/checksum.h>
 
 #ifndef _HAVE_ARCH_COPY_AND_CSUM_FROM_USER
-static inline
+static __always_inline
 __wsum csum_and_copy_from_user (const void __user *src, void *dst,
 				      int len)
 {
@@ -33,7 +33,7 @@ __wsum csum_and_copy_from_user (const void __user *src, void *dst,
 #endif
 
 #ifndef HAVE_CSUM_COPY_USER
-static __inline__ __wsum csum_and_copy_to_user
+static __always_inline __wsum csum_and_copy_to_user
 (const void *src, void __user *dst, int len)
 {
 	__wsum sum = csum_partial(src, len, ~0U);
@@ -45,7 +45,7 @@ static __inline__ __wsum csum_and_copy_to_user
 #endif
 
 #ifndef _HAVE_ARCH_CSUM_AND_COPY
-static inline __wsum
+static __always_inline __wsum
 csum_partial_copy_nocheck(const void *src, void *dst, int len)
 {
 	memcpy(dst, src, len);
@@ -54,7 +54,7 @@ csum_partial_copy_nocheck(const void *src, void *dst, int len)
 #endif
 
 #ifndef HAVE_ARCH_CSUM_ADD
-static inline __wsum csum_add(__wsum csum, __wsum addend)
+static __always_inline __wsum csum_add(__wsum csum, __wsum addend)
 {
 	u32 res = (__force u32)csum;
 	res += (__force u32)addend;
@@ -62,12 +62,12 @@ static inline __wsum csum_add(__wsum csum, __wsum addend)
 }
 #endif
 
-static inline __wsum csum_sub(__wsum csum, __wsum addend)
+static __always_inline __wsum csum_sub(__wsum csum, __wsum addend)
 {
 	return csum_add(csum, ~addend);
 }
 
-static inline __sum16 csum16_add(__sum16 csum, __be16 addend)
+static __always_inline __sum16 csum16_add(__sum16 csum, __be16 addend)
 {
 	u16 res = (__force u16)csum;
 
@@ -75,12 +75,12 @@ static inline __sum16 csum16_add(__sum16 csum, __be16 addend)
 	return (__force __sum16)(res + (res < (__force u16)addend));
 }
 
-static inline __sum16 csum16_sub(__sum16 csum, __be16 addend)
+static __always_inline __sum16 csum16_sub(__sum16 csum, __be16 addend)
 {
 	return csum16_add(csum, ~addend);
 }
 
-static inline __wsum csum_shift(__wsum sum, int offset)
+static __always_inline __wsum csum_shift(__wsum sum, int offset)
 {
 	/* rotate sum to align it with a 16b boundary */
 	if (offset & 1)
@@ -88,42 +88,43 @@ static inline __wsum csum_shift(__wsum sum, int offset)
 	return sum;
 }
 
-static inline __wsum
+static __always_inline __wsum
 csum_block_add(__wsum csum, __wsum csum2, int offset)
 {
 	return csum_add(csum, csum_shift(csum2, offset));
 }
 
-static inline __wsum
+static __always_inline __wsum
 csum_block_add_ext(__wsum csum, __wsum csum2, int offset, int len)
 {
 	return csum_block_add(csum, csum2, offset);
 }
 
-static inline __wsum
+static __always_inline __wsum
 csum_block_sub(__wsum csum, __wsum csum2, int offset)
 {
 	return csum_block_add(csum, ~csum2, offset);
 }
 
-static inline __wsum csum_unfold(__sum16 n)
+static __always_inline __wsum csum_unfold(__sum16 n)
 {
 	return (__force __wsum)n;
 }
 
-static inline __wsum csum_partial_ext(const void *buff, int len, __wsum sum)
+static __always_inline
+__wsum csum_partial_ext(const void *buff, int len, __wsum sum)
 {
 	return csum_partial(buff, len, sum);
 }
 
 #define CSUM_MANGLED_0 ((__force __sum16)0xffff)
 
-static inline void csum_replace_by_diff(__sum16 *sum, __wsum diff)
+static __always_inline void csum_replace_by_diff(__sum16 *sum, __wsum diff)
 {
 	*sum = csum_fold(csum_add(diff, ~csum_unfold(*sum)));
 }
 
-static inline void csum_replace4(__sum16 *sum, __be32 from, __be32 to)
+static __always_inline void csum_replace4(__sum16 *sum, __be32 from, __be32 to)
 {
 	__wsum tmp = csum_sub(~csum_unfold(*sum), (__force __wsum)from);
 
@@ -136,7 +137,7 @@ static inline void csum_replace4(__sum16 *sum, __be32 from, __be32 to)
  *  m : old value of a 16bit field
  *  m' : new value of a 16bit field
  */
-static inline void csum_replace2(__sum16 *sum, __be16 old, __be16 new)
+static __always_inline void csum_replace2(__sum16 *sum, __be16 old, __be16 new)
 {
 	*sum = ~csum16_add(csum16_sub(~(*sum), old), new);
 }
@@ -150,15 +151,15 @@ void inet_proto_csum_replace16(__sum16 *sum, struct sk_buff *skb,
 void inet_proto_csum_replace_by_diff(__sum16 *sum, struct sk_buff *skb,
 				     __wsum diff, bool pseudohdr);
 
-static inline void inet_proto_csum_replace2(__sum16 *sum, struct sk_buff *skb,
-					    __be16 from, __be16 to,
-					    bool pseudohdr)
+static __always_inline
+void inet_proto_csum_replace2(__sum16 *sum, struct sk_buff *skb,
+			      __be16 from, __be16 to, bool pseudohdr)
 {
 	inet_proto_csum_replace4(sum, skb, (__force __be32)from,
 				 (__force __be32)to, pseudohdr);
 }
 
-static inline __wsum remcsum_adjust(void *ptr, __wsum csum,
+static __always_inline __wsum remcsum_adjust(void *ptr, __wsum csum,
 				    int start, int offset)
 {
 	__sum16 *psum = (__sum16 *)(ptr + offset);
@@ -175,12 +176,12 @@ static inline __wsum remcsum_adjust(void *ptr, __wsum csum,
 	return delta;
 }
 
-static inline void remcsum_unadjust(__sum16 *psum, __wsum delta)
+static __always_inline void remcsum_unadjust(__sum16 *psum, __wsum delta)
 {
 	*psum = csum_fold(csum_sub(delta, (__force __wsum)*psum));
 }
 
-static inline __wsum wsum_negate(__wsum val)
+static __always_inline __wsum wsum_negate(__wsum val)
 {
 	return (__force __wsum)-((__force u32)val);
 }
-- 
2.34.1

RE: [PATCH net v3] net: Force inlining of checksum functions in net/checksum.h

[David Laight]

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)

Re: [PATCH net v3] net: Force inlining of checksum functions in net/checksum.h

[Christophe Leroy]

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

Re: [PATCH net v3] net: Force inlining of checksum functions in net/checksum.h

[Christophe Leroy]

Le 17/02/2022 à 15:50, Christophe Leroy a écrit :
> 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
>>> 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")


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

RE: [PATCH net v3] net: Force inlining of checksum functions in net/checksum.h

[David Laight]

From: Christophe Leroy
> Sent: 17 February 2022 14:55
> 
> Le 17/02/2022 à 15:50, Christophe Leroy a écrit :
> > 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
> >>> 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.

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

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

Re: [PATCH net v3] net: Force inlining of checksum functions in net/checksum.h

[Joe Perches]

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.

Thanks.  Trivial style notes:

> diff --git a/include/net/checksum.h b/include/net/checksum.h
[]
> @@ -22,7 +22,7 @@
>  #include <asm/checksum.h>
>  
>  #ifndef _HAVE_ARCH_COPY_AND_CSUM_FROM_USER
> -static inline
> +static __always_inline
>  __wsum csum_and_copy_from_user (const void __user *src, void *dst,
>  				      int len)
>  {

__wsum might be better placed on the previous line.

[]

> @@ -45,7 +45,7 @@ static __inline__ __wsum csum_and_copy_to_user
>  #endif
>  
>  #ifndef _HAVE_ARCH_CSUM_AND_COPY
> -static inline __wsum
> +static __always_inline __wsum
>  csum_partial_copy_nocheck(const void *src, void *dst, int len)

To be consistent with the location of the __wsum return value
when splitting the function definitions across multiple lines.

(like the below)

> @@ -88,42 +88,43 @@ static inline __wsum csum_shift(__wsum sum, int offset)
>  	return sum;
>  }
>  
> -static inline __wsum
> +static __always_inline __wsum
>  csum_block_add(__wsum csum, __wsum csum2, int offset)
>  {
>  	return csum_add(csum, csum_shift(csum2, offset));
>  }
>  
> -static inline __wsum
> +static __always_inline __wsum
>  csum_block_add_ext(__wsum csum, __wsum csum2, int offset, int len)
>  {
>  	return csum_block_add(csum, csum2, offset);
>  }
>  
> -static inline __wsum
> +static __always_inline __wsum
>  csum_block_sub(__wsum csum, __wsum csum2, int offset)
>  {
>  	return csum_block_add(csum, ~csum2, offset);
>  }
>  
> -static inline __wsum csum_unfold(__sum16 n)
> +static __always_inline __wsum csum_unfold(__sum16 n)
>  {
>  	return (__force __wsum)n;
>  }
>  

[]

> -static inline __wsum csum_partial_ext(const void *buff, int len, __wsum sum)
> +static __always_inline
> +__wsum csum_partial_ext(const void *buff, int len, __wsum sum)
>  {
>  	return csum_partial(buff, len, sum);
>  }

And this __wsum could be moved too.

> @@ -150,15 +151,15 @@ void inet_proto_csum_replace16(__sum16 *sum, struct sk_buff *skb,
[]
> -static inline __wsum remcsum_adjust(void *ptr, __wsum csum,
> +static __always_inline __wsum remcsum_adjust(void *ptr, __wsum csum,
>  				    int start, int offset)
>  {
>  	__sum16 *psum = (__sum16 *)(ptr + offset);

And this one could be split like the above

static __always_inline __wsum
remcsum_adjust(void *ptr, __wsum csum, int start, int offset)

Re: [PATCH net v3] net: Force inlining of checksum functions in net/checksum.h

[Masahiro Yamada]

On Fri, Feb 18, 2022 at 12:15 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Christophe Leroy
> > Sent: 17 February 2022 14:55
> >
> > Le 17/02/2022 à 15:50, Christophe Leroy a écrit :
> > > 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
> > >>> 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

RE: [PATCH net v3] net: Force inlining of checksum functions in net/checksum.h

[David Laight]

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)

Re: [PATCH net v3] net: Force inlining of checksum functions in net/checksum.h

[Masahiro Yamada]

On Fri, Feb 18, 2022 at 1:49 AM David Laight <David.Laight@aculab.com> wrote:
>
> 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'.


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)


--
Best Regards
Masahiro Yamada

Re: [PATCH net v3] net: Force inlining of checksum functions in net/checksum.h

[Segher Boessenkool]

On Fri, Feb 18, 2022 at 02:27:16AM +0900, Masahiro Yamada wrote:
> On Fri, Feb 18, 2022 at 1:49 AM David Laight <David.Laight@aculab.com> wrote:
> > 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!


Segher

Re: [PATCH net v3] net: Force inlining of checksum functions in net/checksum.h

[Masahiro Yamada]

On Fri, Feb 18, 2022 at 3:10 AM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Fri, Feb 18, 2022 at 02:27:16AM +0900, Masahiro Yamada wrote:
> > On Fri, Feb 18, 2022 at 1:49 AM David Laight <David.Laight@aculab.com> wrote:
> > > 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









>
>
> Segher







-- 
Best Regards
Masahiro Yamada

RE: [PATCH net v3] net: Force inlining of checksum functions in net/checksum.h

[David Laight]

From: Masahiro Yamada
> Sent: 17 February 2022 17:27
> 
> On Fri, Feb 18, 2022 at 1:49 AM David Laight <David.Laight@aculab.com> wrote:
> >
> > 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'.
> 
> 
> 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)

Re: [PATCH net v3] net: Force inlining of checksum functions in net/checksum.h

[Segher Boessenkool]

On Fri, Feb 18, 2022 at 10:35:48AM +0900, Masahiro Yamada wrote:
> On Fri, Feb 18, 2022 at 3:10 AM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> > On Fri, Feb 18, 2022 at 02:27:16AM +0900, Masahiro Yamada wrote:
> > > On Fri, Feb 18, 2022 at 1:49 AM David Laight <David.Laight@aculab.com> wrote:
> > > > 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

Re: [PATCH net v3] net: Force inlining of checksum functions in net/checksum.h

[Stephen Hemminger]

On Fri, 18 Feb 2022 06:12:37 -0600
Segher Boessenkool <segher@kernel.crashing.org> wrote:

> On Fri, Feb 18, 2022 at 10:35:48AM +0900, Masahiro Yamada wrote:
> > On Fri, Feb 18, 2022 at 3:10 AM Segher Boessenkool
> > <segher@kernel.crashing.org> wrote:  
> > > On Fri, Feb 18, 2022 at 02:27:16AM +0900, Masahiro Yamada wrote:  
> > > > On Fri, Feb 18, 2022 at 1:49 AM David Laight <David.Laight@aculab.com> wrote:  
> > > > > 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.

Re: [PATCH net v3] net: Force inlining of checksum functions in net/checksum.h

[Segher Boessenkool]

On Fri, Feb 18, 2022 at 08:29:20AM -0800, Stephen Hemminger wrote:
> On Fri, 18 Feb 2022 06:12:37 -0600
> Segher Boessenkool <segher@kernel.crashing.org> wrote:
> > On Fri, Feb 18, 2022 at 10:35:48AM +0900, Masahiro Yamada wrote:
> > > On Fri, Feb 18, 2022 at 3:10 AM Segher Boessenkool
> > > <segher@kernel.crashing.org> wrote:  
> > > > On Fri, Feb 18, 2022 at 02:27:16AM +0900, Masahiro Yamada wrote:  
> > > > > On Fri, Feb 18, 2022 at 1:49 AM David Laight <David.Laight@aculab.com> wrote:  
> > > > > > 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