[PATCH] reduce large do_mount stack usage with noinlines

[PATCH] reduce large do_mount stack usage with noinlines

[Eric Sandeen]

do_mount() uses a whopping 616 bytes of stack on x86_64 in 
2.6.24-mm1, largely thanks to gcc inlining the various helper 
functions.

noinlining these can slim it down a lot; on my box this patch
gets it down to 168, which is mostly the struct nameidata nd;
left on the stack.

These functions are called only as do_mount() helpers;
none of them should be in any path that would see a performance
benefit from inlining...

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Index: linux-2.6.24-mm1/fs/namespace.c
===================================================================
--- linux-2.6.24-mm1.orig/fs/namespace.c
+++ linux-2.6.24-mm1/fs/namespace.c
@@ -1296,7 +1296,7 @@ out_unlock:
 /*
  * recursively change the type of the mountpoint.
  */
-static int do_change_type(struct nameidata *nd, int flag)
+static noinline int do_change_type(struct nameidata *nd, int flag)
 {
 	struct vfsmount *m, *mnt = nd->path.mnt;
 	int recurse = flag & MS_REC;
@@ -1320,7 +1320,7 @@ static int do_change_type(struct nameida
 /*
  * do loopback mount.
  */
-static int do_loopback(struct nameidata *nd, char *old_name, int recurse)
+static noinline int do_loopback(struct nameidata *nd, char *old_name, int recurse)
 {
 	struct nameidata old_nd;
 	struct vfsmount *mnt = NULL;
@@ -1387,7 +1387,7 @@ static int change_mount_flags(struct vfs
  * If you've mounted a non-root directory somewhere and want to do remount
  * on it - tough luck.
  */
-static int do_remount(struct nameidata *nd, int flags, int mnt_flags,
+static noinline int do_remount(struct nameidata *nd, int flags, int mnt_flags,
 		      void *data)
 {
 	int err;
@@ -1425,7 +1425,7 @@ static inline int tree_contains_unbindab
 	return 0;
 }
 
-static int do_move_mount(struct nameidata *nd, char *old_name)
+static noinline int do_move_mount(struct nameidata *nd, char *old_name)
 {
 	struct nameidata old_nd, parent_nd;
 	struct vfsmount *p;
@@ -1505,7 +1505,7 @@ out:
  * create a new mount for userspace and request it to be added into the
  * namespace's tree
  */
-static int do_new_mount(struct nameidata *nd, char *type, int flags,
+static noinline int do_new_mount(struct nameidata *nd, char *type, int flags,
 			int mnt_flags, char *name, void *data)
 {
 	struct vfsmount *mnt;

Re: [PATCH] reduce large do_mount stack usage with noinlines

[Andrew Morton]

On Wed, 06 Feb 2008 16:13:58 -0600
Eric Sandeen <sandeen@redhat.com> wrote:

> do_mount() uses a whopping 616 bytes of stack on x86_64 in 
> 2.6.24-mm1, largely thanks to gcc inlining the various helper 
> functions.

hm, sizeof(nameidata)=136 and I can see about three of them on the stack. 
Must have missed something.

> noinlining these can slim it down a lot; on my box this patch
> gets it down to 168, which is mostly the struct nameidata nd;
> left on the stack.
> 
> These functions are called only as do_mount() helpers;
> none of them should be in any path that would see a performance
> benefit from inlining...
> 

Does the patch actually help?  I mean, if a() calls b() and both use N
bytes of locals, our worst-case stack usage remains ~2N whether or not b()
was inlined in a()?  In fact, uninlining makes things a little worse due to
callframe stuff.

> -static int do_change_type(struct nameidata *nd, int flag)
> +static noinline int do_change_type(struct nameidata *nd, int flag)

There's no way for the reader to work out why this is here, so I do think
it should be commented somewhere.

Re: [PATCH] reduce large do_mount stack usage with noinlines

[Arjan van de Ven]

On Wed, 6 Feb 2008 14:34:57 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> 
> Does the patch actually help?  I mean, if a() calls b() and both use N
> bytes of locals, our worst-case stack usage remains ~2N whether or
> not b() was inlined in a()?  In fact, uninlining makes things a
> little worse due to callframe stuff.

it gets interesting at the three-way..
if a() calls b() and then calls c(), and they all use N,
the total usage is now 3N not 2N.

(although current gcc is already somewhat smarter about this, and 3N might actually be 2N for some cases)

Re: [PATCH] reduce large do_mount stack usage with noinlines

[Eric Sandeen]

Andrew Morton wrote:

> Does the patch actually help?  I mean, if a() calls b() and both use N
> bytes of locals, our worst-case stack usage remains ~2N whether or not b()
> was inlined in a()?  In fact, uninlining makes things a little worse due to
> callframe stuff.

I think it does.

[linux-2.6.24-mm1]$ make fs/namespace.o > /dev/null
[linux-2.6.24-mm1]$ objdump -d fs/namespace.o | scripts/checkstack.pl
x86_64 | grep do_mount
0x00002307 do_mount [namespace.o]:                      616
[linux-2.6.24-mm1]$ quilt push
Applying patch patches/do_mount_stack
patching file fs/namespace.c

Now at patch patches/do_mount_stack
[linux-2.6.24-mm1]$ make fs/namespace.o > /dev/null
[linux-2.6.24-mm1]$ objdump -d fs/namespace.o | scripts/checkstack.pl
x86_64 | grep do_mount
0x00002a8b do_mount [namespace.o]:                      168

So clearly that one function is reduced.  But it's more than that....

I guess the problem is a() calls b() or c() or d() or e() or f(), and
gcc adds up all that stack usage, or seems to, and we get more like 6N
regardless of the path taken.

For example, 2 of the helper functions, once un-inlined, are:

0x00001fd9 do_move_mount [namespace.o]:                 288
0x00001e94 do_loopback [namespace.o]:                   168

so it looks like we do carry that baggage even if we go the
do_new_mount() path for example.

>> -static int do_change_type(struct nameidata *nd, int flag)
>> +static noinline int do_change_type(struct nameidata *nd, int flag)
> 
> There's no way for the reader to work out why this is here, so I do think
> it should be commented somewhere.

Ok, good point, will resend... want a comment on each, or perhaps above
do_mount?  I suppose on each.

-Eric

Re: [PATCH] reduce large do_mount stack usage with noinlines

[Eric Sandeen]

Arjan van de Ven wrote:
> On Wed, 6 Feb 2008 14:34:57 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
>> Does the patch actually help?  I mean, if a() calls b() and both use N
>> bytes of locals, our worst-case stack usage remains ~2N whether or
>> not b() was inlined in a()?  In fact, uninlining makes things a
>> little worse due to callframe stuff.
> 
> it gets interesting at the three-way..
> if a() calls b() and then calls c(), and they all use N,
> the total usage is now 3N not 2N.

*nod*

It'd be nice if it could be max of (a,b,c) though.  Or maybe compilers
don't work that way.  :)

> (although current gcc is already somewhat smarter about this, and 3N might actually be 2N for some cases)

on x86, gcc 4.1.2, do_mount goes from 360 to 112 bytes w/ the patch I sent.

with gcc 4.3, it goes from 364 to 104.

-ERic

Re: [PATCH] reduce large do_mount stack usage with noinlines

[Eric Sandeen]

(updated with comments about the noinlines, and a fix for
 an >80 char line)

do_mount() uses a whopping 616 bytes of stack on x86_64 in 
2.6.24-mm1, largely thanks to gcc inlining the various helper 
functions.

noinlining these can slim it down a lot; on my box this patch
gets it down to 168, which is mostly the struct nameidata nd;
left on the stack.

These functions are called only as do_mount() helpers;
none of them should be in any path that would see a performance
benefit from inlining...

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Index: linux-2.6.24-mm1/fs/namespace.c
===================================================================
--- linux-2.6.24-mm1.orig/fs/namespace.c
+++ linux-2.6.24-mm1/fs/namespace.c
@@ -1295,8 +1295,9 @@ out_unlock:
 
 /*
  * recursively change the type of the mountpoint.
+ * noinline this do_mount helper to save do_mount stack space.
  */
-static int do_change_type(struct nameidata *nd, int flag)
+static noinline int do_change_type(struct nameidata *nd, int flag)
 {
 	struct vfsmount *m, *mnt = nd->path.mnt;
 	int recurse = flag & MS_REC;
@@ -1319,8 +1320,10 @@ static int do_change_type(struct nameida
 
 /*
  * do loopback mount.
+ * noinline this do_mount helper to save do_mount stack space.
  */
-static int do_loopback(struct nameidata *nd, char *old_name, int recurse)
+static noinline int do_loopback(struct nameidata *nd, char *old_name,
+				int recurse)
 {
 	struct nameidata old_nd;
 	struct vfsmount *mnt = NULL;
@@ -1386,8 +1389,9 @@ static int change_mount_flags(struct vfs
  * change filesystem flags. dir should be a physical root of filesystem.
  * If you've mounted a non-root directory somewhere and want to do remount
  * on it - tough luck.
+ * noinline this do_mount helper to save do_mount stack space.
  */
-static int do_remount(struct nameidata *nd, int flags, int mnt_flags,
+static noinline int do_remount(struct nameidata *nd, int flags, int mnt_flags,
 		      void *data)
 {
 	int err;
@@ -1425,7 +1429,10 @@ static inline int tree_contains_unbindab
 	return 0;
 }
 
-static int do_move_mount(struct nameidata *nd, char *old_name)
+/*
+ * noinline this do_mount helper to save do_mount stack space.
+ */
+static noinline int do_move_mount(struct nameidata *nd, char *old_name)
 {
 	struct nameidata old_nd, parent_nd;
 	struct vfsmount *p;
@@ -1504,8 +1511,9 @@ out:
 /*
  * create a new mount for userspace and request it to be added into the
  * namespace's tree
+ * noinline this do_mount helper to save do_mount stack space.
  */
-static int do_new_mount(struct nameidata *nd, char *type, int flags,
+static noinline int do_new_mount(struct nameidata *nd, char *type, int flags,
 			int mnt_flags, char *name, void *data)
 {
 	struct vfsmount *mnt;

Re: [PATCH] reduce large do_mount stack usage with noinlines

[Andrew Morton]

On Wed, 06 Feb 2008 17:11:38 -0600
Eric Sandeen <sandeen@redhat.com> wrote:

>  /*
>   * recursively change the type of the mountpoint.
> + * noinline this do_mount helper to save do_mount stack space.
>   */
> -static int do_change_type(struct nameidata *nd, int flag)
> +static noinline int do_change_type(struct nameidata *nd, int flag)

What we could do here is defined a new noinline_because_of_stack_suckiness
and use that.  Reasons:

- self-documenting, so we don't need to comment each site

- can be made a no-op for suitable __GNUC__ values if gcc ever fixes this

- our children we can go through and delete them all when nobody is using
  the offending gcc versions any more.

what thinkest thou?

Re: [PATCH] reduce large do_mount stack usage with noinlines

[Eric Sandeen]

Andrew Morton wrote:
> On Wed, 06 Feb 2008 17:11:38 -0600
> Eric Sandeen <sandeen@redhat.com> wrote:
> 
>>  /*
>>   * recursively change the type of the mountpoint.
>> + * noinline this do_mount helper to save do_mount stack space.
>>   */
>> -static int do_change_type(struct nameidata *nd, int flag)
>> +static noinline int do_change_type(struct nameidata *nd, int flag)
> 
> What we could do here is defined a new noinline_because_of_stack_suckiness
> and use that.  Reasons:
> 
> - self-documenting, so we don't need to comment each site
> 
> - can be made a no-op for suitable __GNUC__ values if gcc ever fixes this
> 
> - our children we can go through and delete them all when nobody is using
>   the offending gcc versions any more.
> 
> what thinkest thou?

Yes, sounds very good to me.  I'm sure there are more places which could
use this.

Or perhaps there are some magic flags to gcc so that it doesn't inline
so aggressively in situations like this?  IOW is it gcc breakage, or
design - but maybe with defaults that don't fit well in the limited
stack... (well, I suppose the "for N mutually exclusive helper functions
each with stack usage S, use about N*S stack when inlining them all" bit
probably isn't a feature.)

FWIW the XFS team finally just wrested control from GCC.
http://oss.sgi.com/archives/xfs/2006-11/msg00195.html

-Eric

Re: [PATCH] reduce large do_mount stack usage with noinlines

[Andrew Morton]

On Wed, 06 Feb 2008 17:34:39 -0600
Eric Sandeen <sandeen@redhat.com> wrote:

> Andrew Morton wrote:
> > On Wed, 06 Feb 2008 17:11:38 -0600
> > Eric Sandeen <sandeen@redhat.com> wrote:
> > 
> >>  /*
> >>   * recursively change the type of the mountpoint.
> >> + * noinline this do_mount helper to save do_mount stack space.
> >>   */
> >> -static int do_change_type(struct nameidata *nd, int flag)
> >> +static noinline int do_change_type(struct nameidata *nd, int flag)
> > 
> > What we could do here is defined a new noinline_because_of_stack_suckiness
> > and use that.  Reasons:
> > 
> > - self-documenting, so we don't need to comment each site
> > 
> > - can be made a no-op for suitable __GNUC__ values if gcc ever fixes this
> > 
> > - our children we can go through and delete them all when nobody is using
> >   the offending gcc versions any more.
> > 
> > what thinkest thou?
> 
> Yes, sounds very good to me.  I'm sure there are more places which could
> use this.
> 
> Or perhaps there are some magic flags to gcc so that it doesn't inline
> so aggressively in situations like this?  IOW is it gcc breakage, or
> design - but maybe with defaults that don't fit well in the limited
> stack... (well, I suppose the "for N mutually exclusive helper functions
> each with stack usage S, use about N*S stack when inlining them all" bit
> probably isn't a feature.)

The auto inlining is OK.  The problem is that when gcc handles this:

static inline foo()
{
	char a[10];
}

static inline bar()
{
	char a[10];
}

zot()
{
	foo();
	bar();
}

it will use 20 bytes of stack instead of using the same 10 bytes for both
"calls".  It doesn't need to do that, and other compilers avoid it, iirc.


Now, it _used_ to be the case that when presented with this:

foo()
{
	{
		char a[10];

		bar(a);
	}
	{
		char a[10];

		bar(a);
	}
}

gcc would also consume 20 bytes of stack.  But I see that this is fixed in
gcc-4.0.3.

These two things are equivalent and hopefully gcc will soon fix the
inlined-functions-use-too-much-stack thing.  Once that happens, we don't
need your patch.


> FWIW the XFS team finally just wrested control from GCC.
> http://oss.sgi.com/archives/xfs/2006-11/msg00195.html
> 

yup.

Re: [PATCH] reduce large do_mount stack usage with noinlines

[Eric Sandeen]

Andrew Morton wrote:
> On Wed, 06 Feb 2008 17:11:38 -0600
> Eric Sandeen <sandeen@redhat.com> wrote:
> 
>>  /*
>>   * recursively change the type of the mountpoint.
>> + * noinline this do_mount helper to save do_mount stack space.
>>   */
>> -static int do_change_type(struct nameidata *nd, int flag)
>> +static noinline int do_change_type(struct nameidata *nd, int flag)
> 
> What we could do here is defined a new noinline_because_of_stack_suckiness
> and use that.

Something like:

Index: linux-2.6.24-mm1/include/linux/compiler-gcc.h
===================================================================
--- linux-2.6.24-mm1.orig/include/linux/compiler-gcc.h
+++ linux-2.6.24-mm1/include/linux/compiler-gcc.h
@@ -53,3 +53,9 @@
 #define  noinline			__attribute__((noinline))
 #define __attribute_const__		__attribute__((__const__))
 #define __maybe_unused			__attribute__((unused))
+
+/*
+ * When gcc inlines multiple functions into a parent function,
+ * the stack space used sometimes increases excessively...
+ */
+#define noinline_stackspace	noinline


?

I couldn't think of a great name for it.  There are several noinline
users throughout the kernel with stackspace related comments, so if
desired, I could sprinkle this around.  I'm not very pleased with it
aesthetically though. :)

-Eric

Re: [PATCH] reduce large do_mount stack usage with noinlines

[Arjan van de Ven]

On Thu, 07 Feb 2008 17:08:19 -0600
Eric Sandeen <sandeen@redhat.com> wrote:
> 
> ?
> 
> I couldn't think of a great name for it.  There are several noinline
> users throughout the kernel with stackspace related comments, so if
> desired, I could sprinkle this around.  I'm not very pleased with it
> aesthetically though. :)


how about just __stackhog ?

-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] reduce large do_mount stack usage with noinlines

[Andrew Morton]

On Thu, 07 Feb 2008 17:08:19 -0600
Eric Sandeen <sandeen@redhat.com> wrote:

> Index: linux-2.6.24-mm1/include/linux/compiler-gcc.h
> ===================================================================
> --- linux-2.6.24-mm1.orig/include/linux/compiler-gcc.h
> +++ linux-2.6.24-mm1/include/linux/compiler-gcc.h
> @@ -53,3 +53,9 @@
>  #define  noinline			__attribute__((noinline))
>  #define __attribute_const__		__attribute__((__const__))
>  #define __maybe_unused			__attribute__((unused))
> +
> +/*
> + * When gcc inlines multiple functions into a parent function,
> + * the stack space used sometimes increases excessively...
> + */
> +#define noinline_stackspace	noinline
> 
> 
> ?
> 
> I couldn't think of a great name for it.  There are several noinline
> users throughout the kernel with stackspace related comments, so if
> desired, I could sprinkle this around.  I'm not very pleased with it
> aesthetically though. :)

I think it's fine.  People can go look at the definition site, read the
comment and come away happy.

Re: [PATCH] reduce large do_mount stack usage with noinlines

[Andi Kleen]

Andrew Morton <akpm@linux-foundation.org> writes:
>>   */
>> -static int do_change_type(struct nameidata *nd, int flag)
>> +static noinline int do_change_type(struct nameidata *nd, int flag)
>
> What we could do here is defined a new noinline_because_of_stack_suckiness
> and use that.  Reasons:
>
> - self-documenting, so we don't need to comment each site
>
> - can be made a no-op for suitable __GNUC__ values if gcc ever fixes this

In theory it should be already fixed; iirc Richard H. (cc'ed) added
code for this somewhere in 4.x. Don't quite remember which x, likely 
either 1 or 2.

e.g. if I do a quick test here on gcc 4.2 then it definitely
reuses stack slots between inlines. As you can see only ~100 bytes
are allocated, not ~200.

-Andi

% cat ts.c
static inline a(void)
{
	char x[100];
	extf(x);
}

static inline b(void)
{
	char y[100];
	extf(y);
}

f()
{
	a();
	b();		
}
% gcc -O2 -S ts.c
% cat ts.s
  ...
f:
.LFB4:
	pushq	%rbx
.LCFI0:
	xorl	%eax, %eax
	subq	$112, %rsp
.LCFI1:
	movq	%rsp, %rdi
	call	extf
	movq	%rsp, %rdi
	xorl	%eax, %eax
	call	extf
	addq	$112, %rsp
	popq	%rbx
	ret
...
%

Re: [PATCH] reduce large do_mount stack usage with noinlines

[Eric Sandeen]

Andi Kleen wrote:
> Andrew Morton <akpm@linux-foundation.org> writes:
>>>   */
>>> -static int do_change_type(struct nameidata *nd, int flag)
>>> +static noinline int do_change_type(struct nameidata *nd, int flag)
>> What we could do here is defined a new noinline_because_of_stack_suckiness
>> and use that.  Reasons:
>>
>> - self-documenting, so we don't need to comment each site
>>
>> - can be made a no-op for suitable __GNUC__ values if gcc ever fixes this
> 
> In theory it should be already fixed; iirc Richard H. (cc'ed) added
> code for this somewhere in 4.x. Don't quite remember which x, likely 
> either 1 or 2.
> 
> e.g. if I do a quick test here on gcc 4.2 then it definitely
> reuses stack slots between inlines. As you can see only ~100 bytes
> are allocated, not ~200.

On gcc 4.1.2 and 4.3 (fedora flavors) I don't see it re-used in
do_mount, though... *shrug*

-Eric

Re: [PATCH] reduce large do_mount stack usage with noinlines

[Al Viro]

On Fri, Feb 08, 2008 at 10:54:09AM -0600, Eric Sandeen wrote:

> On gcc 4.1.2 and 4.3 (fedora flavors) I don't see it re-used in
> do_mount, though... *shrug*

Frankly, a wrapper for path_lookup() that would take struct path *,
refuse to do LOOKUP_PARENT (i.e. guaranteed to have nothing stored
in nameidata other than ->mnt and ->dentry) and copied result of
lookup into passed struct path * would help a lot more.

Taking it to fs/namei.c would prevent inlining just fine and we'd
be left with pair of pointers in caller's stack frame instead of
full struct nameidata.  All users in fs/namespace.c can be trivially
converted to that and AFAICS it would save a lot more than we would
get from making everything in there not inlined.

I'll do that in fs/namei.c sanitizing series (opens as soon as ro-bind
stuff goes in).