[PATCH] x86: Add clflush before monitor for Intel 7400 series - v2

[PATCH] x86: Add clflush before monitor for Intel 7400 series - v2

[Pallipadi, Venkatesh]

For Intel 7400 series CPUs, the recommendation is to use a clflush on the
monitored address just before monitor and mwait pair [1]. This clflush makes
sure that there are no false wakeups from mwait when the monitored address
was recently written to.

[1] "MONITOR/MWAIT Recommendations for Intel Xeon Processor 7400 series"
section in specification update document of 7400 series
http://download.intel.com/design/xeon/specupdt/32033601.pdf

Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>

---

Earlier patch:
http://marc.info/?l=linux-kernel&m=122341330606025&w=2

Current changes:
Minor modification based on Peter's comment

 arch/x86/include/asm/cpufeature.h |    1 +
 arch/x86/kernel/cpu/intel.c       |    3 +++
 arch/x86/kernel/process.c         |    6 ++++++
 3 files changed, 10 insertions(+)

Index: linux-2.6/arch/x86/kernel/cpu/intel.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/cpu/intel.c	2009-02-06 15:58:25.000000000 -0800
+++ linux-2.6/arch/x86/kernel/cpu/intel.c	2009-02-06 16:11:41.000000000 -0800
@@ -291,6 +291,9 @@ static void __cpuinit init_intel(struct 
 		ds_init_intel(c);
 	}
 
+	if (c->x86 == 6 && c->x86_model == 29 && cpu_has_clflush)
+		set_cpu_cap(c, X86_FEATURE_CLFLUSH_MONITOR);
+
 #ifdef CONFIG_X86_64
 	if (c->x86 == 15)
 		c->x86_cache_alignment = c->x86_clflush_size * 2;
