Re: [tip:x86/core] x86: Enable Intel Turbo Boost Max Technology 3.0

[Tim Chen]

On Mon, 2016-11-28 at 09:51 +0100, Ingo Molnar wrote:
> * Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> 
> >Â
> > 
> > > 
> > > > 
> > > > +#include <linux/sched.h>
> > > > +#include <linux/cpumask.h>
> > > > +#include <linux/cpuset.h>
> > > > +#include <asm/mutex.h>
> > > > +#include <linux/sched.h>
> > > > +#include <linux/sysctl.h>
> > > > +#include <linux/nodemask.h>
> > > 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