Re: [patch] voluntary-preempt-2.6.9-rc1-bk4-Q5

[Ingo Molnar <mingo@elte.hu>]

* Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Lee Revell <rlrevell@joe-job.com> wrote:
> 
> > File under boot-time stuff, I guess.  This could be bad if X crashes,
> > but I can't remember the last time this happened to me, and I use xorg
> > CVS.
> 
> but the first wbinvd() within prepare_set() seems completely unnecessary
> - we can flush the cache after disabling the cache just fine.

the third wbinvd() in post_set() seems unnecessary too - what kind of
cache do we expect to flush, we've disabled caching in the CPU ... But
the Intel pseudocode does it too - this is a thinko i think.

another thing is that interrupts are not disabled (although the Intel
docs suggest so). It is best to disable interrupts because any handler
executing in this window will perform extremely slowly (because caches
are disabled), and might even interfere with MTRR setting. Best disable
IRQs.

so ... could you try the patch below - does it work and how does the
latency look like now? (ontop of an unmodified generic.c)

	Ingo

--- linux/arch/i386/kernel/cpu/mtrr/generic.c.orig	
+++ linux/arch/i386/kernel/cpu/mtrr/generic.c	
@@ -240,11 +240,14 @@ static void prepare_set(void)
 	/*  Note that this is not ideal, since the cache is only flushed/disabled
 	   for this CPU while the MTRRs are changed, but changing this requires
 	   more invasive changes to the way the kernel boots  */
-	spin_lock(&set_atomicity_lock);
+	/*
+	 * Since we are disabling the cache dont allow any interrupts - they
+	 * would run extremely slow and would only increase the pain:
+	 */
+	spin_lock_irq(&set_atomicity_lock);
 
 	/*  Enter the no-fill (CD=1, NW=0) cache mode and flush caches. */
 	cr0 = read_cr0() | 0x40000000;	/* set CD flag */
-	wbinvd();
 	write_cr0(cr0);
 	wbinvd();
 
@@ -266,8 +269,7 @@ static void prepare_set(void)
 
 static void post_set(void)
 {
-	/*  Flush caches and TLBs  */
-	wbinvd();
+	/*  Flush TLBs (no need to flush caches - they are disabled)  */
 	__flush_tlb();
 
 	/* Intel (P6) standard MTRRs */
@@ -279,7 +281,7 @@ static void post_set(void)
 	/*  Restore value of CR4  */
 	if ( cpu_has_pge )
 		write_cr4(cr4);
-	spin_unlock(&set_atomicity_lock);
+	spin_unlock_irq(&set_atomicity_lock);
 }
 
 static void generic_set_all(void)