div_u64/do_div stack size usage, was Re: [v3] block: Removed a warning while compiling with a cross compiler for parisc

div_u64/do_div stack size usage, was Re: [v3] block: Removed a warning while compiling with a cross compiler for parisc

[Christoph Hellwig]

On Tue, Jul 06, 2021 at 05:30:54PM +0200, Abd-Alrhman Masalkhi wrote:
> Thank you for your comment, the div_u64 function is called 5 times
> inside diskstats_show function, so I have made a test case; I have
> replaced one call with a constant number then I have compiled the 
> kernel, the result was instead of emitting "the frame size of 1656
> bytes is larger than 1280 bytes" warning, it has emitted "the frame
> size of 1328 bytes is larger than 1280 bytes" warning, so I came to the
> conclusion that each call to div_u64 will add 328 bytes to the stack
> frame of diskstats_show function, since it is an inlined function. so I
> thought it might be the solution that to preventing div_u64 to be
> inlined in diskstats_show function.

Adding a bunch of relevant parties to the CC list - any idea how we
can make the generic do_div / div_u64 not use up such gigantic amounts
of stack?

Re: div_u64/do_div stack size usage, was Re: [v3] block: Removed a warning while compiling with a cross compiler for parisc

[Arnd Bergmann]

On Tue, Jul 6, 2021 at 7:03 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, Jul 06, 2021 at 05:30:54PM +0200, Abd-Alrhman Masalkhi wrote:
> > Thank you for your comment, the div_u64 function is called 5 times
> > inside diskstats_show function, so I have made a test case; I have
> > replaced one call with a constant number then I have compiled the
> > kernel, the result was instead of emitting "the frame size of 1656
> > bytes is larger than 1280 bytes" warning, it has emitted "the frame
> > size of 1328 bytes is larger than 1280 bytes" warning, so I came to the
> > conclusion that each call to div_u64 will add 328 bytes to the stack
> > frame of diskstats_show function, since it is an inlined function. so I
> > thought it might be the solution that to preventing div_u64 to be
> > inlined in diskstats_show function.
>
> Adding a bunch of relevant parties to the CC list - any idea how we
> can make the generic do_div / div_u64 not use up such gigantic amounts
> of stack?

I've seen variations of this problem many times, though usually not
involving do_div().

My guess is that this is happening here because of a combination of
things, most of the time it doesn't get nearly as bad:

- parisc has larger stack frames than others
- ilog2() as used in __div64_const32() is somewhat unreliable, it may
  end up determining that its input is a __builtin_constant_p(), but then
  still produce code for the non-constant case when the caller is
  only partially inlined
- Some compiler options make the problem worse by increasing the
  pressure on the register allocator.
- Some compiler targets don't deal well with register pressure and
  use more stack slots than they really should.

If you have the .config file that triggers this and the exact compiler
version, I can have a closer look to narrow down which of these
are part of the problem for that particular file.

One thing we did on ARM OABI (which does not deal well with
64-bit math) was to turn off the inline version of __arch_xprod_64
and instead use an extern function for do_div().

       Arnd

Re: div_u64/do_div stack size usage, was Re: [v3] block: Removed a warning while compiling with a cross compiler for parisc

[Abd-Alrhman Masalkhi]

