Date:   Tue, 29 Nov 2016 00:22:27 +0100
From:   "Rafael J. Wysocki" <rjw@...ysocki.net>
To:     Tim Chen <tim.c.chen@...ux.intel.com>
Cc:     Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>, peterz@...radead.org,
        hpa@...or.com, srinivas.pandruvada@...ux.intel.com,
        linux-kernel@...r.kernel.org, linux-tip-commits@...r.kernel.org,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>
Subject: Re: [tip:x86/core] x86: Enable Intel Turbo Boost Max Technology 3.0

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 <tglx@...utronix.de> 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.

No, I haven't.  They are in tip AFAICS.

Thanks,
Rafael

Powered by blists - more mailing lists