Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1361808 > unrolled thread

[PATCH 0/3] idle, Honor Hardware Disabled States

Started byPrarit Bhargava <prarit@redhat.com>
First post2016-03-21 14:00 +0100
Last post2016-03-31 07:00 +0200
Articles 8 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] idle, Honor Hardware Disabled States Prarit Bhargava <prarit@redhat.com> - 2016-03-21 14:00 +0100
    [PATCH 1/3] idle, rename MSR_NHM_SNB_PKG_CST_CFG_CTL to MSR_PKG_CST_CONFIG_CONTROL Prarit Bhargava <prarit@redhat.com> - 2016-03-21 14:00 +0100
      Re: [PATCH 1/3] idle, rename MSR_NHM_SNB_PKG_CST_CFG_CTL to MSR_PKG_CST_CONFIG_CONTROL Len Brown <lenb@kernel.org> - 2016-03-23 21:00 +0100
    Re: [PATCH 0/3] idle, Honor Hardware Disabled States Len Brown <lenb@kernel.org> - 2016-03-23 21:10 +0100
      Re: [PATCH 0/3] idle, Honor Hardware Disabled States Prarit Bhargava <prarit@redhat.com> - 2016-03-24 01:00 +0100
        Re: [PATCH 0/3] idle, Honor Hardware Disabled States Len Brown <lenb@kernel.org> - 2016-03-24 23:00 +0100
          Re: [PATCH 0/3] idle, Honor Hardware Disabled States Prarit Bhargava <prarit@redhat.com> - 2016-03-28 16:50 +0200
            Re: [PATCH 0/3] idle, Honor Hardware Disabled States Len Brown <lenb@kernel.org> - 2016-03-31 07:00 +0200

#1361808 — [PATCH 0/3] idle, Honor Hardware Disabled States

FromPrarit Bhargava <prarit@redhat.com>
Date2016-03-21 14:00 +0100
Subject[PATCH 0/3] idle, Honor Hardware Disabled States
Message-ID<rf7rs-5J5-11@gated-at.bofh.it>
Some Intel processors have cstates limited by MSR_PKG_CST_CONFIG_CONTROL
(0xE2), which specifies the lowest processor-specific C-state code name
(consuming the least power).  The intel_idle driver does not query this MSR to
see if states are limited by hardware.  In the case of the i7-6700HQ the value
of bits 3:0 on the MSR are 0x6, meaning that C9-SKL (0x7) and C10-SKL (0x8)
should have been disabled.

This patch adds a limit field to the cpuidle_state struct to hold the
cstate encodings for each processor, and queries the values the values
to confirm support.

After applying this patchset the system now boots and reports that
some states have been disabled:

[root@dhcp40-125 ~]# dmesg | grep intel_idle
[    1.163367] intel_idle: MWAIT substates: 0x11142120
[    1.163368] intel_idle: v0.4 model 0x5E
[    1.163369] intel_idle: lapic_timer_reliable_states 0xffffffff
[    1.163370] intel_idle: state C9-SKL (0x7) is disabled.  Max Package limit is 0x6.
[    1.163371] intel_idle: state C10-SKL (0x8) is disabled.  Max Package limit is 0x6.

Cc: x86@kernel.org
Cc: Len Brown <lenb@kernel.org>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Dasaratharaman Chandramouli <dasaratharaman.chandramouli@intel.com>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Prarit Bhargava <prarit@redhat.com>

Prarit Bhargava (3):
  idle, rename MSR_NHM_SNB_PKG_CST_CFG_CTL to
    MSR_PKG_CST_CONFIG_CONTROL
  intel_idle, Introduce cstate limits and fix hardware cstate disable
    value
  cpuidle, Prevent users from enabling cstates that are disabled in
    Hardware

 arch/x86/include/asm/msr-index.h      |    2 +-
 drivers/cpuidle/governors/ladder.c    |    3 ++
 drivers/cpuidle/governors/menu.c      |    4 ++
 drivers/cpuidle/sysfs.c               |   34 +++++++++++-
 drivers/idle/intel_idle.c             |   93 ++++++++++++++++++++++++++++++++-
 include/linux/cpuidle.h               |   11 +++-
 tools/power/x86/turbostat/turbostat.8 |    2 +-
 tools/power/x86/turbostat/turbostat.c |   10 ++--
 8 files changed, 146 insertions(+), 13 deletions(-)

