From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754519AbcK1IvT (ORCPT ); Mon, 28 Nov 2016 03:51:19 -0500 Received: from mail-wm0-f66.google.com ([74.125.82.66]:34416 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754361AbcK1IvH (ORCPT ); Mon, 28 Nov 2016 03:51:07 -0500 Date: Mon, 28 Nov 2016 09:51:02 +0100 From: Ingo Molnar To: Thomas Gleixner Cc: peterz@infradead.org, tim.c.chen@linux.intel.com, hpa@zytor.com, srinivas.pandruvada@linux.intel.com, linux-kernel@vger.kernel.org, linux-tip-commits@vger.kernel.org Subject: Re: [tip:x86/core] x86: Enable Intel Turbo Boost Max Technology 3.0 Message-ID: <20161128085102.GA17652@gmail.com> References: <20161125081946.GA24513@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Thomas Gleixner wrote: > On Fri, 25 Nov 2016, Ingo Molnar wrote: > > > > > * tip-bot for Tim Chen wrote: > > > > > Commit-ID: 5e76b2ab36b40ca33023e78725bdc69eafd63134 > > > Gitweb: http://git.kernel.org/tip/5e76b2ab36b40ca33023e78725bdc69eafd63134 > > > Author: Tim Chen > > > AuthorDate: Tue, 22 Nov 2016 12:23:55 -0800 > > > Committer: Thomas Gleixner > > > CommitDate: Thu, 24 Nov 2016 20:44:19 +0100 > > > > > > x86: Enable Intel Turbo Boost Max Technology 3.0 > > > > This patch doesn't build: > > > > Note that this patch has to be redone anyway, as it won't even build: > > The branch where I merged it to builds fine. Indeed you are right - asm/mutex.h is gone in locking/core, so this is a semantic merge conflict, not a build failure. > Though, yes I missed the asm/mutex.h include which obviously should be > linux/mutex.h > > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > +#include > > > > arch/x86/kernel/itmt.c:26:23: fatal error: asm/mutex.h: No such file or directory > > > > > +config SCHED_ITMT > > > + bool "Intel Turbo Boost Max Technology (ITMT) scheduler support" > > > + depends on SCHED_MC && CPU_SUP_INTEL && X86_INTEL_PSTATE > > > + ---help--- > > > + ITMT enabled scheduler support improves the CPU scheduler's decision > > > + to move tasks to cpu core that can be boosted to a higher frequency > > > + than others. It will have better performance at a cost of slightly > > > + increased overhead in task migrations. If unsure say N here. > > > > Argh, so the 'itmt' name really sucks as well - could we please make it something > > more obvious - like SCHED_INTEL_TURBO or so - and similarly rename the file as > > well? > > > > The sched_intel_turbo.c file could thus host all things related to scheduler > > support of turbo frequencies - it shouldn't be named after the Intel acronym of > > the day... > > It would be nice to come up with such nitpicks during review. This thing went > through 8 iterations, but nothing came up and I didn't mind the itmt naming. Yeah, so I had to NAK an early iteration and didn't get around to doing a really detailed review yet - and after (falsely) thinking it had a build failure I got overly worked up about the bad naming: my bad and apologies! So the code looks good to me but the naming still sucks a bit - I'm fine with having the commits re-merged as-is and renaming the Kconfig variable to something more expressive: I've done this in tip:sched/core and have fixed the asm/mutex.h thing as well. Wrt. improving the naming: Firstly, popular tech news has coined the 'Turbo Boost Max' technology 'TBM' (TBM2 and TBM3) as the natural acronym of the Intel feature - not 'ITMT'. So to anyone except people well aware of Intel acronyms the term 'ITMT' will be pretty meaningless. Does something more generic like SCHED_MC_PRIO (as an extension to SCHED_MC) work with everyone? Intel Turbo Max 3.0 is the current (only) implementation of it, but I don't think the technology will stop at that stage as dies are getting larger but thinner. I also think the Kconfig text is somewhat misleading and the default-disabled status is counterproductive: +config SCHED_ITMT + bool "Intel Turbo Boost Max Technology (ITMT) scheduler support" + depends on SCHED_MC && CPU_SUP_INTEL && X86_INTEL_PSTATE + ---help--- + ITMT enabled scheduler support improves the CPU scheduler's decision + to move tasks to cpu core that can be boosted to a higher frequency + than others. It will have better performance at a cost of slightly + increased overhead in task migrations. If unsure say N here. ... the extra cost of smarter CPU selection is IMHO overwhelmed by the negative effects of not knowing about core frequency ordering, on most workloads. A better default would be default-y I believe (that is what we do for CPU hardware enablement typically), and a better description would be something like: +config SCHED_MC_PRIO + bool "CPU core priorities scheduler support" + depends on SCHED_MC && CPU_SUP_INTEL && X86_INTEL_PSTATE + default y + ---help--- + Intel Turbo Boost Max 3.0 enabled CPUs have a core ordering determined at + manufacturing time, which allows certain cores to reach higher turbo + frequencies (when running single threaded workloads) than others. + + Enabling this kernel feature teaches the scheduler about the TBM3 priority + order of the CPU cores and adjusts the scheduler's CPU selection logic + accordingly, so that higher overall system performance can be achieved. + + This feature will have no effect on CPUs without this feature. + + If unsure say Y here. If/when other architectures make use of this the Kconfig entry can be moved into the scheduler Kconfig - but for the time being it can stay in arch/x86/. Another variant would be to eliminate the Kconfig option altogether and make it a natural feature of SCHED_MC (like it is in the core scheduler). Thanks, Ingo