From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753627AbcK1RgJ (ORCPT ); Mon, 28 Nov 2016 12:36:09 -0500 Received: from mga06.intel.com ([134.134.136.31]:45630 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752128AbcK1RgA (ORCPT ); Mon, 28 Nov 2016 12:36:00 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,564,1473145200"; d="scan'208";a="1065271179" Message-ID: <1480354558.3064.28.camel@linux.intel.com> Subject: Re: [tip:x86/core] x86: Enable Intel Turbo Boost Max Technology 3.0 From: Tim Chen To: Ingo Molnar , Thomas Gleixner Cc: 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" Date: Mon, 28 Nov 2016 09:35:58 -0800 In-Reply-To: <20161128085102.GA17652@gmail.com> References: <20161125081946.GA24513@gmail.com> <20161128085102.GA17652@gmail.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.5.2 (3.18.5.2-1.fc23) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. Thanks. Tim