-- 
1.7.9.3

[toc] | [next] | [standalone]

#1361810 — [PATCH 1/3] idle, rename MSR_NHM_SNB_PKG_CST_CFG_CTL to MSR_PKG_CST_CONFIG_CONTROL

FromPrarit Bhargava <prarit@redhat.com>
Date2016-03-21 14:00 +0100
Subject[PATCH 1/3] idle, rename MSR_NHM_SNB_PKG_CST_CFG_CTL to MSR_PKG_CST_CONFIG_CONTROL
Message-ID<rf7B8-5Mj-19@gated-at.bofh.it>
In reply to#1361808
MSR 0xE2 is not a Nehalem specific register and is used by newer
processors.  This patch renames the processor to the name given in the
Intel SDM [1]

[1] http://www.intel.com/content/www/us/en/processors/architectures-software-developer-manuals.html

Cc: x86@kernel.org
Cc: Len Brown <lenb@kernel.org>
Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
Cc: Borislav Petkov <bp@suse.de>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Dasaratharaman Chandramouli <dasaratharaman.chandramouli@intel.com>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Prarit Bhargava <prarit@redhat.com>
---
 arch/x86/include/asm/msr-index.h      |    2 +-
 drivers/idle/intel_idle.c             |    4 ++--
 tools/power/x86/turbostat/turbostat.8 |    2 +-
 tools/power/x86/turbostat/turbostat.c |   10 +++++-----
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 984ab75..63909ba 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -42,7 +42,7 @@
 #define MSR_FSB_FREQ			0x000000cd
 #define MSR_PLATFORM_INFO		0x000000ce
 
-#define MSR_NHM_SNB_PKG_CST_CFG_CTL	0x000000e2
+#define MSR_PKG_CST_CONFIG_CONTROL	0x000000e2
 #define NHM_C3_AUTO_DEMOTE		(1UL << 25)
 #define NHM_C1_AUTO_DEMOTE		(1UL << 26)
 #define ATM_LNC_C6_AUTO_DEMOTE		(1UL << 25)
diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index cd4510a..2609dff 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -814,9 +814,9 @@ static void auto_demotion_disable(void *dummy)
 {
 	unsigned long long msr_bits;
 
-	rdmsrl(MSR_NHM_SNB_PKG_CST_CFG_CTL, msr_bits);
+	rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_bits);
 	msr_bits &= ~(icpu->auto_demotion_disable_flags);
