Re: [PATCH] kbuild, x86: revert macros in extended asm workarounds

From: Nadav Amit <namit@vmware.com>
To: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Borislav Petkov <bp@alien8.de>, "H . Peter Anvin" <hpa@zytor.com>,
	X86 ML <x86@kernel.org>, Richard Biener <rguenther@suse.de>,
	Logan Gunthorpe <logang@deltatee.com>,
	Sedat Dilek <sedat.dilek@gmail.com>,
	Segher Boessenkool <segher@kernel.crashing.org>,
	linux-arch <linux-arch@vger.kernel.org>,
	Arnd Bergmann <arnd@arndb.de>,
	Luc Van Oostenryck <luc.vanoostenryck@gmail.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	"virtualization@lists.linux-foundation.org" 
	<virtualization@lists.linux-foundation.org>,
	Michal Marek <michal.lkml@markovi.net>,
	"linux-sparse@vger.kernel.org" <linux-sparse@vger.kernel.org>,
	Alok Kataria <akataria@vmware.com>,
	Juergen Gross <jgross@suse.com>,
	Andy Lutomirski <luto@kernel.org>,
	Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>
Subject: Re: [PATCH] kbuild, x86: revert macros in extended asm workarounds
Date: Sun, 16 Dec 2018 02:33:39 +0000	[thread overview]
Message-ID: <07BE39B2-1F99-4AE4-97F3-0871A39C5E7D@vmware.com> (raw)
In-Reply-To: <CAK7LNASB=HvU8DwUQQkz_r3sY1DN8Vv-qfNa54-ZDXSpfvEYpg@mail.gmail.com>

> On Dec 14, 2018, at 4:51 PM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> 
> Hi Peter,
> 
> On Thu, Dec 13, 2018 at 7:53 PM Peter Zijlstra <peterz@infradead.org> wrote:
>> On Thu, Dec 13, 2018 at 06:17:41PM +0900, Masahiro Yamada wrote:
>>> Revert the following commits:
>>> 
>>> - 5bdcd510c2ac9efaf55c4cbd8d46421d8e2320cd
>>>  ("x86/jump-labels: Macrofy inline assembly code to work around GCC inlining bugs")
>>> 
>>> - d5a581d84ae6b8a4a740464b80d8d9cf1e7947b2
>>>  ("x86/cpufeature: Macrofy inline assembly code to work around GCC inlining bugs")
>>> 
>>> - 0474d5d9d2f7f3b11262f7bf87d0e7314ead9200.
>>>  ("x86/extable: Macrofy inline assembly code to work around GCC inlining bugs")
>>> 
>>> - 494b5168f2de009eb80f198f668da374295098dd.
>>>  ("x86/paravirt: Work around GCC inlining bugs when compiling paravirt ops")
>>> 
>>> - f81f8ad56fd1c7b99b2ed1c314527f7d9ac447c6.
>>>  ("x86/bug: Macrofy the BUG table section handling, to work around GCC inlining bugs")
>>> 
>>> - 77f48ec28e4ccff94d2e5f4260a83ac27a7f3099.
>>>  ("x86/alternatives: Macrofy lock prefixes to work around GCC inlining bugs")
>>> 
>>> - 9e1725b410594911cc5981b6c7b4cea4ec054ca8.
>>>  ("x86/refcount: Work around GCC inlining bug")
>>>  (Conflicts: arch/x86/include/asm/refcount.h)
>>> 
>>> - c06c4d8090513f2974dfdbed2ac98634357ac475.
>>>  ("x86/objtool: Use asm macros to work around GCC inlining bugs")
>>> 
>>> - 77b0bf55bc675233d22cd5df97605d516d64525e.
>>>  ("kbuild/Makefile: Prepare for using macros in inline assembly code to work around asm() related GCC inlining bugs")
>> 
>> I don't think we want to blindly revert all that. Some of them actually
>> made sense and did clean up things irrespective of the asm-inline issue.
>> 
>> In particular I like the jump-label one.
> 
> [1] The #error message is unnecessary.
> 
> [2] keep STATC_BRANCH_NOP/JMP instead of STATIC_JUMP_IF_TRUE/FALSE
> 
> 
> 
> In v2, I will make sure to not re-add [1].
> I am not sure about [2].
> 
> 
> Do you mean only [1],
> or both of them?
> 
> 
> 
>> The cpufeature one OTOh, yeah,
>> I'd love to get that reverted.
>> 
>> And as a note; the normal commit quoting style is:
>> 
>>  d5a581d84ae6 ("x86/cpufeature: Macrofy inline assembly code to work around GCC inlining bugs")
> 
> 
> OK. I will do so in v2.

I recommend to do the following for v2:

1. Run some static measurements (e.g., function sizes, number of function
symbols) to ensure that GCC works as it should. If possible, run small
performance evaluations. IIRC, I saw small but consistent performance
difference when I ran a loop with mprotect() that kept changing permissions.
This was due to PV MMU functions that caused inlining mess.

2. Break the patch into separate patches, based on the original patch-set
order (reversed). This is the common practice, which allows people to review
patches, perform bisections, and revert when needed.

3. Cc the relevant people who ack'd the original patches, e.g., Kees Cook,
who’s on top of the reference-counters and Linus, who proposed this
approach.

In general, I think that from the start it was clear that the motivation for
the patch-set is not just performance and also better code. For example, I
see no reason to revert the PV-changes or the lock-prefix changes that
improved the code readability.

Regards,
Nadav