Re: [PATCH] statmount: reduce runtime stack usage

[Arnd Bergmann]

On Wed, Dec 13, 2023, at 02:13, Ian Kent wrote:
> On 13/12/23 05:48, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@xxxxxxxx>
>>
>> prepare_kstatmount() constructs a copy of 'struct kstatmount' on the stack
>> and copies it into the local variable on the stack of its caller. Because
>> of the size of this structure, this ends up overflowing the limit for
>> a single function's stack frame when prepare_kstatmount() gets inlined
>> and both copies are on the same frame without the compiler being able
>> to collapse them into one:
>>
>> fs/namespace.c:4995:1: error: stack frame size (1536) exceeds limit (1024) in '__se_sys_statmount' [-Werror,-Wframe-larger-than]
>>   4995 | SYSCALL_DEFINE4(statmount, const struct mnt_id_req __user *, req,
>>
>> Mark the inner function as noinline_for_stack so the second copy is
>> freed before calling do_statmount() enters filesystem specific code.
>> The extra copy of the structure is a bit inefficient, but this
>> system call should not be performance critical.
>
> Are you sure this is not performance sensitive, or is the performance
> critical comment not related to the system call being called many times?
>
>
> It's going to be a while (if ever) before callers change there ways.
>
> Consider what happens when a bunch of mounts are being mounted.
>
>
> First there are a lot of events and making the getting of mount info.
> more efficient means more of those events get processed (itself an issue
> that's going to need notification sub-system improvement) resulting in
> the system call being called even more.

Ok, I'll send a v2 that is more efficent. I expected it to look uglier,
but I don't think it's actually that bad:

--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -4957,15 +4957,12 @@ static int prepare_kstatmount(struct kstatmount *ks, struct mnt_id_req *kreq,
        if (!access_ok(buf, bufsize))
                return -EFAULT;
 
-       *ks = (struct kstatmount){
-               .mask           = kreq->param,
-               .buf            = buf,
-               .bufsize        = bufsize,
-               .seq = {
-                       .size   = seq_size,
-                       .buf    = kvmalloc(seq_size, GFP_KERNEL_ACCOUNT),
-               },
-       };
+       memset(ks, 0, sizeof(*ks));
+       ks->mask        = kreq->param,
+       ks->buf         = buf,
+       ks->bufsize     = bufsize,
+       ks->seq.size    = seq_size,
+       ks->seq.buf     = kvmalloc(seq_size, GFP_KERNEL_ACCOUNT),
        if (!ks->seq.buf)
                return -ENOMEM;
        return 0;