Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1530496 > unrolled thread
| Started by | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| First post | 2016-11-25 20:20 +0100 |
| Last post | 2016-11-29 19:50 +0100 |
| Articles | 6 — 4 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [tip:x86/core] x86: Enable Intel Turbo Boost Max Technology 3.0 Thomas Gleixner <tglx@linutronix.de> - 2016-11-25 20:20 +0100
Re: [tip:x86/core] x86: Enable Intel Turbo Boost Max Technology 3.0 Ingo Molnar <mingo@kernel.org> - 2016-11-28 10:00 +0100
Re: [tip:x86/core] x86: Enable Intel Turbo Boost Max Technology 3.0 Tim Chen <tim.c.chen@linux.intel.com> - 2016-11-28 18:40 +0100
Re: [tip:x86/core] x86: Enable Intel Turbo Boost Max Technology 3.0 "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-11-29 00:30 +0100
Re: [tip:x86/core] x86: Enable Intel Turbo Boost Max Technology 3.0 Ingo Molnar <mingo@kernel.org> - 2016-11-29 08:20 +0100
Re: [tip:x86/core] x86: Enable Intel Turbo Boost Max Technology 3.0 Tim Chen <tim.c.chen@linux.intel.com> - 2016-11-29 19:50 +0100
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2016-11-25 20:20 +0100 |
| Subject | Re: [tip:x86/core] x86: Enable Intel Turbo Boost Max Technology 3.0 |
| Message-ID | <sHucq-4Bv-11@gated-at.bofh.it> |
On Fri, 25 Nov 2016, Ingo Molnar wrote: > > * tip-bot for Tim Chen <tipbot@zytor.com> wrote: > > > Commit-ID: 5e76b2ab36b40ca33023e78725bdc69eafd63134 > > Gitweb: http://git.kernel.org/tip/5e76b2ab36b40ca33023e78725bdc69eafd63134 > > Author: Tim Chen <tim.c.chen@linux.intel.com> > > AuthorDate: Tue, 22 Nov 2016 12:23:55 -0800 > > Committer: Thomas Gleixner <tglx@linutronix.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. 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. Thanks, tglx
[toc] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2016-11-28 10:00 +0100 |
| Subject | Re: [tip:x86/core] x86: Enable Intel Turbo Boost Max Technology 3.0 |
| Message-ID | <sIpX3-8hS-9@gated-at.bofh.it> |
| In reply to | #1530496 |
* Thomas Gleixner <tglx@linutronix.de> wrote: > On Fri, 25 Nov 2016, Ingo Molnar wrote: > > > > > * tip-bot for Tim Chen <tipbot@zytor.com> wrote: > > > > > Commit-ID: 5e76b2ab36b40ca33023e78725bdc69eafd63134 > > > Gitweb: http://git.kernel.org/tip/5e76b2ab36b40ca33023e78725bdc69eafd63134 > > > Author: Tim Chen <tim.c.chen@linux.intel.com> > > > AuthorDate: Tue, 22 Nov 2016 12:23:55 -0800 > > > Committer: Thomas Gleixner <tglx@linutronix.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
[toc] | [prev] | [next] | [standalone]
| From | Tim Chen <tim.c.chen@linux.intel.com> |
|---|---|
| Date | 2016-11-28 18:40 +0100 |
| Subject | Re: [tip:x86/core] x86: Enable Intel Turbo Boost Max Technology 3.0 |
| Message-ID | <sIy4h-55G-25@gated-at.bofh.it> |
| In reply to | #1531126 |
On Mon, 2016-11-28 at 09:51 +0100, Ingo Molnar wrote: > * Thomas Gleixner <tglx@linutronix.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. Thanks. Tim
[toc] | [prev] | [next] | [standalone]
| From | "Rafael J. Wysocki" <rjw@rjwysocki.net> |
|---|---|
| Date | 2016-11-29 00:30 +0100 |
| Subject | Re: [tip:x86/core] x86: Enable Intel Turbo Boost Max Technology 3.0 |
| Message-ID | <sIDx0-dD-11@gated-at.bofh.it> |
| In reply to | #1531478 |
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@linutronix.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
[toc] | [prev] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2016-11-29 08:20 +0100 |
| Subject | Re: [tip:x86/core] x86: Enable Intel Turbo Boost Max Technology 3.0 |
| Message-ID | <sIKRP-5c9-3@gated-at.bofh.it> |
| In reply to | #1531478 |
* Tim Chen <tim.c.chen@linux.intel.com> wrote: > > + 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. Ok, could you please send a delta patch on top of tip:sched/core that does this and the other improvements? Thanks, Ingo
[toc] | [prev] | [next] | [standalone]
| From | Tim Chen <tim.c.chen@linux.intel.com> |
|---|---|
| Date | 2016-11-29 19:50 +0100 |
| Subject | Re: [tip:x86/core] x86: Enable Intel Turbo Boost Max Technology 3.0 |
| Message-ID | <sIVDA-3IZ-39@gated-at.bofh.it> |
| In reply to | #1531961 |
On Tue, 2016-11-29 at 08:11 +0100, Ingo Molnar wrote: > * Tim Chen <tim.c.chen@linux.intel.com> wrote: > > > > > > > > > + 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. > Ok, could you please send a delta patch on top of tip:sched/core that does this > and the other improvements? > I have sent a delta patch in a separate mail for this change. Thanks. Tim
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web