All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] block: Removed a warning while compiling with a cross compiler for parisc
@ 2021-07-06 11:19 Abd-Alrhman Masalkhi
  2021-07-06 13:59 ` Christoph Hellwig
  0 siblings, 1 reply; 14+ messages in thread
From: Abd-Alrhman Masalkhi @ 2021-07-06 11:19 UTC (permalink / raw)
  To: axboe
  Cc: linux-block, linux-kernel, dan.carpenter, kbuild, lkp,
	Abd-Alrhman Masalkhi

I have compiled the kernel with a cross compiler "hppa-linux-gnu-" v9.3.0
on x86-64 host machine. I got the following warning:

block/genhd.c: In function ‘diskstats_show’:
block/genhd.c:1227:1: warning: the frame size of 1688 bytes is larger
than 1280 bytes [-Wframe-larger-than=]
 1227  |  }

By Reduced the stack footprint, using new printf specifier to print the
bdevname and wrapping div_u64 function with a non-inline wrapper function,
the warning was not emitted anymore.

Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 block/genhd.c | 37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 79aa40b4c39c..0b091f572bc5 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1106,6 +1106,11 @@ const struct device_type disk_type = {
 };
 
 #ifdef CONFIG_PROC_FS
+static noinline u64 call_div_u64(u64 dividend, u32 divisor)
+{
+	return div_u64(dividend, divisor);
+}
+
 /*
  * aggregate disk stat collector.  Uses the same stats that the sysfs
  * entries do, above, but makes them available through one seq_file.
@@ -1117,7 +1122,6 @@ static int diskstats_show(struct seq_file *seqf, void *v)
 {
 	struct gendisk *gp = v;
 	struct block_device *hd;
-	char buf[BDEVNAME_SIZE];
 	unsigned int inflight;
 	struct disk_stats stat;
 	unsigned long idx;
@@ -1140,40 +1144,39 @@ static int diskstats_show(struct seq_file *seqf, void *v)
 		else
 			inflight = part_in_flight(hd);
 
-		seq_printf(seqf, "%4d %7d %s "
+		seq_printf(seqf, "%4d %7d %pg "
 			   "%lu %lu %lu %u "
 			   "%lu %lu %lu %u "
 			   "%u %u %u "
 			   "%lu %lu %lu %u "
 			   "%lu %u"
 			   "\n",
-			   MAJOR(hd->bd_dev), MINOR(hd->bd_dev),
-			   disk_name(gp, hd->bd_partno, buf),
+			   MAJOR(hd->bd_dev), MINOR(hd->bd_dev), hd,
 			   stat.ios[STAT_READ],
 			   stat.merges[STAT_READ],
 			   stat.sectors[STAT_READ],
-			   (unsigned int)div_u64(stat.nsecs[STAT_READ],
-							NSEC_PER_MSEC),
+			   (unsigned int)call_div_u64(stat.nsecs[STAT_READ],
+						      NSEC_PER_MSEC),
 			   stat.ios[STAT_WRITE],
 			   stat.merges[STAT_WRITE],
 			   stat.sectors[STAT_WRITE],
-			   (unsigned int)div_u64(stat.nsecs[STAT_WRITE],
-							NSEC_PER_MSEC),
+			   (unsigned int)call_div_u64(stat.nsecs[STAT_WRITE],
+						      NSEC_PER_MSEC),
 			   inflight,
 			   jiffies_to_msecs(stat.io_ticks),
-			   (unsigned int)div_u64(stat.nsecs[STAT_READ] +
-						 stat.nsecs[STAT_WRITE] +
-						 stat.nsecs[STAT_DISCARD] +
-						 stat.nsecs[STAT_FLUSH],
-							NSEC_PER_MSEC),
+			   (unsigned int)call_div_u64(stat.nsecs[STAT_READ] +
+						      stat.nsecs[STAT_WRITE] +
+						      stat.nsecs[STAT_DISCARD] +
+						      stat.nsecs[STAT_FLUSH],
+						      NSEC_PER_MSEC),
 			   stat.ios[STAT_DISCARD],
 			   stat.merges[STAT_DISCARD],
 			   stat.sectors[STAT_DISCARD],
-			   (unsigned int)div_u64(stat.nsecs[STAT_DISCARD],
-						 NSEC_PER_MSEC),
+			   (unsigned int)call_div_u64(stat.nsecs[STAT_DISCARD],
+						      NSEC_PER_MSEC),
 			   stat.ios[STAT_FLUSH],
-			   (unsigned int)div_u64(stat.nsecs[STAT_FLUSH],
-						 NSEC_PER_MSEC)
+			   (unsigned int)call_div_u64(stat.nsecs[STAT_FLUSH],
+						      NSEC_PER_MSEC)
 			);
 	}
 	rcu_read_unlock();
-- 
2.29.0.rc1.dirty


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v3] block: Removed a warning while compiling with a cross compiler for parisc
  2021-07-06 11:19 [PATCH v3] block: Removed a warning while compiling with a cross compiler for parisc Abd-Alrhman Masalkhi
@ 2021-07-06 13:59 ` Christoph Hellwig
  2021-07-06 15:30   ` [v3] " Abd-Alrhman Masalkhi
  0 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2021-07-06 13:59 UTC (permalink / raw)
  To: Abd-Alrhman Masalkhi
  Cc: axboe, linux-block, linux-kernel, dan.carpenter, kbuild, lkp

>  #ifdef CONFIG_PROC_FS
> +static noinline u64 call_div_u64(u64 dividend, u32 divisor)
> +{
> +	return div_u64(dividend, divisor);
> +}

Do you have any information on how much this reduces stack usage?
As it looks a little silly given the div_u64 implementation.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [v3] block: Removed a warning while compiling with a cross compiler for parisc
  2021-07-06 13:59 ` Christoph Hellwig