-	wrmsrl(MSR_NHM_SNB_PKG_CST_CFG_CTL, msr_bits);
+	wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_bits);
 }
 static void c1e_promotion_disable(void *dummy)
 {
diff --git a/tools/power/x86/turbostat/turbostat.8 b/tools/power/x86/turbostat/turbostat.8
index 622db68..7973d2a 100644
--- a/tools/power/x86/turbostat/turbostat.8
+++ b/tools/power/x86/turbostat/turbostat.8
@@ -118,7 +118,7 @@ cpu0: MSR_NHM_PLATFORM_INFO: 0x80838f3012300
 8 * 100 = 800 MHz max efficiency
 35 * 100 = 3500 MHz TSC frequency
 cpu0: MSR_IA32_POWER_CTL: 0x0004005d (C1E auto-promotion: DISabled)
-cpu0: MSR_NHM_SNB_PKG_CST_CFG_CTL: 0x1e000400 (UNdemote-C3, UNdemote-C1, demote-C3, demote-C1, UNlocked: pkg-cstate-limit=0: pc0)
+cpu0: MSR_PKG_CST_CONFIG_CONTROL: 0x1e000400 (UNdemote-C3, UNdemote-C1, demote-C3, demote-C1, UNlocked: pkg-cstate-limit=0: pc0)
 cpu0: MSR_NHM_TURBO_RATIO_LIMIT: 0x25262727
 37 * 100 = 3700 MHz max turbo 4 active cores
 38 * 100 = 3800 MHz max turbo 3 active cores
diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
index 3fa94e2..289ff9f 100644
--- a/tools/power/x86/turbostat/turbostat.c
+++ b/tools/power/x86/turbostat/turbostat.c
@@ -1384,12 +1384,12 @@ dump_nhm_cst_cfg(void)
 {
 	unsigned long long msr;
 
-	get_msr(base_cpu, MSR_NHM_SNB_PKG_CST_CFG_CTL, &msr);
+	get_msr(base_cpu, MSR_PKG_CST_CONFIG_CONTROL, &msr);
 
 #define SNB_C1_AUTO_UNDEMOTE              (1UL << 27)
 #define SNB_C3_AUTO_UNDEMOTE              (1UL << 28)
 
-	fprintf(stderr, "cpu%d: MSR_NHM_SNB_PKG_CST_CFG_CTL: 0x%08llx", base_cpu, msr);
+	fprintf(stderr, "cpu%d: MSR_PKG_CST_CONFIG_CONTROL: 0x%08llx", base_cpu, msr);
 
 	fprintf(stderr, " (%s%s%s%s%slocked: pkg-cstate-limit=%d: %s)\n",
 		(msr & SNB_C3_AUTO_UNDEMOTE) ? "UNdemote-C3, " : "",
@@ -1808,7 +1808,7 @@ void check_permissions()
  * MSR_SMI_COUNT                   0x00000034
  *
  * MSR_PLATFORM_INFO               0x000000ce
- * MSR_NHM_SNB_PKG_CST_CFG_CTL     0x000000e2
+ * MSR_PKG_CST_CONFIG_CONTROL     0x000000e2
  *
  * MSR_PKG_C3_RESIDENCY            0x000003f8
  * MSR_PKG_C6_RESIDENCY            0x000003f9
@@ -1816,7 +1816,7 @@ void check_permissions()
  * MSR_CORE_C6_RESIDENCY           0x000003fd
  *
  * Side effect:
- * sets global pkg_cstate_limit to decode MSR_NHM_SNB_PKG_CST_CFG_CTL
+ * sets global pkg_cstate_limit to decode MSR_PKG_CST_CONFIG_CONTROL
  */
 int probe_nhm_msrs(unsigned int family, unsigned int model)
 {
@@ -1873,7 +1873,7 @@ int probe_nhm_msrs(unsigned int family, unsigned int model)
 	default:
 		return 0;
 	}
-	get_msr(base_cpu, MSR_NHM_SNB_PKG_CST_CFG_CTL, &msr);
+	get_msr(base_cpu, MSR_PKG_CST_CONFIG_CONTROL, &msr);
 	pkg_cstate_limit = pkg_cstate_limits[msr & 0xF];
 
 	get_msr(base_cpu, MSR_PLATFORM_INFO, &msr);
-- 
1.7.9.3

[toc] | [prev] | [next] | [standalone]

#1363649 — Re: [PATCH 1/3] idle, rename MSR_NHM_SNB_PKG_CST_CFG_CTL to MSR_PKG_CST_CONFIG_CONTROL

FromLen Brown <lenb@kernel.org>
Date2016-03-23 21:00 +0100
SubjectRe: [PATCH 1/3] idle, rename MSR_NHM_SNB_PKG_CST_CFG_CTL to MSR_PKG_CST_CONFIG_CONTROL
Message-ID<rfX6F-8f8-1@gated-at.bofh.it>
In reply to#1361810
I agree with this intent, but re-names are a headache.

The last time we re-named an MSR, we had to add the new name,
try to get patches in flight to cut over to it, and then a few
releases later, remove the old name.

Right now, we have the same situation as patches in flight use this register,
so I'll not apply this patch right now.

thanks,
-Len




On Mon, Mar 21, 2016 at 8:49 AM, Prarit Bhargava <prarit@redhat.com> wrote:
> MSR 0xE2 is not a Nehalem specific register and is used by newer
> processors.  This patch renames the processor to the name given in the
> Intel SDM [1]
>
> [1] http://www.intel.com/content/www/us/en/processors/architectures-software-developer-manuals.html
>
> Cc: x86@kernel.org
> Cc: Len Brown <lenb@kernel.org>
> Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Dasaratharaman Chandramouli <dasaratharaman.chandramouli@intel.com>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> ---
>  arch/x86/include/asm/msr-index.h      |    2 +-
>  drivers/idle/intel_idle.c             |    4 ++--
>  tools/power/x86/turbostat/turbostat.8 |    2 +-
>  tools/power/x86/turbostat/turbostat.c |   10 +++++-----
>  4 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 984ab75..63909ba 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -42,7 +42,7 @@
>  #define MSR_FSB_FREQ                   0x000000cd
>  #define MSR_PLATFORM_INFO              0x000000ce
>
> -#define MSR_NHM_SNB_PKG_CST_CFG_CTL    0x000000e2
> +#define MSR_PKG_CST_CONFIG_CONTROL     0x000000e2
>  #define NHM_C3_AUTO_DEMOTE             (1UL << 25)
>  #define NHM_C1_AUTO_DEMOTE             (1UL << 26)
>  #define ATM_LNC_C6_AUTO_DEMOTE         (1UL << 25)
> diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
> index cd4510a..2609dff 100644
> --- a/drivers/idle/intel_idle.c
> +++ b/drivers/idle/intel_idle.c
> @@ -814,9 +814,9 @@ static void auto_demotion_disable(void *dummy)
>  {
>         unsigned long long msr_bits;
>
> -       rdmsrl(MSR_NHM_SNB_PKG_CST_CFG_CTL, msr_bits);
> +       rdmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_bits);
>         msr_bits &= ~(icpu->auto_demotion_disable_flags);
> -       wrmsrl(MSR_NHM_SNB_PKG_CST_CFG_CTL, msr_bits);
> +       wrmsrl(MSR_PKG_CST_CONFIG_CONTROL, msr_bits);
>  }
>  static void c1e_promotion_disable(void *dummy)
>  {
> diff --git a/tools/power/x86/turbostat/turbostat.8 b/tools/power/x86/turbostat/turbostat.8
> index 622db68..7973d2a 100644
> --- a/tools/power/x86/turbostat/turbostat.8
> +++ b/tools/power/x86/turbostat/turbostat.8
> @@ -118,7 +118,7 @@ cpu0: MSR_NHM_PLATFORM_INFO: 0x80838f3012300
>  8 * 100 = 800 MHz max efficiency
>  35 * 100 = 3500 MHz TSC frequency
>  cpu0: MSR_IA32_POWER_CTL: 0x0004005d (C1E auto-promotion: DISabled)
> -cpu0: MSR_NHM_SNB_PKG_CST_CFG_CTL: 0x1e000400 (UNdemote-C3, UNdemote-C1, demote-C3, demote-C1, UNlocked: pkg-cstate-limit=0: pc0)
> +cpu0: MSR_PKG_CST_CONFIG_CONTROL: 0x1e000400 (UNdemote-C3, UNdemote-C1, demote-C3, demote-C1, UNlocked: pkg-cstate-limit=0: pc0)
>  cpu0: MSR_NHM_TURBO_RATIO_LIMIT: 0x25262727
>  37 * 100 = 3700 MHz max turbo 4 active cores
>  38 * 100 = 3800 MHz max turbo 3 active cores
> diff --git a/tools/power/x86/turbostat/turbostat.c b/tools/power/x86/turbostat/turbostat.c
> index 3fa94e2..289ff9f 100644
> --- a/tools/power/x86/turbostat/turbostat.c
> +++ b/tools/power/x86/turbostat/turbostat.c
> @@ -1384,12 +1384,12 @@ dump_nhm_cst_cfg(void)
>  {
>         unsigned long long msr;
>
> -       get_msr(base_cpu, MSR_NHM_SNB_PKG_CST_CFG_CTL, &msr);
> +       get_msr(base_cpu, MSR_PKG_CST_CONFIG_CONTROL, &msr);
>
>  #define SNB_C1_AUTO_UNDEMOTE              (1UL << 27)
>  #define SNB_C3_AUTO_UNDEMOTE              (1UL << 28)
>
> -       fprintf(stderr, "cpu%d: MSR_NHM_SNB_PKG_CST_CFG_CTL: 0x%08llx", base_cpu, msr);
> +       fprintf(stderr, "cpu%d: MSR_PKG_CST_CONFIG_CONTROL: 0x%08llx", base_cpu, msr);
>
>         fprintf(stderr, " (%s%s%s%s%slocked: pkg-cstate-limit=%d: %s)\n",
>                 (msr & SNB_C3_AUTO_UNDEMOTE) ? "UNdemote-C3, " : "",
> @@ -1808,7 +1808,7 @@ void check_permissions()
>   * MSR_SMI_COUNT                   0x00000034
>   *
>   * MSR_PLATFORM_INFO               0x000000ce
> - * MSR_NHM_SNB_PKG_CST_CFG_CTL     0x000000e2
> + * MSR_PKG_CST_CONFIG_CONTROL     0x000000e2
>   *
>   * MSR_PKG_C3_RESIDENCY            0x000003f8
>   * MSR_PKG_C6_RESIDENCY            0x000003f9
> @@ -1816,7 +1816,7 @@ void check_permissions()
>   * MSR_CORE_C6_RESIDENCY           0x000003fd
>   *
>   * Side effect:
> - * sets global pkg_cstate_limit to decode MSR_NHM_SNB_PKG_CST_CFG_CTL
> + * sets global pkg_cstate_limit to decode MSR_PKG_CST_CONFIG_CONTROL
>   */
>  int probe_nhm_msrs(unsigned int family, unsigned int model)
>  {
> @@ -1873,7 +1873,7 @@ int probe_nhm_msrs(unsigned int family, unsigned int model)
>         default:
>                 return 0;
>         }
> -       get_msr(base_cpu, MSR_NHM_SNB_PKG_CST_CFG_CTL, &msr);
> +       get_msr(base_cpu, MSR_PKG_CST_CONFIG_CONTROL, &msr);
>         pkg_cstate_limit = pkg_cstate_limits[msr & 0xF];
>
>         get_msr(base_cpu, MSR_PLATFORM_INFO, &msr);
> --
> 1.7.9.3
>



-- 
Len Brown, Intel Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1363656

FromLen Brown <lenb@kernel.org>
Date2016-03-23 21:10 +0100
Message-ID<rfXgm-7y-15@gated-at.bofh.it>
In reply to#1361808
This patch assumes that if a package state is disabled,
the corresponding core state must be disabled.
That assumption is false.
Indeed, that is a very popular and useful configuration.

But even if that were not the case, this software is not necessary,
since the hardware handles demotion "c-state clipping" automatically.

Yes, there is a case where a certain version of a certain processor
has broken demotion, but this isn't the right fix for that.
The right fix for that is here:

https://bugzilla.kernel.org/show_bug.cgi?id=109081

and will be upstream shortly.

thanks,
-Len



On Mon, Mar 21, 2016 at 8:49 AM, Prarit Bhargava <prarit@redhat.com> wrote:
> Some Intel processors have cstates limited by MSR_PKG_CST_CONFIG_CONTROL
> (0xE2), which specifies the lowest processor-specific C-state code name
> (consuming the least power).  The intel_idle driver does not query this MSR to
> see if states are limited by hardware.  In the case of the i7-6700HQ the value
> of bits 3:0 on the MSR are 0x6, meaning that C9-SKL (0x7) and C10-SKL (0x8)
> should have been disabled.
>
> This patch adds a limit field to the cpuidle_state struct to hold the
> cstate encodings for each processor, and queries the values the values
> to confirm support.
>
> After applying this patchset the system now boots and reports that
> some states have been disabled:
>
> [root@dhcp40-125 ~]# dmesg | grep intel_idle
> [    1.163367] intel_idle: MWAIT substates: 0x11142120
> [    1.163368] intel_idle: v0.4 model 0x5E
> [    1.163369] intel_idle: lapic_timer_reliable_states 0xffffffff
> [    1.163370] intel_idle: state C9-SKL (0x7) is disabled.  Max Package limit is 0x6.
> [    1.163371] intel_idle: state C10-SKL (0x8) is disabled.  Max Package limit is 0x6.
>
> Cc: x86@kernel.org
> Cc: Len Brown <lenb@kernel.org>
> Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
> Cc: Borislav Petkov <bp@suse.de>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Dasaratharaman Chandramouli <dasaratharaman.chandramouli@intel.com>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
>
> Prarit Bhargava (3):
>   idle, rename MSR_NHM_SNB_PKG_CST_CFG_CTL to
>     MSR_PKG_CST_CONFIG_CONTROL
>   intel_idle, Introduce cstate limits and fix hardware cstate disable
>     value
>   cpuidle, Prevent users from enabling cstates that are disabled in
>     Hardware
>
>  arch/x86/include/asm/msr-index.h      |    2 +-
>  drivers/cpuidle/governors/ladder.c    |    3 ++
>  drivers/cpuidle/governors/menu.c      |    4 ++
>  drivers/cpuidle/sysfs.c               |   34 +++++++++++-
>  drivers/idle/intel_idle.c             |   93 ++++++++++++++++++++++++++++++++-
>  include/linux/cpuidle.h               |   11 +++-
>  tools/power/x86/turbostat/turbostat.8 |    2 +-
>  tools/power/x86/turbostat/turbostat.c |   10 ++--
>  8 files changed, 146 insertions(+), 13 deletions(-)
>
> --
> 1.7.9.3
>



-- 
Len Brown, Intel Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1363791

FromPrarit Bhargava <prarit@redhat.com>
Date2016-03-24 01:00 +0100
Message-ID<rg0QW-2rr-5@gated-at.bofh.it>
In reply to#1363656

On 03/23/2016 04:05 PM, Len Brown wrote:
> This patch assumes that if a package state is disabled,
> the corresponding core state must be disabled.
> That assumption is false.
> Indeed, that is a very popular and useful configuration.
> 
> But even if that were not the case, this software is not necessary,
> since the hardware handles demotion "c-state clipping" automatically.
> 
> Yes, there is a case where a certain version of a certain processor
> has broken demotion, but this isn't the right fix for that.
> The right fix for that is here:
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=109081

Len, should I rebase on top of this?  Would that work for you?

P.

> 
> and will be upstream shortly.
> 
> thanks,
> -Len
> 
> 
> 
> On Mon, Mar 21, 2016 at 8:49 AM, Prarit Bhargava <prarit@redhat.com> wrote:
>> Some Intel processors have cstates limited by MSR_PKG_CST_CONFIG_CONTROL
>> (0xE2), which specifies the lowest processor-specific C-state code name
>> (consuming the least power).  The intel_idle driver does not query this MSR to
>> see if states are limited by hardware.  In the case of the i7-6700HQ the value
>> of bits 3:0 on the MSR are 0x6, meaning that C9-SKL (0x7) and C10-SKL (0x8)
>> should have been disabled.
>>
>> This patch adds a limit field to the cpuidle_state struct to hold the
>> cstate encodings for each processor, and queries the values the values
>> to confirm support.
>>
>> After applying this patchset the system now boots and reports that
>> some states have been disabled:
>>
>> [root@dhcp40-125 ~]# dmesg | grep intel_idle
>> [    1.163367] intel_idle: MWAIT substates: 0x11142120
>> [    1.163368] intel_idle: v0.4 model 0x5E
>> [    1.163369] intel_idle: lapic_timer_reliable_states 0xffffffff
>> [    1.163370] intel_idle: state C9-SKL (0x7) is disabled.  Max Package limit is 0x6.
>> [    1.163371] intel_idle: state C10-SKL (0x8) is disabled.  Max Package limit is 0x6.
>>
>> Cc: x86@kernel.org
>> Cc: Len Brown <lenb@kernel.org>
>> Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
>> Cc: Borislav Petkov <bp@suse.de>
>> Cc: Andi Kleen <ak@linux.intel.com>
>> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Cc: Dasaratharaman Chandramouli <dasaratharaman.chandramouli@intel.com>
>> Cc: linux-pm@vger.kernel.org
>> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
>>
>> Prarit Bhargava (3):
>>   idle, rename MSR_NHM_SNB_PKG_CST_CFG_CTL to
>>     MSR_PKG_CST_CONFIG_CONTROL
>>   intel_idle, Introduce cstate limits and fix hardware cstate disable
>>     value
>>   cpuidle, Prevent users from enabling cstates that are disabled in
>>     Hardware
>>
>>  arch/x86/include/asm/msr-index.h      |    2 +-
>>  drivers/cpuidle/governors/ladder.c    |    3 ++
>>  drivers/cpuidle/governors/menu.c      |    4 ++
>>  drivers/cpuidle/sysfs.c               |   34 +++++++++++-
>>  drivers/idle/intel_idle.c             |   93 ++++++++++++++++++++++++++++++++-
>>  include/linux/cpuidle.h               |   11 +++-
>>  tools/power/x86/turbostat/turbostat.8 |    2 +-
>>  tools/power/x86/turbostat/turbostat.c |   10 ++--
>>  8 files changed, 146 insertions(+), 13 deletions(-)
>>
>> --
>> 1.7.9.3
>>
> 
> 
>

[toc] | [prev] | [next] | [standalone]

#1364466

FromLen Brown <lenb@kernel.org>
Date2016-03-24 23:00 +0100
Message-ID<rglsm-j4-3@gated-at.bofh.it>
In reply to#1363791
On Wed, Mar 23, 2016 at 7:50 PM, Prarit Bhargava <prarit@redhat.com> wrote:
>
>
> On 03/23/2016 04:05 PM, Len Brown wrote:
>> This patch assumes that if a package state is disabled,
>> the corresponding core state must be disabled.
>> That assumption is false.
>> Indeed, that is a very popular and useful configuration.
>>
>> But even if that were not the case, this software is not necessary,
>> since the hardware handles demotion "c-state clipping" automatically.
>>
>> Yes, there is a case where a certain version of a certain processor
>> has broken demotion, but this isn't the right fix for that.
>> The right fix for that is here:
>>
>> https://bugzilla.kernel.org/show_bug.cgi?id=109081
>
> Len, should I rebase on top of this?  Would that work for you?


I guess I wasn't clear.
I don't see the benefit of your patch.
Please explain it to me.

thanks,
-Len

[toc] | [prev] | [next] | [standalone]

#1365530

FromPrarit Bhargava <prarit@redhat.com>
Date2016-03-28 16:50 +0200
Message-ID<rhGEq-8rj-7@gated-at.bofh.it>
In reply to#1364466

On 03/24/2016 05:54 PM, Len Brown wrote:
> On Wed, Mar 23, 2016 at 7:50 PM, Prarit Bhargava <prarit@redhat.com> wrote:
>>
>>
>> On 03/23/2016 04:05 PM, Len Brown wrote:
>>> This patch assumes that if a package state is disabled,
>>> the corresponding core state must be disabled.
>>> That assumption is false.
>>> Indeed, that is a very popular and useful configuration.
>>>
>>> But even if that were not the case, this software is not necessary,
>>> since the hardware handles demotion "c-state clipping" automatically.
>>>
>>> Yes, there is a case where a certain version of a certain processor
>>> has broken demotion, but this isn't the right fix for that.
>>> The right fix for that is here:
>>>
>>> https://bugzilla.kernel.org/show_bug.cgi?id=109081
>>
>> Len, should I rebase on top of this?  Would that work for you?
> 
> 
> I guess I wasn't clear.
> I don't see the benefit of your patch.
> Please explain it to me.
> 

Len,

Your patch does

+	skl_cstates[5].disabled = 1;	/* C8-SKL */
+	skl_cstates[6].disabled = 1;	/* C9-SKL */

and I don't think that is correct for SKY-H.

Your patch does not take into account that the states are explicitly disabled
in MSR_NHM_SNB_PKG_CST_CFG_CTL.  That is the problem here and what you've done
is simply hammered a disable into those states.

Additionally, your patch does not show the user the correct state information:

    [root@dhcp40-125 ~]# egrep ^ /sys/devices/system/cpu/cpu0/cpuidle/state?/disable
    /sys/devices/system/cpu/cpu0/cpuidle/state0/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state1/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state2/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state3/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state4/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state5/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state6/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state7/disable:1:0 << should be 1
    /sys/devices/system/cpu/cpu0/cpuidle/state8/disable:1:0 << should be 1

The fix is to honour the settings in MSR_NHM_SNB_PKG_CST_CFG_CTL.  I cannot say
for certain that ALL SKY-H are impacted (you are admittedly in better position
to say so or not).  I can say that on the 2 systems tested here the
MSR_NHM_SNB_PKG_CST_CFG_CTL do have the appropriate disable value set.

/me could be missing some important info  -- again, perhaps there are some
SKY-H's out there that do not have states disabled in
MSR_NHM_SNB_PKG_CST_CFG_CTL, and that's why I've proposed rebasing on top of
your change.

P.


> thanks,
> -Len
>

[toc] | [prev] | [next] | [standalone]

#1367811

FromLen Brown <lenb@kernel.org>
Date2016-03-31 07:00 +0200
Message-ID<riCS5-83I-1@gated-at.bofh.it>
In reply to#1365530
> Len,
>
> Your patch does
>
> +       skl_cstates[5].disabled = 1;    /* C8-SKL */
> +       skl_cstates[6].disabled = 1;    /* C9-SKL */
>
> and I don't think that is correct for SKY-H.

For https://bugzilla.kernel.org/show_bug.cgi?id=109081
it is correct.

> Your patch does not take into account that the states are explicitly disabled
> in MSR_NHM_SNB_PKG_CST_CFG_CTL.  That is the problem here and what you've done
> is simply hammered a disable into those states.

ENOPARSE.
Are we talking about the failure in
https://bugzilla.kernel.org/show_bug.cgi?id=109081
or a different problem?

>
> Additionally, your patch does not show the user the correct state information:
>
>     [root@dhcp40-125 ~]# egrep ^ /sys/devices/system/cpu/cpu0/cpuidle/state?/disable
>     /sys/devices/system/cpu/cpu0/cpuidle/state0/disable:1:0
>     /sys/devices/system/cpu/cpu0/cpuidle/state1/disable:1:0
>     /sys/devices/system/cpu/cpu0/cpuidle/state2/disable:1:0
>     /sys/devices/system/cpu/cpu0/cpuidle/state3/disable:1:0
>     /sys/devices/system/cpu/cpu0/cpuidle/state4/disable:1:0
>     /sys/devices/system/cpu/cpu0/cpuidle/state5/disable:1:0
>     /sys/devices/system/cpu/cpu0/cpuidle/state6/disable:1:0
>     /sys/devices/system/cpu/cpu0/cpuidle/state7/disable:1:0 << should be 1
>     /sys/devices/system/cpu/cpu0/cpuidle/state8/disable:1:0 << should be 1

the 'disabled' attribute you see in sysfs is not
struct cpuidle_state.disabled
it is
struct cpuidle_state_usage.disabled

> The fix is to honour the settings in MSR_NHM_SNB_PKG_CST_CFG_CTL.  I cannot say
> for certain that ALL SKY-H are impacted (you are admittedly in better position
> to say so or not).  I can say that on the 2 systems tested here the
> MSR_NHM_SNB_PKG_CST_CFG_CTL do have the appropriate disable value set.
>
> /me could be missing some important info  -- again, perhaps there are some
> SKY-H's out there that do not have states disabled in
> MSR_NHM_SNB_PKG_CST_CFG_CTL, and that's why I've proposed rebasing on top of
> your change.

Do you see this debug message when you run current upstream on this hardware?

                /* if state marked as disabled, skip it */
                if (cpuidle_state_table[cstate].disabled != 0) {
                        pr_debug(PREFIX "state %s is disabled",
                                cpuidle_state_table[cstate].name);
                        continue;
                }


If no, then my patch is not disabling C8/C9 on your system.

Also, if it were, the code above causes the states to not appear
at all in sysfs, because they are not registered.

Re: MSR_NHM_SNB_PKG_CST_CFG_CTL

if PC10 is disabled there, then functionally, it doesn't matter what we do,
which is why my patch does nothing when PC10 is disabled.

In such a scenario, pc10 presence in sysfs (and cpufreq)
is cosmetic.  The hardware knows what to do.

Do you think that cosmetic issue is worth dealing with?
Note that the decoding of that MSR changes with every CPU,
so to get it right (like turbostat does), we'd need a table.
Also, it would be useful only for states which are  PC states only.
ie. we can't disable CC7 just because PC7 is disabled. etc.
So you could remove PC8, PC9, PC10 from sysfs on SKL
when they are disabled, but that is all.

thanks,
Len Brown, Intel Open Source Technology Center

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web