Index: linux-2.6/arch/x86/kernel/process.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/process.c	2009-02-03 12:58:38.000000000 -0800
+++ linux-2.6/arch/x86/kernel/process.c	2009-02-06 16:11:41.000000000 -0800
@@ -180,6 +180,9 @@ void mwait_idle_with_hints(unsigned long
 
 	trace_power_start(&it, POWER_CSTATE, (ax>>4)+1);
 	if (!need_resched()) {
+		if (cpu_has(&current_cpu_data, X86_FEATURE_CLFLUSH_MONITOR))
+			clflush((void *)&current_thread_info()->flags);
+
 		__monitor((void *)&current_thread_info()->flags, 0, 0);
 		smp_mb();
 		if (!need_resched())
@@ -194,6 +197,9 @@ static void mwait_idle(void)
 	struct power_trace it;
 	if (!need_resched()) {
 		trace_power_start(&it, POWER_CSTATE, 1);
+		if (cpu_has(&current_cpu_data, X86_FEATURE_CLFLUSH_MONITOR))
+			clflush((void *)&current_thread_info()->flags);
+
 		__monitor((void *)&current_thread_info()->flags, 0, 0);
 		smp_mb();
 		if (!need_resched())
Index: linux-2.6/arch/x86/include/asm/cpufeature.h
===================================================================
--- linux-2.6.orig/arch/x86/include/asm/cpufeature.h	2009-02-06 16:11:12.000000000 -0800
+++ linux-2.6/arch/x86/include/asm/cpufeature.h	2009-02-06 16:40:44.000000000 -0800
@@ -93,6 +93,7 @@
 #define X86_FEATURE_XTOPOLOGY	(3*32+22) /* cpu topology enum extensions */
 #define X86_FEATURE_TSC_RELIABLE (3*32+23) /* TSC is known to be reliable */
 #define X86_FEATURE_NONSTOP_TSC	(3*32+24) /* TSC does not stop in C states */
+#define X86_FEATURE_CLFLUSH_MONITOR (3*32+25) /* "" clflush reqd with monitor */
 
 /* Intel-defined CPU features, CPUID level 0x00000001 (ecx), word 4 */
 #define X86_FEATURE_XMM3	(4*32+ 0) /* "pni" SSE-3 */

Re: [PATCH] x86: Add clflush before monitor for Intel 7400 series - v2

[Alan Cox]

On Fri, 6 Feb 2009 16:47:29 -0800
"Pallipadi, Venkatesh" <venkatesh.pallipadi@intel.com> wrote:

> 
> 
> For Intel 7400 series CPUs, the recommendation is to use a clflush on the
> monitored address just before monitor and mwait pair [1]. This clflush makes
> sure that there are no false wakeups from mwait when the monitored address
> was recently written to.

Given our mwait usages will very quickly go back to sleep in such a case
and it would almost certainly be one sleep only is this really worth the
effort ?

Re: [PATCH] x86: Add clflush before monitor for Intel 7400 series - v2

[Pallipadi, Venkatesh]

On Sat, Feb 07, 2009 at 06:02:45AM -0800, Alan Cox wrote:
> On Fri, 6 Feb 2009 16:47:29 -0800
> "Pallipadi, Venkatesh" <venkatesh.pallipadi@intel.com> wrote:
> 
> > 
> > 
> > For Intel 7400 series CPUs, the recommendation is to use a clflush on the
> > monitored address just before monitor and mwait pair [1]. This clflush makes
> > sure that there are no false wakeups from mwait when the monitored address
> > was recently written to.
> 
> Given our mwait usages will very quickly go back to sleep in such a case
> and it would almost certainly be one sleep only is this really worth the
> effort ?

Yes. If we only consider the CPU idle behavior, we really do not need the
patch as we will go back to idle. But, there are other factors:
- drivers/idle/i7300_idle.c  which tries to save memory power based on CPU
  idle time. It gets confused with these short idles.
- cpuidle menu governor  These platforms may also support more than one C-state.
  C1 and CC3. So, we will go through the C-state policy in menu governor, which
  again looks at idle time and may end up taking wrong decisions due to these
  short idles.

We can make the above code to be more clever, to ignore short idles. But, this
patch seems to be the easier and clean way as the errata is only in a particular
CPU model.

Thanks,
Venki

Re: [PATCH] x86: Add clflush before monitor for Intel 7400 series - v2

[Nick Piggin]

On Sunday 08 February 2009 03:47:07 Pallipadi, Venkatesh wrote:
> On Sat, Feb 07, 2009 at 06:02:45AM -0800, Alan Cox wrote:
> > On Fri, 6 Feb 2009 16:47:29 -0800
> >
> > "Pallipadi, Venkatesh" <venkatesh.pallipadi@intel.com> wrote:
> > > For Intel 7400 series CPUs, the recommendation is to use a clflush on
> > > the monitored address just before monitor and mwait pair [1]. This
> > > clflush makes sure that there are no false wakeups from mwait when the
> > > monitored address was recently written to.
> >
> > Given our mwait usages will very quickly go back to sleep in such a case
> > and it would almost certainly be one sleep only is this really worth the
> > effort ?
>
> Yes. If we only consider the CPU idle behavior, we really do not need the
> patch as we will go back to idle. But, there are other factors:
> - drivers/idle/i7300_idle.c  which tries to save memory power based on CPU
>   idle time. It gets confused with these short idles.
> - cpuidle menu governor  These platforms may also support more than one
> C-state. C1 and CC3. So, we will go through the C-state policy in menu
> governor, which again looks at idle time and may end up taking wrong
> decisions due to these short idles.
>
> We can make the above code to be more clever, to ignore short idles. But,
> this patch seems to be the easier and clean way as the errata is only in a
> particular CPU model.

Have you benchmarked it? With something like tbench which IIRC should
generate a good number of idle/busy transitions?

Re: [PATCH] x86: Add clflush before monitor for Intel 7400 series - v2

[Pallipadi, Venkatesh]

On Sun, 2009-02-08 at 21:53 -0800, Nick Piggin wrote:
> On Sunday 08 February 2009 03:47:07 Pallipadi, Venkatesh wrote:
> > On Sat, Feb 07, 2009 at 06:02:45AM -0800, Alan Cox wrote:
> > > On Fri, 6 Feb 2009 16:47:29 -0800
> > >
> > > "Pallipadi, Venkatesh" <venkatesh.pallipadi@intel.com> wrote:
> > > > For Intel 7400 series CPUs, the recommendation is to use a clflush on
> > > > the monitored address just before monitor and mwait pair [1]. This
> > > > clflush makes sure that there are no false wakeups from mwait when the
> > > > monitored address was recently written to.
> > >
> > > Given our mwait usages will very quickly go back to sleep in such a case
> > > and it would almost certainly be one sleep only is this really worth the
> > > effort ?
> >
> > Yes. If we only consider the CPU idle behavior, we really do not need the
> > patch as we will go back to idle. But, there are other factors:
> > - drivers/idle/i7300_idle.c  which tries to save memory power based on CPU
> >   idle time. It gets confused with these short idles.
> > - cpuidle menu governor  These platforms may also support more than one
> > C-state. C1 and CC3. So, we will go through the C-state policy in menu
> > governor, which again looks at idle time and may end up taking wrong
> > decisions due to these short idles.
> >
> > We can make the above code to be more clever, to ignore short idles. But,
> > this patch seems to be the easier and clean way as the errata is only in a
> > particular CPU model.
> 
> Have you benchmarked it? With something like tbench which IIRC should
> generate a good number of idle/busy transitions?
> 

We haven't run tbench specifically. But, we noticed this issue running
specpower workload on this platform. Especially the 10-20% point of
specpower, which also has notable idle/busy transitions, this patch
helps.

Thanks,
Venki

[PATCH] x86: Add clflush before monitor for Intel 7400 series - v2

[Pallipadi, Venkatesh]

(Sorry for the duplicate. resending to proper recipients)

For Intel 7400 series CPUs, the recommendation is to use a clflush on the
monitored address just before monitor and mwait pair [1]. This clflush makes
sure that there are no false wakeups from mwait when the monitored address
was recently written to.

[1] "MONITOR/MWAIT Recommendations for Intel Xeon Processor 7400 series"
section in specification update document of 7400 series
http://download.intel.com/design/xeon/specupdt/32033601.pdf

Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>

---

Earlier patch:
http://marc.info/?l=linux-kernel&m=122341330606025&w=2

Current changes:
Minor modification based on Peter's comment

 arch/x86/include/asm/cpufeature.h |    1 +
 arch/x86/kernel/cpu/intel.c       |    3 +++
 arch/x86/kernel/process.c         |    6 ++++++
 3 files changed, 10 insertions(+)

Index: linux-2.6/arch/x86/kernel/cpu/intel.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/cpu/intel.c	2009-02-06 15:58:25.000000000 -0800
+++ linux-2.6/arch/x86/kernel/cpu/intel.c	2009-02-06 16:11:41.000000000 -0800
@@ -291,6 +291,9 @@ static void __cpuinit init_intel(struct 
 		ds_init_intel(c);
 	}
 
+	if (c->x86 == 6 && c->x86_model == 29 && cpu_has_clflush)
+		set_cpu_cap(c, X86_FEATURE_CLFLUSH_MONITOR);
+
 #ifdef CONFIG_X86_64
 	if (c->x86 == 15)
 		c->x86_cache_alignment = c->x86_clflush_size * 2;
Index: linux-2.6/arch/x86/kernel/process.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/process.c	2009-02-03 12:58:38.000000000 -0800
+++ linux-2.6/arch/x86/kernel/process.c	2009-02-06 16:11:41.000000000 -0800
@@ -180,6 +180,9 @@ void mwait_idle_with_hints(unsigned long
 
 	trace_power_start(&it, POWER_CSTATE, (ax>>4)+1);
 	if (!need_resched()) {
+		if (cpu_has(&current_cpu_data, X86_FEATURE_CLFLUSH_MONITOR))
+			clflush((void *)&current_thread_info()->flags);
+
 		__monitor((void *)&current_thread_info()->flags, 0, 0);
 		smp_mb();
 		if (!need_resched())
@@ -194,6 +197,9 @@ static void mwait_idle(void)
 	struct power_trace it;
 	if (!need_resched()) {
 		trace_power_start(&it, POWER_CSTATE, 1);
+		if (cpu_has(&current_cpu_data, X86_FEATURE_CLFLUSH_MONITOR))
+			clflush((void *)&current_thread_info()->flags);
+
 		__monitor((void *)&current_thread_info()->flags, 0, 0);
 		smp_mb();
 		if (!need_resched())
Index: linux-2.6/arch/x86/include/asm/cpufeature.h
===================================================================
--- linux-2.6.orig/arch/x86/include/asm/cpufeature.h	2009-02-06 16:11:12.000000000 -0800
+++ linux-2.6/arch/x86/include/asm/cpufeature.h	2009-02-06 16:40:44.000000000 -0800
@@ -93,6 +93,7 @@
 #define X86_FEATURE_XTOPOLOGY	(3*32+22) /* cpu topology enum extensions */
 #define X86_FEATURE_TSC_RELIABLE (3*32+23) /* TSC is known to be reliable */
 #define X86_FEATURE_NONSTOP_TSC	(3*32+24) /* TSC does not stop in C states */
+#define X86_FEATURE_CLFLUSH_MONITOR (3*32+25) /* "" clflush reqd with monitor */
 
 /* Intel-defined CPU features, CPUID level 0x00000001 (ecx), word 4 */
 #define X86_FEATURE_XMM3	(4*32+ 0) /* "pni" SSE-3 */

Re: [PATCH] x86: Add clflush before monitor for Intel 7400 series - v2

[Ingo Molnar]

* Pallipadi, Venkatesh <venkatesh.pallipadi@intel.com> wrote:

> For Intel 7400 series CPUs, the recommendation is to use a clflush on the 
> monitored address just before monitor and mwait pair [1]. This clflush makes sure 
> that there are no false wakeups from mwait when the monitored address was recently 
> written to.
> 
> [1] "MONITOR/MWAIT Recommendations for Intel Xeon Processor 7400 series"
> section in specification update document of 7400 series
> http://download.intel.com/design/xeon/specupdt/32033601.pdf
> 
> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
> 
> ---
> 
> Earlier patch:
> http://marc.info/?l=linux-kernel&m=122341330606025&w=2
> 
> Current changes:
> Minor modification based on Peter's comment
> 
>  arch/x86/include/asm/cpufeature.h |    1 +
>  arch/x86/kernel/cpu/intel.c       |    3 +++
>  arch/x86/kernel/process.c         |    6 ++++++
>  3 files changed, 10 insertions(+)

applied to tip/x86/urgent, thanks Venki!

	Ingo