@ 2021-07-06 15:30   ` Abd-Alrhman Masalkhi
  2021-07-06 17:01     ` div_u64/do_div stack size usage, was " Christoph Hellwig
  0 siblings, 1 reply; 14+ messages in thread
From: Abd-Alrhman Masalkhi @ 2021-07-06 15:30 UTC (permalink / raw)
  To: hch; +Cc: axboe, linux-block, linux-kernel, dan.carpenter, kbuild, lkp

Hello Christoph,

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. I have used the new printf specifier to print the bdevname as you advised me to do and it has reduced the stack footprint, but the reduced amount was not enough to not emit the warning anymore, so I looked into div_u64... do you think the approach that I have taken is the proper fix?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* div_u64/do_div stack size usage, was Re: [v3] block: Removed a warning while compiling with a cross compiler for parisc
  2021-07-06 15:30   ` [v3] " Abd-Alrhman Masalkhi
@ 2021-07-06 17:01     ` Christoph Hellwig
  2021-07-06 17:35       ` Arnd Bergmann
  0 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2021-07-06 17:01 UTC (permalink / raw)
  To: Arnd Bergmann, Bernardo Innocenti,
	torvalds@linux-foundation.org Abd-Alrhman Masalkhi
  Cc: hch, axboe, linux-block, linux-kernel, dan.carpenter, linux-parisc

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?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: div_u64/do_div stack size usage, was Re: [v3] block: Removed a warning while compiling with a cross compiler for parisc
  2021-07-06 17:01     ` div_u64/do_div stack size usage, was " Christoph Hellwig
@ 2021-07-06 17:35       ` Arnd Bergmann
  2021-07-06 20:59         ` Abd-Alrhman Masalkhi
  0 siblings, 1 reply; 14+ messages in thread
From: Arnd Bergmann @ 2021-07-06 17:35 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Bernardo Innocenti,
	torvalds@linux-foundation.org Abd-Alrhman Masalkhi, Jens Axboe,
	linux-block, Linux Kernel Mailing List, Dan Carpenter,
	Parisc List

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: div_u64/do_div stack size usage, was Re: [v3] block: Removed a warning while compiling with a cross compiler for parisc
  2021-07-06 17:35       ` Arnd Bergmann
@ 2021-07-06 20:59         ` Abd-Alrhman Masalkhi
  2021-07-07  8:17           ` Arnd Bergmann
  0 siblings, 1 reply; 14+ messages in thread
From: Abd-Alrhman Masalkhi @ 2021-07-06 20:59 UTC (permalink / raw)
  To: arnd
  Cc: hch, axboe, bernie, linux-parisc, linux-block, linux-kernel,
	dan.carpenter

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: div_u64/do_div stack size usage, was Re: [v3] block: Removed a warning while compiling with a cross compiler for parisc
  2021-07-06 20:59         ` Abd-Alrhman Masalkhi
@ 2021-07-07  8:17           ` Arnd Bergmann
  2021-07-07 13:36             ` Helge Deller
                               ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Arnd Bergmann @ 2021-07-07  8:17 UTC (permalink / raw)
  To: Abd-Alrhman Masalkhi
  Cc: Christoph Hellwig, Jens Axboe, bernie, Parisc List, linux-block,
	Linux Kernel Mailing List, Dan Carpenter

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: div_u64/do_div stack size usage, was Re: [v3] block: Removed a warning while compiling with a cross compiler for parisc
  2021-07-07  8:17           ` Arnd Bergmann
@ 2021-07-07 13:36             ` Helge Deller
  2021-07-07 20:39               ` Arnd Bergmann
  2021-07-07 14:36             ` John David Anglin
  2021-07-07 15:30             ` Abd-Alrhman Masalkhi
  2 siblings, 1 reply; 14+ messages in thread
From: Helge Deller @ 2021-07-07 13:36 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Abd-Alrhman Masalkhi, Christoph Hellwig, Jens Axboe, bernie,
	Parisc List, linux-block, Linux Kernel Mailing List,
	Dan Carpenter

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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: div_u64/do_div stack size usage, was Re: [v3] block: Removed a warning while compiling with a cross compiler for parisc
  2021-07-07  8:17           ` Arnd Bergmann
  2021-07-07 13:36             ` Helge Deller
@ 2021-07-07 14:36             ` John David Anglin
  2021-07-07 15:30             ` Abd-Alrhman Masalkhi
  2 siblings, 0 replies; 14+ messages in thread
From: John David Anglin @ 2021-07-07 14:36 UTC (permalink / raw)
  To: Arnd Bergmann, Abd-Alrhman Masalkhi
  Cc: Christoph Hellwig, Jens Axboe, bernie, Parisc List, linux-block,
	Linux Kernel Mailing List, Dan Carpenter

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



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: div_u64/do_div stack size usage, was Re: [v3] block: Removed a warning while compiling with a cross compiler for parisc
  2021-07-07  8:17           ` Arnd Bergmann
  2021-07-07 13:36             ` Helge Deller
  2021-07-07 14:36             ` John David Anglin
