From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755922AbcK1X0J (ORCPT ); Mon, 28 Nov 2016 18:26:09 -0500 Received: from cloudserver094114.home.net.pl ([79.96.170.134]:52832 "EHLO cloudserver094114.home.net.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753405AbcK1XZ7 (ORCPT ); Mon, 28 Nov 2016 18:25:59 -0500 From: "Rafael J. Wysocki" To: Tim Chen Cc: Ingo Molnar , Thomas Gleixner , peterz@infradead.org, hpa@zytor.com, srinivas.pandruvada@linux.intel.com, linux-kernel@vger.kernel.org, linux-tip-commits@vger.kernel.org, "Rafael J. Wysocki" Subject: Re: [tip:x86/core] x86: Enable Intel Turbo Boost Max Technology 3.0 Date: Tue, 29 Nov 2016 00:22:27 +0100 Message-ID: <37034270.S8ghLvshMx@aspire.rjw.lan> User-Agent: KMail/4.14.10 (Linux/4.9.0-rc5+; KDE/4.14.9; x86_64; ; ) In-Reply-To: <1480354558.3064.28.camel@linux.intel.com> References: <20161128085102.GA17652@gmail.com> <1480354558.3064.28.camel@linux.intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Monday, November 28, 2016 09:35:58 AM Tim Chen wrote: > On Mon, 2016-11-28 at 09:51 +0100, Ingo Molnar wrote: > > * Thomas Gleixner wrote: > > > > > > > > > > > +#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). > > I am fine with renaming SCHED_ITMT to SCHED_MC_PRIO. Patch 7 and 8 that > Rafael merged into his tree also have SCHED_ITMT so they will need to > be updated if we renamed it. No, I haven't. They are in tip AFAICS. Thanks, Rafael