sched-fix-scheduling-latencies-in-mttrc reenables interrupts
PATCH 1/1 sched-fix-scheduling-latencies-in-mttrc reenables interrupts
Hi Ingo & Andrew,
The patch sched-fix-scheduling-latencies-in-mttr in recent -mm kernels has the bad side-effect of re-enabling interrupts even if they were disabled. This caused bugs in Software Suspend 2 which reenabled MTRRs whilst interrupts were already disabled.
Attached is a replacement patch which uses spin_lock_irqsave instead of spin_lock_irq.
Kind regards,
Bernard.
Signature · 1 line
Bernard Blackham <bernard at blackham dot com dot au>
arch/i386/kernel/cpu/mtrr/generic.c renamed from arch/i386/kernel/cpu/mtrr/generic.c.orig +8 -5
@@ -232,6 +232,7 @@ static unsigned long set_mtrr_state(u32static unsigned long cr4 =0 ;static u32 deftype_lo, deftype_hi;static spinlock_t set_atomicity_lock =SPIN_LOCK_UNLOCKED ; +static unsigned long sal_flags;static void prepare_set (void ) {
@@ -240,11 +241,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_irqsave (&set_atomicity_lock, sal_flags);/* 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 +270,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 +282,7 @@ static void post_set(void)/* Restore value of CR4 */ if ( cpu_has_pge )write_cr4 (cr4); -spin_unlock (&set_atomicity_lock); +spin_unlock_irqrestore (&set_atomicity_lock, sal_flags); }static void generic_set_all (void )
Quoted 5 lines · Bernard Blackham
The patch sched-fix-scheduling-latencies-in-mttr in recent -mm
kernels has the bad side-effect of re-enabling interrupts even if they were disabled. This caused bugs in Software Suspend 2 which reenabled MTRRs whilst interrupts were already disabled.
OK.
Quoted 5 lines
Attached is a replacement patch which uses
spin_lock_irqsaveinstead ofspin_lock_irq.+static unsigned long
sal_flags;
Quoted 12 lines
static void
prepare_set(void)
{
@@ -240,11 +241,14 @@ static voidprepare_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_irqsave(&set_atomicity_lock, sal_flags);
Not OK. That global `sal_flags' is not protected by the spinlock because spin_lock_irqsave() saves the flags before taking the lock.
How about this?
arch/i386/kernel/cpu/mtrr/generic.c renamed from arch/i386/kernel/cpu/mtrr/generic.c~sched-fix-scheduling-latencies-in-mttrc-reenables-interrupts +17 -6
@@ -233,6 +233,13 @@ static unsigned long cr4 = 0;static u32 deftype_lo, deftype_hi;static spinlock_t set_atomicity_lock =SPIN_LOCK_UNLOCKED ; +/* +* Since we are disabling the cache don't allow any interrupts - they +* would run extremely slow and would only increase the pain. The caller must +* ensure that local interrupts are disabled and are reenabled after post_set() +* has been called. +*/ +static void prepare_set (void ) {unsigned long cr0;
@@ -240,11 +247,8 @@ 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 */ -/* -* 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); + +spin_lock (&set_atomicity_lock);/* Enter the no-fill (CD=1, NW=0) cache mode and flush caches. */ cr0 =read_cr0 () |0x40000000 ;/* set CD flag */
@@ -281,19 +285,22 @@ static void post_set(void)/* Restore value of CR4 */ if ( cpu_has_pge )write_cr4 (cr4); -spin_unlock_irq (&set_atomicity_lock); +spin_unlock (&set_atomicity_lock); }static void generic_set_all (void ) {unsigned long mask, count; +unsigned long flags; +local_irq_save (flags);prepare_set ();/* Actually set the state */ mask =set_mtrr_state (deftype_lo,deftype_hi);post_set (); +local_irq_restore (flags);/* Use the atomic bitops to update the global mask */ for (count =0 ; count <sizeof mask *8 ; ++count) {
@@ -316,6 +323,9 @@ static void generic_set_mtrr(unsigned in[RETURNS] Nothing. */ { +unsigned long flags; + +local_irq_save (flags);prepare_set ();if (size ==0 ) {
@@ -330,6 +340,7 @@ static void generic_set_mtrr(unsigned in }post_set (); +local_irq_restore (flags); }int generic_validate_add_page (unsigned long base,unsigned long size,unsigned int type)
_
Quoted 3 lines · Andrew Morton
Not OK. That global `sal_flags' is not protected by the spinlock because
spin_lock_irqsave()saves the flags before taking the lock.
Ahh, d'oh.
Quoted 3 lines
How about this?
diff -puN
arch/i386/kernel/cpu/mtrr/generic.c~sched-fix-scheduling-latencies-in-mttrc-reenables-interruptsarch/i386/kernel/cpu/mtrr/generic.c
Won't object here.
Thanks,
Bernard.
Signature · 1 line
Bernard Blackham <bernard at blackham dot com dot au>