@ 2021-07-07 15:30             ` Abd-Alrhman Masalkhi
  2 siblings, 0 replies; 14+ messages in thread
From: Abd-Alrhman Masalkhi @ 2021-07-07 15:30 UTC (permalink / raw)
  To: arnd
  Cc: hch, deller, dave.anglin, axboe, bernie, linux-parisc,
	linux-block, linux-kernel, dan.carpenter

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: div_u64/do_div stack size usage, was Re: [v3] block: Removed a warning while compiling with a cross compiler for parisc
  2021-07-07 13:36             ` Helge Deller
@ 2021-07-07 20:39               ` Arnd Bergmann
  2021-07-08  9:29                 ` Helge Deller
  0 siblings, 1 reply; 14+ messages in thread
From: Arnd Bergmann @ 2021-07-07 20:39 UTC (permalink / raw)
  To: Helge Deller
  Cc: Abd-Alrhman Masalkhi, Christoph Hellwig, Jens Axboe,
	Bernardo Innocenti, Parisc List, linux-block,
	Linux Kernel Mailing List, Dan Carpenter

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: div_u64/do_div stack size usage, was Re: [v3] block: Removed a warning while compiling with a cross compiler for parisc
  2021-07-07 20:39               ` Arnd Bergmann
@ 2021-07-08  9:29                 ` Helge Deller
  2021-07-08 11:37                   ` Arnd Bergmann
  0 siblings, 1 reply; 14+ messages in thread
From: Helge Deller @ 2021-07-08  9:29 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Abd-Alrhman Masalkhi, Christoph Hellwig, Jens Axboe,
	Bernardo Innocenti, Parisc List, linux-block,
	Linux Kernel Mailing List, Dan Carpenter

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: div_u64/do_div stack size usage, was Re: [v3] block: Removed a warning while compiling with a cross compiler for parisc
  2021-07-08  9:29                 ` Helge Deller
@ 2021-07-08 11:37                   ` Arnd Bergmann
  2021-07-08 15:01                     ` John David Anglin
  0 siblings, 1 reply; 14+ messages in thread
From: Arnd Bergmann @ 2021-07-08 11:37 UTC (permalink / raw)
  To: Helge Deller
  Cc: Abd-Alrhman Masalkhi, Christoph Hellwig, Jens Axboe,
	Bernardo Innocenti, Parisc List, linux-block,
	Linux Kernel Mailing List, Dan Carpenter

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: div_u64/do_div stack size usage, was Re: [v3] block: Removed a warning while compiling with a cross compiler for parisc
  2021-07-08 11:37                   ` Arnd Bergmann
@ 2021-07-08 15:01                     ` John David Anglin
  0 siblings, 0 replies; 14+ messages in thread
From: John David Anglin @ 2021-07-08 15:01 UTC (permalink / raw)
  To: Arnd Bergmann, Helge Deller
  Cc: Abd-Alrhman Masalkhi, Christoph Hellwig, Jens Axboe,
	Bernardo Innocenti, Parisc List, linux-block,
	Linux Kernel Mailing List, Dan Carpenter

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



^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-07-08 15:01 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-06 11:19 [PATCH v3] block: Removed a warning while compiling with a cross compiler for parisc Abd-Alrhman Masalkhi
2021-07-06 13:59 ` Christoph Hellwig
2021-07-06 15:30   ` [v3] " Abd-Alrhman Masalkhi
2021-07-06 17:01     ` div_u64/do_div stack size usage, was " Christoph Hellwig
2021-07-06 17:35       ` Arnd Bergmann
2021-07-06 20:59         ` Abd-Alrhman Masalkhi
2021-07-07  8:17           ` Arnd Bergmann
2021-07-07 13:36             ` Helge Deller
2021-07-07 20:39               ` Arnd Bergmann
2021-07-08  9:29                 ` Helge Deller
2021-07-08 11:37                   ` Arnd Bergmann
2021-07-08 15:01                     ` John David Anglin
2021-07-07 14:36             ` John David Anglin
2021-07-07 15:30             ` Abd-Alrhman Masalkhi

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.