* [cryptodev:master 130/134] aes_generic.c:undefined reference to `_restgpr_31_x'
@ 2018-01-12 14:11 kbuild test robot
2018-01-12 14:55 ` Arnd Bergmann
0 siblings, 1 reply; 11+ messages in thread
From: kbuild test robot @ 2018-01-12 14:11 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: kbuild-all, linux-crypto, Herbert Xu
[-- Attachment #1: Type: text/plain, Size: 993 bytes --]
tree: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
head: b40fa82cd6138350f723aa47b37e3e3e80906b40
commit: 148b974deea927f5dbb6c468af2707b488bfa2de [130/134] crypto: aes-generic - build with -Os on gcc-7+
config: powerpc-linkstation_defconfig (attached as .config)
compiler: powerpc-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 148b974deea927f5dbb6c468af2707b488bfa2de
# save the attached .config to linux build tree
make.cross ARCH=powerpc
All errors (new ones prefixed by >>):
crypto/aes_generic.o: In function `crypto_aes_set_key':
>> aes_generic.c:(.text+0x4e0): undefined reference to `_restgpr_31_x'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 18333 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [cryptodev:master 130/134] aes_generic.c:undefined reference to `_restgpr_31_x'
2018-01-12 14:11 [cryptodev:master 130/134] aes_generic.c:undefined reference to `_restgpr_31_x' kbuild test robot
@ 2018-01-12 14:55 ` Arnd Bergmann
2018-01-12 16:39 ` Segher Boessenkool
0 siblings, 1 reply; 11+ messages in thread
From: Arnd Bergmann @ 2018-01-12 14:55 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all, open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
Herbert Xu, linuxppc-dev
On Fri, Jan 12, 2018 at 3:11 PM, kbuild test robot
<fengguang.wu@intel.com> wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git master
> head: b40fa82cd6138350f723aa47b37e3e3e80906b40
> commit: 148b974deea927f5dbb6c468af2707b488bfa2de [130/134] crypto: aes-generic - build with -Os on gcc-7+
> config: powerpc-linkstation_defconfig (attached as .config)
> compiler: powerpc-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
> reproduce:
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> git checkout 148b974deea927f5dbb6c468af2707b488bfa2de
> # save the attached .config to linux build tree
> make.cross ARCH=powerpc
>
> All errors (new ones prefixed by >>):
>
> crypto/aes_generic.o: In function `crypto_aes_set_key':
>>> aes_generic.c:(.text+0x4e0): undefined reference to `_restgpr_31_x'
adding linuxpcc-dev to Cc, maybe someone knows a way out of this.
It appears related to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=43810
but I don't know what _restgpr_31_x actually does, why it's not provided
by the kernel or why the aes_generic implementation needs this on
powerpc when built with 'gcc -Os'. FWIW, the -Os change was needed
to work around a possible kernel stack overflow that can happen with
gcc-7.2, see https://patchwork.kernel.org/patch/10143607/ and
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83356
Arnd
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [cryptodev:master 130/134] aes_generic.c:undefined reference to `_restgpr_31_x'
2018-01-12 14:55 ` Arnd Bergmann
@ 2018-01-12 16:39 ` Segher Boessenkool
2018-01-12 19:43 ` Arnd Bergmann
0 siblings, 1 reply; 11+ messages in thread
From: Segher Boessenkool @ 2018-01-12 16:39 UTC (permalink / raw)
To: Arnd Bergmann
Cc: kbuild test robot, Herbert Xu, linuxppc-dev, kbuild-all,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE
Hi!
On Fri, Jan 12, 2018 at 03:55:47PM +0100, Arnd Bergmann wrote:
> > crypto/aes_generic.o: In function `crypto_aes_set_key':
> >>> aes_generic.c:(.text+0x4e0): undefined reference to `_restgpr_31_x'
>
> adding linuxpcc-dev to Cc, maybe someone knows a way out of this.
> It appears related to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=43810
It is not.
> but I don't know what _restgpr_31_x actually does,
It restores GPR31 from stack, restores LR from stack, and returns
(_x is "exit").
> why it's not provided by the kernel
Because the kernel refuses to use libgcc. Let's, uh, not start that
again? :-)
> or why the aes_generic implementation needs this on
> powerpc when built with 'gcc -Os'. FWIW, the -Os change was needed
> to work around a possible kernel stack overflow that can happen with
> gcc-7.2, see https://patchwork.kernel.org/patch/10143607/ and
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83356
The _x versions are smaller but slower; that's why they are used with -Os.
Apparently nothing else was built with -Os (and the other needed flags)
before.
Segher
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [cryptodev:master 130/134] aes_generic.c:undefined reference to `_restgpr_31_x'
2018-01-12 16:39 ` Segher Boessenkool
@ 2018-01-12 19:43 ` Arnd Bergmann
2018-01-12 20:41 ` Segher Boessenkool
0 siblings, 1 reply; 11+ messages in thread
From: Arnd Bergmann @ 2018-01-12 19:43 UTC (permalink / raw)
To: Segher Boessenkool
Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
kbuild test robot, linuxppc-dev, Herbert Xu, kbuild-all
On Fri, Jan 12, 2018 at 5:39 PM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>> or why the aes_generic implementation needs this on
>> powerpc when built with 'gcc -Os'. FWIW, the -Os change was needed
>> to work around a possible kernel stack overflow that can happen with
>> gcc-7.2, see https://patchwork.kernel.org/patch/10143607/ and
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83356
>
> The _x versions are smaller but slower; that's why they are used with -Os.
> Apparently nothing else was built with -Os (and the other needed flags)
> before.
Ah, that explains it, the definition is in arch/powerpc/lib/crtsavres.S,
but inside of #ifdef CONFIG_CC_OPTIMIZE_FOR_SIZE.
We could theoretically work around it by turning that into
"#if defined(CONFIG_CC_OPTIMIZE_FOR_SIZE) ||
defined(CONFIG_CRYPTO_AES)", but that seems rather ugly.
My earlier patch already tried to be more specific, turning very
specific optimizations off rather than moving from -O2 to -Os,
but that turned out to lead to significantly worse performance,
where -Os improved performance slightly. Is there a way
to ask powerpc compilers to use mostly -Os but not the
specific thing that makes it link to _restgpr_31_x?
Arnd
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [cryptodev:master 130/134] aes_generic.c:undefined reference to `_restgpr_31_x'
2018-01-12 19:43 ` Arnd Bergmann
@ 2018-01-12 20:41 ` Segher Boessenkool
2018-01-12 21:29 ` Arnd Bergmann
0 siblings, 1 reply; 11+ messages in thread
From: Segher Boessenkool @ 2018-01-12 20:41 UTC (permalink / raw)
To: Arnd Bergmann
Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
kbuild test robot, linuxppc-dev, Herbert Xu, kbuild-all
On Fri, Jan 12, 2018 at 08:43:21PM +0100, Arnd Bergmann wrote:
> On Fri, Jan 12, 2018 at 5:39 PM, Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
>
> >> or why the aes_generic implementation needs this on
> >> powerpc when built with 'gcc -Os'. FWIW, the -Os change was needed
> >> to work around a possible kernel stack overflow that can happen with
> >> gcc-7.2, see https://patchwork.kernel.org/patch/10143607/ and
> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83356
> >
> > The _x versions are smaller but slower; that's why they are used with -Os.
> > Apparently nothing else was built with -Os (and the other needed flags)
> > before.
>
> Ah, that explains it, the definition is in arch/powerpc/lib/crtsavres.S,
> but inside of #ifdef CONFIG_CC_OPTIMIZE_FOR_SIZE.
Ah ok. Right.
> We could theoretically work around it by turning that into
> "#if defined(CONFIG_CC_OPTIMIZE_FOR_SIZE) ||
> defined(CONFIG_CRYPTO_AES)", but that seems rather ugly.
>
> My earlier patch already tried to be more specific, turning very
> specific optimizations off rather than moving from -O2 to -Os,
> but that turned out to lead to significantly worse performance,
> where -Os improved performance slightly. Is there a way
> to ask powerpc compilers to use mostly -Os but not the
> specific thing that makes it link to _restgpr_31_x?
There is no such thing, sorry. Would be very hard to implement, and
older compilers will never get it, so it won't help you anyway :-(
Maybe for now just enable it in crtsavres.S always, with a comment?
That -Os workaround is hopefully not going to live long either...
Segher
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [cryptodev:master 130/134] aes_generic.c:undefined reference to `_restgpr_31_x'
2018-01-12 20:41 ` Segher Boessenkool
@ 2018-01-12 21:29 ` Arnd Bergmann
2018-01-12 21:41 ` Segher Boessenkool
0 siblings, 1 reply; 11+ messages in thread
From: Arnd Bergmann @ 2018-01-12 21:29 UTC (permalink / raw)
To: Segher Boessenkool
Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
kbuild test robot, linuxppc-dev, Herbert Xu, kbuild-all
On Fri, Jan 12, 2018 at 9:41 PM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
> On Fri, Jan 12, 2018 at 08:43:21PM +0100, Arnd Bergmann wrote:
>> On Fri, Jan 12, 2018 at 5:39 PM, Segher Boessenkool
>> We could theoretically work around it by turning that into
>> "#if defined(CONFIG_CC_OPTIMIZE_FOR_SIZE) ||
>> defined(CONFIG_CRYPTO_AES)", but that seems rather ugly.
>>
>> My earlier patch already tried to be more specific, turning very
>> specific optimizations off rather than moving from -O2 to -Os,
>> but that turned out to lead to significantly worse performance,
>> where -Os improved performance slightly. Is there a way
>> to ask powerpc compilers to use mostly -Os but not the
>> specific thing that makes it link to _restgpr_31_x?
>
> There is no such thing, sorry. Would be very hard to implement, and
> older compilers will never get it, so it won't help you anyway :-(
We use -Os only for gcc-7.1 and higher, where it produces faster
code for AES and avoids running into
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83356
> Maybe for now just enable it in crtsavres.S always, with a comment?
> That -Os workaround is hopefully not going to live long either...
It depends on whether or how soon someone comes up with a
better fix for PR83356.
gcc-8.0.0 is currently not affected by it, so we could limit the
workaround (and the hack in crtsavres.S) to gcc-7-only.
Arnd
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [cryptodev:master 130/134] aes_generic.c:undefined reference to `_restgpr_31_x'
2018-01-12 21:29 ` Arnd Bergmann
@ 2018-01-12 21:41 ` Segher Boessenkool
2018-01-12 21:45 ` Arnd Bergmann
0 siblings, 1 reply; 11+ messages in thread
From: Segher Boessenkool @ 2018-01-12 21:41 UTC (permalink / raw)
To: Arnd Bergmann
Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
kbuild test robot, linuxppc-dev, Herbert Xu
On Fri, Jan 12, 2018 at 10:29:01PM +0100, Arnd Bergmann wrote:
> On Fri, Jan 12, 2018 at 9:41 PM, Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> > On Fri, Jan 12, 2018 at 08:43:21PM +0100, Arnd Bergmann wrote:
> >> On Fri, Jan 12, 2018 at 5:39 PM, Segher Boessenkool
>
> >> We could theoretically work around it by turning that into
> >> "#if defined(CONFIG_CC_OPTIMIZE_FOR_SIZE) ||
> >> defined(CONFIG_CRYPTO_AES)", but that seems rather ugly.
> >>
> >> My earlier patch already tried to be more specific, turning very
> >> specific optimizations off rather than moving from -O2 to -Os,
> >> but that turned out to lead to significantly worse performance,
> >> where -Os improved performance slightly. Is there a way
> >> to ask powerpc compilers to use mostly -Os but not the
> >> specific thing that makes it link to _restgpr_31_x?
> >
> > There is no such thing, sorry. Would be very hard to implement, and
> > older compilers will never get it, so it won't help you anyway :-(
>
> We use -Os only for gcc-7.1 and higher, where it produces faster
> code for AES and avoids running into
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83356
>
> > Maybe for now just enable it in crtsavres.S always, with a comment?
> > That -Os workaround is hopefully not going to live long either...
>
> It depends on whether or how soon someone comes up with a
> better fix for PR83356.
> gcc-8.0.0 is currently not affected by it, so we could limit the
> workaround (and the hack in crtsavres.S) to gcc-7-only.
I guess you could enable the _x routines whenever you use ubsan? Ubsan
will cause much bigger code growth than the handful of insns in those
routines?
Segher
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [cryptodev:master 130/134] aes_generic.c:undefined reference to `_restgpr_31_x'
2018-01-12 21:41 ` Segher Boessenkool
@ 2018-01-12 21:45 ` Arnd Bergmann
2018-01-12 22:10 ` Segher Boessenkool
0 siblings, 1 reply; 11+ messages in thread
From: Arnd Bergmann @ 2018-01-12 21:45 UTC (permalink / raw)
To: Segher Boessenkool
Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
kbuild test robot, linuxppc-dev, Herbert Xu
On Fri, Jan 12, 2018 at 10:41 PM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
> On Fri, Jan 12, 2018 at 10:29:01PM +0100, Arnd Bergmann wrote:
>> On Fri, Jan 12, 2018 at 9:41 PM, Segher Boessenkool
>> <segher@kernel.crashing.org> wrote:
>> > On Fri, Jan 12, 2018 at 08:43:21PM +0100, Arnd Bergmann wrote:
>> >> On Fri, Jan 12, 2018 at 5:39 PM, Segher Boessenkool
>>
>> >> We could theoretically work around it by turning that into
>> >> "#if defined(CONFIG_CC_OPTIMIZE_FOR_SIZE) ||
>> >> defined(CONFIG_CRYPTO_AES)", but that seems rather ugly.
>> >>
>> >> My earlier patch already tried to be more specific, turning very
>> >> specific optimizations off rather than moving from -O2 to -Os,
>> >> but that turned out to lead to significantly worse performance,
>> >> where -Os improved performance slightly. Is there a way
>> >> to ask powerpc compilers to use mostly -Os but not the
>> >> specific thing that makes it link to _restgpr_31_x?
>> >
>> > There is no such thing, sorry. Would be very hard to implement, and
>> > older compilers will never get it, so it won't help you anyway :-(
>>
>> We use -Os only for gcc-7.1 and higher, where it produces faster
>> code for AES and avoids running into
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83356
>>
>> > Maybe for now just enable it in crtsavres.S always, with a comment?
>> > That -Os workaround is hopefully not going to live long either...
>>
>> It depends on whether or how soon someone comes up with a
>> better fix for PR83356.
>> gcc-8.0.0 is currently not affected by it, so we could limit the
>> workaround (and the hack in crtsavres.S) to gcc-7-only.
>
> I guess you could enable the _x routines whenever you use ubsan? Ubsan
> will cause much bigger code growth than the handful of insns in those
> routines?
Right, that could work, too. My patch that Herbert merged intentionally
used -Os also for non-UBSAN builds because it turned out to
be much faster (see gcc PR83651), but we could revert that back
to the default and only use the -Os for UBSAN, essentially
addressing only PR83356 but not PR83651.
Arnd
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [cryptodev:master 130/134] aes_generic.c:undefined reference to `_restgpr_31_x'
2018-01-12 21:45 ` Arnd Bergmann
@ 2018-01-12 22:10 ` Segher Boessenkool
2018-01-14 21:40 ` Arnd Bergmann
0 siblings, 1 reply; 11+ messages in thread
From: Segher Boessenkool @ 2018-01-12 22:10 UTC (permalink / raw)
To: Arnd Bergmann
Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
kbuild test robot, linuxppc-dev, Herbert Xu
On Fri, Jan 12, 2018 at 10:45:31PM +0100, Arnd Bergmann wrote:
> > I guess you could enable the _x routines whenever you use ubsan? Ubsan
> > will cause much bigger code growth than the handful of insns in those
> > routines?
>
> Right, that could work, too. My patch that Herbert merged intentionally
> used -Os also for non-UBSAN builds because it turned out to
> be much faster (see gcc PR83651),
"Much"?
-Os is *slower* with 8.0, 5% faster with 7.2, 4% faster with 7.1,
slower with 7.0 and 6.3. Your numbers, #c1.
Anf this is the generic code of course, which is slow anyway (not to
mention insecure).
> but we could revert that back
> to the default and only use the -Os for UBSAN, essentially
> addressing only PR83356 but not PR83651.
Segher
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [cryptodev:master 130/134] aes_generic.c:undefined reference to `_restgpr_31_x'
2018-01-12 22:10 ` Segher Boessenkool
@ 2018-01-14 21:40 ` Arnd Bergmann
2018-01-15 0:21 ` Segher Boessenkool
0 siblings, 1 reply; 11+ messages in thread
From: Arnd Bergmann @ 2018-01-14 21:40 UTC (permalink / raw)
To: Segher Boessenkool
Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
kbuild test robot, linuxppc-dev, Herbert Xu
On Fri, Jan 12, 2018 at 11:10 PM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
> On Fri, Jan 12, 2018 at 10:45:31PM +0100, Arnd Bergmann wrote:
>> > I guess you could enable the _x routines whenever you use ubsan? Ubsan
>> > will cause much bigger code growth than the handful of insns in those
>> > routines?
>>
>> Right, that could work, too. My patch that Herbert merged intentionally
>> used -Os also for non-UBSAN builds because it turned out to
>> be much faster (see gcc PR83651),
>
> "Much"?
>
> -Os is *slower* with 8.0, 5% faster with 7.2, 4% faster with 7.1,
> slower with 7.0 and 6.3. Your numbers, #c1.
>
> Anf this is the generic code of course, which is slow anyway (not to
> mention insecure).
Right. I've done some more investigation anyway, starting over with the
analysis of the gcc options that change it. I've found now that turning
off '-fcode-hoisting' but leaving on the other options I had suspected
earlier (-O2 instead of -Os, -ftree-sra, -ftree-pre) also fixes the
stack problem, and appears to result in the best performance so
far. I need to rerun the whole test matrix, but that seems rather
promising, and the result may also help debug what's really happening.
Arnd
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [cryptodev:master 130/134] aes_generic.c:undefined reference to `_restgpr_31_x'
2018-01-14 21:40 ` Arnd Bergmann
@ 2018-01-15 0:21 ` Segher Boessenkool
0 siblings, 0 replies; 11+ messages in thread
From: Segher Boessenkool @ 2018-01-15 0:21 UTC (permalink / raw)
To: Arnd Bergmann
Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
kbuild test robot, linuxppc-dev, Herbert Xu
On Sun, Jan 14, 2018 at 10:40:36PM +0100, Arnd Bergmann wrote:
> Right. I've done some more investigation anyway, starting over with the
> analysis of the gcc options that change it. I've found now that turning
> off '-fcode-hoisting' but leaving on the other options I had suspected
> earlier (-O2 instead of -Os, -ftree-sra, -ftree-pre) also fixes the
> stack problem, and appears to result in the best performance so
> far.
Oh nice!
> I need to rerun the whole test matrix, but that seems rather
> promising, and the result may also help debug what's really happening.
-fcode-hoisting moves all expression evaluation to as early as possible;
for this AES code that means it will increase register pressure a lot,
causing a lot of spilling (well, that is my guess). If that is so, then
we need to dial down -fcode-hoisting a bit, maybe make it aware of
register pressure.
Glad you found a smoking gun,
Segher
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-01-15 0:21 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-12 14:11 [cryptodev:master 130/134] aes_generic.c:undefined reference to `_restgpr_31_x' kbuild test robot
2018-01-12 14:55 ` Arnd Bergmann
2018-01-12 16:39 ` Segher Boessenkool
2018-01-12 19:43 ` Arnd Bergmann
2018-01-12 20:41 ` Segher Boessenkool
2018-01-12 21:29 ` Arnd Bergmann
2018-01-12 21:41 ` Segher Boessenkool
2018-01-12 21:45 ` Arnd Bergmann
2018-01-12 22:10 ` Segher Boessenkool
2018-01-14 21:40 ` Arnd Bergmann
2018-01-15 0:21 ` Segher Boessenkool
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.