Re: [PATCH V5 3/7] x86/entry: Move PUSH_AND_CLEAR_REGS out of error_entry()

From: Lai Jiangshan <jiangshanlai@gmail.com>
To: Borislav Petkov <bp@alien8.de>
Cc: LKML <linux-kernel@vger.kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Josh Poimboeuf <jpoimboe@redhat.com>,
	Andy Lutomirski <luto@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>, X86 ML <x86@kernel.org>,
	Lai Jiangshan <jiangshan.ljs@antgroup.com>,
	Ingo Molnar <mingo@redhat.com>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	"H. Peter Anvin" <hpa@zytor.com>
Subject: Re: [PATCH V5 3/7] x86/entry: Move PUSH_AND_CLEAR_REGS out of error_entry()
Date: Wed, 13 Apr 2022 11:48:21 +0800	[thread overview]
Message-ID: <CAJhGHyC7KUj=s+w+isBewdL7iyUyEm4h++rWRoz2zBc0zAD2gw@mail.gmail.com> (raw)
In-Reply-To: <YlWNIj198mKM3hMG@zn.tnic>

On Tue, Apr 12, 2022 at 10:31 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Tue, Apr 12, 2022 at 09:52:44PM +0800, Lai Jiangshan wrote:
> > The mapped size for the text is always 2M when the kernel is booted
> > since it is 2M-aligned.  So I don't think the size is a concern.
>
> This is not how this argumentation works: you add 0.01% size increase
> here, 0.01% slowdown there and it all soon adds up into a bloated
> kernel. Besides, nowadays the kernel runs a lot as a guest and guest
> kernel size does matter.

I agree with you.

I meant the real .entry.text size is unchanged.  No matter what
the complied .entry.text size is, 5.5k before the patch, or 8.2k
after the patch, the size of .entry.text in the kernel is always 2M.

But the overall size is enlarged which is a concern.

>
> So no, just because you want to turn error_entry() into C code and for
> that you'll add some bloat doesn't justify it in my book. All changes to
> core code need to be absolutely needed and justifiable.
>
> > For example, a heavy page fault and IPI or timer at the same time.
> > I'm not sure if it is a real case.
>
> I wouldn't be surprised if it shows in some perf profiles.
>
> > I'm Okay with asm_error_entry().  And also we can use an ASM function
> > containing PUSH_AND_CLEAR_REGS only.
>
> Just do the necessary and minimal thing - this is core code and in asm
> for that matter, so be conservative pls. This is not some random driver.
>

I will follow your instructions and add an extra ASM function.

And maybe add this to the changelog:

A function call is added for every interrupt or exception, but it
is a low-level asm function call and actually, only a "call" and a
"ret" instructions are added.  It will enable error_entry() to be
C code that can use inlined sync_regs() and save a C function call
which will reduce more instructions in return.

I'm also waiting for reviews for other patches and I'm not going to
send this new code too soon since I don't want to send a new revision
with only this patch updated.

Thanks
Lai