Re: [PATCH 2/2] x86/asm/entry/32: Remove unnecessary optimization in stub32_clone

From: Ingo Molnar <mingo@kernel.org>
To: Josh Triplett <josh@joshtriplett.org>
Cc: Denys Vlasenko <dvlasenk@redhat.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Borislav Petkov <bp@alien8.de>, "H. Peter Anvin" <hpa@zytor.com>,
	Andy Lutomirski <luto@amacapital.net>,
	Oleg Nesterov <oleg@redhat.com>,
	Frederic Weisbecker <fweisbec@gmail.com>,
	Alexei Starovoitov <ast@plumgrid.com>,
	Will Drewry <wad@chromium.org>, Kees Cook <keescook@chromium.org>,
	x86@kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] x86/asm/entry/32: Remove unnecessary optimization in stub32_clone
Date: Fri, 5 Jun 2015 13:44:01 +0200	[thread overview]
Message-ID: <20150605114401.GA6305@gmail.com> (raw)
In-Reply-To: <20150604155813.GA7829@x>

* Josh Triplett <josh@joshtriplett.org> wrote:

> On Thu, Jun 04, 2015 at 12:07:31PM +0200, Denys Vlasenko wrote:
> > On 06/03/2015 06:38 PM, Josh Triplett wrote:
> > > On Wed, Jun 03, 2015 at 03:58:50PM +0200, Denys Vlasenko wrote:
> > >> Really swap arguments #4 and #5 in stub32_clone instead of "optimizing"
> > >> it into a move.
> > >>
> > >> Yes, tls_val is currently unused. Yes, on some CPUs XCHG is a little bit
> > >> more expensive than MOV. But a cycle or two on an expensive syscall like
> > >> clone() is way below noise floor, and this optimization is simply not worth
> > >> the obfuscation of logic.
> > > [...]
> > >> This is a resend.
> > >>
> > >> There was a patch by Josh Triplett
> > >> "x86: Opt into HAVE_COPY_THREAD_TLS, for both 32-bit and 64-bit"
> > >> sent on May 11,
> > >> which does the same thing as part of a bigger cleanup.
> > >> He was supportive of this patch because of comments.
> > >> He will simply have to drop one hunk from his patch.
> > > 
> > > Strictly speaking, nothing needs this until clone starts paying
> > > attention to its tls argument, which only happens in my cleanup series
> > > that includes this change.  So what's the purpose of driving this patch
> > > separately?
> > 
> > You wanted my comments in this patch to go in:
> > 
> > On 04/22/2015 07:10 PM, Josh Triplett wrote:
> > > I do think my two-patch HAVE_COPY_THREAD_TLS series should go in fixing
> > > this, but I'd like to see the final version of Denys' comment added on
> > > top of it (with an update to the type and name of the tls argument to
> > > match the changes to sys_clone).
> > 
> > If your patch will go in first, I'll send a patch adding only the comment.
> > 
> > Since for now your patch did not make it yet, I'm submitting
> > a patch which adds both a comment and the insn change.
> 
> Ah, I see.
> 
> My two-patch series is currently sitting in -mm; would you consider providing a 
> version of the patch that adds the comment for Andrew to apply on top of those?

So because -mm is based on linux-next, and linux-next includes the x86 tree, 
amongst them the x86/asm topic, the dependency is the other way around.

Unrelated to these changes to the clone stub there will be conflicts anyway due to 
larger reorganization in the x86 assembly code: so I've picked up Denys's two 
patches, thus your patch set in -mm will become a bit smaller in the end, because 
it can rely on having these changes applied already.

Thanks,

	Ingo