Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190416080335.GM7905@worktop.programming.kicks-ass.net>
In-Reply-To: <20190416080335.GM7905@worktop.programming.kicks-ass.net>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Tue, 16 Apr 2019 10:45:05 -0700
Message-ID: <CAHk-=whgXyXCy9vHY2hKJ9Pa8PnwdwKo9eOQ3jDbCs_FkdDvkQ@mail.gmail.com>
Subject: Re: [PATCH] x86/tlb: Revert: Align TLB invalidation info
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@kernel.org>, Borislav Petkov <bp@alien8.de>,
        Linux List Kernel Mailing <linux-kernel@vger.kernel.org>,
        Andy Lutomirski <luto@kernel.org>,
        Nadav Amit <namit@vmware.com>,
        Dave Hansen <dave.hansen@intel.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Apr 16, 2019 at 1:03 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> Using 320 bytes of stack space for a 40 byte structure is ludicrous and
> clearly not right.

Ack.

That said, I wish we didn't have these stack structures at all. Or at
least were more careful about them. For example, another case of this
struct on the stack looks really iffy too:

                struct flush_tlb_info info;
                info.start = start;
                info.end = end;
                on_each_cpu(do_kernel_range_flush, &info, 1);

note how it only initializes two of the fields, and leaves the others
entirely randomly initialized with garbage?

Yeah, yeah, "do_kernel_range_flush()" only uses those two fields, but
it still makes my skin crawl how we basically pass a largely
uninitialized structure and have other CPU's look at it.

And in another case we do have a nicely initialized structure

    void arch_tlbbatch_flush(struct arch_tlbflush_unmap_batch *batch)
    {
        struct flush_tlb_info info = {
                .mm = NULL,
                .start = 0UL,
                .end = TLB_FLUSH_ALL,
        };

but it looks like it shouldn't have been on the stack in the first
place, because as far as I can tell it's entirely constant, and it
should just be a "static const" structure initialized at compile time.

So as far as I can tell, we could do something like

-static void flush_tlb_func_local(void *info, enum tlb_flush_reason reason)
+static void flush_tlb_func_local(const void *info, enum
tlb_flush_reason reason)
-       struct flush_tlb_info info = {
+       static const struct flush_tlb_info info = {

for that case.

End result: it looks like we have three of these stack things, and all
three had something odd in them.

So very much Ack on that patch, but maybe we could do a bit more cleanup here?

                   Linus