I have compiled the kernel with a cross compiler hppa-linux-gnu- (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0 and the conficuration was the default, Build for generic-32bit "generic-32bit_defconfig"

Abd-Arhman

Re: div_u64/do_div stack size usage, was Re: [v3] block: Removed a warning while compiling with a cross compiler for parisc

[Arnd Bergmann]

On Tue, Jul 6, 2021 at 10:59 PM Abd-Alrhman Masalkhi
<abd.masalkhi@gmail.com> wrote:
>
> I have compiled the kernel with a cross compiler hppa-linux-gnu- (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0 and the conficuration was the default, Build for generic-32bit "generic-32bit_defconfig"

Ok, thanks. I have reproduced this now with gcc-9.4.0 from
https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/.

I have also tried it with the other gcc versions and shown that it
happens with every older compiler as well, but it does not happen
with gcc-10 or gcc-11, which bring the frame size down to 164 or
172 bytes respectively. gcc-10 also fixes similar warnings for
net/ipv4/tcp_input.c, net/sunrpc/stats.c and lib/xxhash.c that
fly under the radar with the default warning level.

My first thought was this was a result of -finline-functions getting
enabled by default in gcc-10, but enabling it on gcc-9 did not
help here. It's likely that the gcc behavior was fixed in the process
of enabling the more aggressive inliner by default though.

I also tried building genhd.o for every architecture that has
gcc-9.4 support and did not find the problem anywhere except
on parisc.

Using CONFIG_CC_OPTIMIZE_FOR_SIZE did solve the
problem for me (frame size down to 164 bytes), but I could not
pinpoint a specific -f option that fixes it for -O2. Maybe we can
simply change the defconfig here? 32-bit parisc systems are
probably memory limited enough that building a -Os kernel
is a good idea anyway. 64-bit parisc already builds with -Os
but does not see the warning with -O2 either.

    Arnd

Re: div_u64/do_div stack size usage, was Re: [v3] block: Removed a warning while compiling with a cross compiler for parisc

[Helge Deller]

* Arnd Bergmann <arnd@arndb.de>:
> On Tue, Jul 6, 2021 at 10:59 PM Abd-Alrhman Masalkhi
> <abd.masalkhi@gmail.com> wrote:
> >
> > I have compiled the kernel with a cross compiler hppa-linux-gnu-
> > (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0 and the conficuration was the
> > default, Build for generic-32bit "generic-32bit_defconfig"
>
> Ok, thanks. I have reproduced this now with gcc-9.4.0 from
> https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/.
>
> I have also tried it with the other gcc versions and shown that it
> happens with every older compiler as well, but it does not happen
> with gcc-10 or gcc-11, which bring the frame size down to 164 or
> 172 bytes respectively. gcc-10 also fixes similar warnings for
> net/ipv4/tcp_input.c, net/sunrpc/stats.c and lib/xxhash.c that
> fly under the radar with the default warning level.
>
> My first thought was this was a result of -finline-functions getting
> enabled by default in gcc-10, but enabling it on gcc-9 did not
> help here. It's likely that the gcc behavior was fixed in the process
> of enabling the more aggressive inliner by default though.
>
> I also tried building genhd.o for every architecture that has
> gcc-9.4 support and did not find the problem anywhere except
> on parisc.
>
> Using CONFIG_CC_OPTIMIZE_FOR_SIZE did solve the
> problem for me (frame size down to 164 bytes), but I could not
> pinpoint a specific -f option that fixes it for -O2. Maybe we can
> simply change the defconfig here? 32-bit parisc systems are
> probably memory limited enough that building a -Os kernel
> is a good idea anyway. 64-bit parisc already builds with -Os
> but does not see the warning with -O2 either.

I agree that the simplest solution is to increase the default value for
parisc here.
On parisc we have a 32k stack (either 1x32k or 2x16k when using IRQ
stacks). I increased the default value to 1280 in 2017, but as can be
seen here this isn't sufficient. Either way, we have an active runtime
check for stack overflows which has never triggered yet, so let's just
remove the compiler warning by increasing the value to 2048. Patch is
below.

Helge

---

[PATCH] parisc: Increase gcc stack frame check to 2048 for 32- and 64-bit

parisc uses much bigger frames than other architectures, so increase the
stack frame check value to 2048 to avoid compiler warnings.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
Cc: Christoph Hellwig <hch@infradead.org>
Signed-off-by: Helge Deller <deller@gmx.de>

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 678c13967580..1d99c3194e18 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -335,9 +335,8 @@ config FRAME_WARN
 	int "Warn for stack frames larger than"
 	range 0 8192
 	default 2048 if GCC_PLUGIN_LATENT_ENTROPY
-	default 1280 if (!64BIT && PARISC)
-	default 1024 if (!64BIT && !PARISC)
-	default 2048 if 64BIT
+	default 1024 if !(64BIT || PARISC)
+	default 2048 if (64BIT || PARISC)
 	help
 	  Tell gcc to warn at build time for stack frames larger than this.
 	  Setting this too low will cause a lot of warnings.

Re: div_u64/do_div stack size usage, was Re: [v3] block: Removed a warning while compiling with a cross compiler for parisc

[Arnd Bergmann]

On Wed, Jul 7, 2021 at 3:36 PM Helge Deller <deller@gmx.de> wrote:
> * Arnd Bergmann <arnd@arndb.de>:
> > My first thought was this was a result of -finline-functions getting
> > enabled by default in gcc-10, but enabling it on gcc-9 did not
> > help here. It's likely that the gcc behavior was fixed in the process
> > of enabling the more aggressive inliner by default though.
> >
> > I also tried building genhd.o for every architecture that has
> > gcc-9.4 support and did not find the problem anywhere except
> > on parisc.
> >
> > Using CONFIG_CC_OPTIMIZE_FOR_SIZE did solve the
> > problem for me (frame size down to 164 bytes), but I could not
> > pinpoint a specific -f option that fixes it for -O2. Maybe we can
> > simply change the defconfig here? 32-bit parisc systems are
> > probably memory limited enough that building a -Os kernel
> > is a good idea anyway. 64-bit parisc already builds with -Os
> > but does not see the warning with -O2 either.
>
> I agree that the simplest solution is to increase the default value for
> parisc here.
> On parisc we have a 32k stack (either 1x32k or 2x16k when using IRQ
> stacks). I increased the default value to 1280 in 2017, but as can be
> seen here this isn't sufficient. Either way, we have an active runtime
> check for stack overflows which has never triggered yet, so let's just
> remove the compiler warning by increasing the value to 2048. Patch is
> below.
>
> [PATCH] parisc: Increase gcc stack frame check to 2048 for 32- and 64-bit
>
> parisc uses much bigger frames than other architectures, so increase the
> stack frame check value to 2048 to avoid compiler warnings.

I think setting it to 2048 is rather excessive, and it would make you miss
other real bugs. What I suggested was to change the defconfig to use
CONFIG_CC_OPTIMIZE_FOR_SIZE instead.

The reasoning for the 1280 byte limit on parisc is that it needs a few extra
bytes for its larger stack frames, and 1024 for the other 32-bit architectures
is only there because anything smaller warns for a handful of functions
that are fine-tuned to need slightly less than that, when the call chain
is predictable and using less would impact performance.

I actually think we should reduce the warning limit for 64-bit architectures
to 1280 bytes as well, but that triggers a couple of warnings that still
need to be resolved first. In almost all cases, a kernel function needing
more than 512 bytes is an indication of either a bug in the kernel, or
(rarely) in the compiler.

        Arnd

Re: div_u64/do_div stack size usage, was Re: [v3] block: Removed a warning while compiling with a cross compiler for parisc

[Helge Deller]

On 7/7/21 10:39 PM, Arnd Bergmann wrote:
> On Wed, Jul 7, 2021 at 3:36 PM Helge Deller <deller@gmx.de> wrote:
>> * Arnd Bergmann <arnd@arndb.de>:
>>> My first thought was this was a result of -finline-functions getting
>>> enabled by default in gcc-10, but enabling it on gcc-9 did not
>>> help here. It's likely that the gcc behavior was fixed in the process
>>> of enabling the more aggressive inliner by default though.
>>>
>>> I also tried building genhd.o for every architecture that has
>>> gcc-9.4 support and did not find the problem anywhere except
>>> on parisc.
>>>
>>> Using CONFIG_CC_OPTIMIZE_FOR_SIZE did solve the
>>> problem for me (frame size down to 164 bytes), but I could not
>>> pinpoint a specific -f option that fixes it for -O2. Maybe we can
>>> simply change the defconfig here? 32-bit parisc systems are
>>> probably memory limited enough that building a -Os kernel
>>> is a good idea anyway. 64-bit parisc already builds with -Os
>>> but does not see the warning with -O2 either.
>>
>> I agree that the simplest solution is to increase the default value for
>> parisc here.
>> On parisc we have a 32k stack (either 1x32k or 2x16k when using IRQ
>> stacks). I increased the default value to 1280 in 2017, but as can be
>> seen here this isn't sufficient. Either way, we have an active runtime
>> check for stack overflows which has never triggered yet, so let's just
>> remove the compiler warning by increasing the value to 2048. Patch is
>> below.
>>
>> [PATCH] parisc: Increase gcc stack frame check to 2048 for 32- and 64-bit
>>
>> parisc uses much bigger frames than other architectures, so increase the
>> stack frame check value to 2048 to avoid compiler warnings.
>
> I think setting it to 2048 is rather excessive,

Since parisc needs roughly twice the frame (and stack) size as x86,
2048 seemed logical since that's the double of what's used on x86.
Of course we can reduce it, e.g. to 1536.

> and it would make you miss other real bugs. What I suggested was to
> change the defconfig to use CONFIG_CC_OPTIMIZE_FOR_SIZE instead.

But then you still will see those warnings in case you choose to not
optize for size.

> The reasoning for the 1280 byte limit on parisc is that it needs a few extra
> bytes for its larger stack frames, and 1024 for the other 32-bit architectures
> is only there because anything smaller warns for a handful of functions
> that are fine-tuned to need slightly less than that, when the call chain
> is predictable and using less would impact performance.
>
> I actually think we should reduce the warning limit for 64-bit architectures
> to 1280 bytes as well, but that triggers a couple of warnings that still
> need to be resolved first. In almost all cases, a kernel function needing
> more than 512 bytes is an indication of either a bug in the kernel, or
> (rarely) in the compiler.

or bad coding, e.g. huge local variables ot too much nesting of local functions.

Helge

Re: div_u64/do_div stack size usage, was Re: [v3] block: Removed a warning while compiling with a cross compiler for parisc

[Arnd Bergmann]

On Thu, Jul 8, 2021 at 11:30 AM Helge Deller <deller@gmx.de> wrote:
> On 7/7/21 10:39 PM, Arnd Bergmann wrote:
> >> [PATCH] parisc: Increase gcc stack frame check to 2048 for 32- and 64-bit
> >>
> >> parisc uses much bigger frames than other architectures, so increase the
> >> stack frame check value to 2048 to avoid compiler warnings.
> >
> > I think setting it to 2048 is rather excessive,
>
> Since parisc needs roughly twice the frame (and stack) size as x86,
> 2048 seemed logical since that's the double of what's used on x86.
> Of course we can reduce it, e.g. to 1536.

But it doesn't use twice as much for large functions at all. The stack
frame for a small function is much larger, so you need a larger kernel
stack to allow for deely nested call chains, but the frame for single
function with large variables is only a bit larger as most of it is used up
by those variables.

> > and it would make you miss other real bugs. What I suggested was to
> > change the defconfig to use CONFIG_CC_OPTIMIZE_FOR_SIZE instead.
>
> But then you still will see those warnings in case you choose to not
> optize for size.

Right, and I would consider that a good thing: this warning is for a real
(though fairly harmless) bug that has already been fixed with newer
toolchains, so anyone that runs into the bug should probably see the
warning for it. Doubling the limit would effectively prevent similar bugs
from being noticed, and they could be in performance-critical code
or cause an actual stack overrun.

I can think of two other, more directed workarounds:

- change block/Makefile to add -Os to the cflags for this one file in
  known-broken configurations (parisc with old gcc and -O2),
  to be removed in a few years when gcc-10 becomes the minimum
  supported version

- add a warning that points to the gcc bug (if someone has a link)
  when building an affected configuration, and let users decide to
  either change their setup (using -Os or a newer compiler) or to
  ignore the warning.

> or bad coding, e.g. huge local variables

That's what I meant with 'kernel bug'.

       Arnd

Re: div_u64/do_div stack size usage, was Re: [v3] block: Removed a warning while compiling with a cross compiler for parisc

[John David Anglin]

On 2021-07-08 7:37 a.m., Arnd Bergmann wrote:
>>> I think setting it to 2048 is rather excessive,
>> Since parisc needs roughly twice the frame (and stack) size as x86,
>> 2048 seemed logical since that's the double of what's used on x86.
>> Of course we can reduce it, e.g. to 1536.
> But it doesn't use twice as much for large functions at all. The stack
> frame for a small function is much larger, so you need a larger kernel
> stack to allow for deely nested call chains, but the frame for single
> function with large variables is only a bit larger as most of it is used up
> by those variables.
Correct.  In the 32-bit target, the stack alignment is 64 bytes.  This is the main reason functions
with small stacks use more stack than on x86.  There's also the frame marker that needs to be
reserved.  In the 64-bit target, the stack alignment is 16 bytes.  However, the minimum allocation
is quite large because of frame marker, 8 call registers and the argument pointer slots.  If a function
uses a significant number of local variables, there shouldn't be much difference in stack size.

Dave

-- 
John David Anglin  dave.anglin@bell.net

Re: div_u64/do_div stack size usage, was Re: [v3] block: Removed a warning while compiling with a cross compiler for parisc

[John David Anglin]

On 2021-07-07 4:17 a.m., Arnd Bergmann wrote:
> I have also tried it with the other gcc versions and shown that it
> happens with every older compiler as well, but it does not happen
> with gcc-10 or gcc-11, which bring the frame size down to 164 or
> 172 bytes respectively. gcc-10 also fixes similar warnings for
> net/ipv4/tcp_input.c, net/sunrpc/stats.c and lib/xxhash.c that
> fly under the radar with the default warning level.
>
> My first thought was this was a result of -finline-functions getting
> enabled by default in gcc-10, but enabling it on gcc-9 did not
> help here. It's likely that the gcc behavior was fixed in the process
> of enabling the more aggressive inliner by default though.
A number of improvements were made to the calculation of RTX costs in gcc-10 and gcc-11.
These dramatically affect inlining and the compilation time for xxhash.c on parisc.  The problem
is pretty much fixed for the 32-bit target but more work is needed for the 64-bit target.

See:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87256

Dave

-- 
John David Anglin  dave.anglin@bell.net

Re: div_u64/do_div stack size usage, was Re: [v3] block: Removed a warning while compiling with a cross compiler for parisc

[Abd-Alrhman Masalkhi]

Sorry for late respond, I was at work. The problem was solved for me too,
after setting the CONFIG_CC_OPTIMIZE_FOR_SIZE, and I have went through the
gcc 9.4 manual to look for the -f option for -O2, it seems that all -f option
that we would not specify is already excluded with -Os. changing defconfig, it
seems for me a good idea.

Abd-Alrhman