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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3c2b682a7d804b5e8749428b50342c82@AcuMS.aculab.com>
Date:   Thu, 17 Feb 2022 15:15:11 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Christophe Leroy' <christophe.leroy@...roup.eu>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Ingo Molnar <mingo@...hat.com>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH net v3] net: Force inlining of checksum functions in
 net/checksum.h

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)