Date:   Mon, 28 Nov 2016 09:51:02 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     peterz@...radead.org, tim.c.chen@...ux.intel.com, hpa@...or.com,
        srinivas.pandruvada@...ux.intel.com, linux-kernel@...r.kernel.org,
        linux-tip-commits@...r.kernel.org
Subject: Re: [tip:x86/core] x86: Enable Intel Turbo Boost Max Technology 3.0


* Thomas Gleixner <tglx@...utronix.de> wrote:

> On Fri, 25 Nov 2016, Ingo Molnar wrote:
> 
> > 
> > * tip-bot for Tim Chen <tipbot@...or.com> wrote:
> > 
> > > Commit-ID:  5e76b2ab36b40ca33023e78725bdc69eafd63134
> > > Gitweb:     http://git.kernel.org/tip/5e76b2ab36b40ca33023e78725bdc69eafd63134
> > > Author:     Tim Chen <tim.c.chen@...ux.intel.com>
> > > AuthorDate: Tue, 22 Nov 2016 12:23:55 -0800
> > > Committer:  Thomas Gleixner <tglx@...utronix.de>
> > > 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 <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).

Thanks,

	Ingo

Powered by blists - more mailing lists