MTRR: Fix race causing set_mtrr to go into infinite loop
PATCH 1/1 MTRR: Fix race causing set_mtrr to go into infinite loop
Processors synchronization in set_mtrr requires the .gate field to be set after .count field is properly initialized. Without an explicit barrier, the compiler was reordering those memory stores. That was sometimes causing a processor (in ipi_handler) to see the .gate change and decrement .count before the latter is set by set_mtrr() (which then hangs in a infinite loop with irqs disabled).
--- arch/i386/kernel/cpu/mtrr/main.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
arch/i386/kernel/cpu/mtrr/main.c +4 -0
@@ -229,6 +229,8 @@ static void set_mtrr(unsigned int reg, unsigned long base, data.smp_size = size; data.smp_type = type;atomic_set (&data.count,num_booting_cpus () -1 ); +/* make sure data.count is visible before unleashing other CPUs */ +smp_wmb ();atomic_set (&data.gate,0 );/* Start the ball rolling on other CPUs */
@@ -242,6 +244,7 @@ static void set_mtrr(unsigned int reg, unsigned long base,/* ok, reset count and toggle gate */ atomic_set (&data.count,num_booting_cpus () -1 ); +smp_wmb ();atomic_set (&data.gate,1 );/* do our MTRR business */
@@ -260,6 +263,7 @@ static void set_mtrr(unsigned int reg, unsigned long base,cpu_relax ();atomic_set (&data.count,num_booting_cpus () -1 ); +smp_wmb ();atomic_set (&data.gate,0 );/*
-- 1.5.2.2
On 06/25/2007 05:38 PM, Loic Prylli wrote:
[cc: Andi]
Quoted 41 lines
Processors synchronization in
set_mtrrrequires the .gate field to be set after .count field is properly initialized. Without an explicit barrier, the compiler was reordering those memory stores. That was sometimes causing a processor (inipi_handler) to see the .gate change and decrement .count before the latter is set byset_mtrr()(which then hangs in a infinite loop with irqs disabled).Signed-off-by Loic Prylli <[email protected]>--- arch/i386/kernel/cpu/mtrr/main.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)arch/i386/kernel/cpu/mtrr/main.c +4
-0@@ -229,6 +229,8 @@ static void set_mtrr(unsigned int reg, unsigned long base, data.smp_size = size; data.smp_type = type;atomic_set (&data.count,num_booting_cpus () -1 ); +/* make sure data.count is visible before unleashing other CPUs */ +smp_wmb ();atomic_set (&data.gate,0 );/* Start the ball rolling on other CPUs */ @@ -242,6 +244,7 @@ static void set_mtrr(unsigned int reg, unsigned long base,/* ok, reset count and toggle gate */ atomic_set (&data.count,num_booting_cpus () -1 ); +smp_wmb ();atomic_set (&data.gate,1 );/* do our MTRR business */ @@ -260,6 +263,7 @@ static void set_mtrr(unsigned int reg, unsigned long base,cpu_relax ();atomic_set (&data.count,num_booting_cpus () -1 ); +smp_wmb ();atomic_set (&data.gate,0 );/*
Quoted 10 lines · On Tuesday 26 June 2007 00:05:17 Chuck Ebbert wrote:
On 06/25/2007 05:38 PM, Loic Prylli wrote:
[cc: Andi]
Quoted 6 lines
Processors synchronization in
set_mtrrrequires the .gate field to be set after .count field is properly initialized. Without an explicit barrier, the compiler was reordering those memory stores. That was sometimes causing a processor (inipi_handler) to see the .gate change and decrement .count before the latter is set byset_mtrr()(which then hangs in a infinite loop with irqs disabled).
Hmm, perhaps we should just put the smp_wmb into atomic_set(). Near all other atomic operations have memory barriers too. I think that would be the better fix.
-Andi
Quoted 15 lines · Andi Kleen
Quoted 12 lines · On Tuesday 26 June 2007 00:05:17 Chuck Ebbert wrote:
On 06/25/2007 05:38 PM, Loic Prylli wrote:
[cc: Andi]
Quoted 7 lines
Processors synchronization in
set_mtrrrequires the .gate field to be set after .count field is properly initialized. Without an explicit barrier, the compiler was reordering those memory stores. That was sometimes causing a processor (inipi_handler) to see the .gate change and decrement .count before the latter is set byset_mtrr()(which then hangs in a infinite loop with irqs disabled).
Quoted 6 lines
Hmm, perhaps we should just put the
smp_wmbintoatomic_set(). Near all other atomic operations have memory barriers too. I think that would be the better fix.-Andi
In Documentation/atomic_ops.txt atomic_set/atomic_read are described as nothing more than a type-safe assignement or reading, without any extra semantics. For other atomic operations, the rule is that any atomic operation that doesn't return a value doesn't come with a barrier (and any operation that returns the atomic value must have memory barriers).
So I guess you are suggesting to change the doc and the implementation for all arches.
I should admit I did not knew a number of atomic operations did not imply memory-barriers before. But maybe the extra cost might not be completely negligible, especially if, for consistency with other "barrier-implied" atomic operations, a new memory barrier is put before and after,
Are you suggested changing just atomic_set(), or also other barrier-free atomic operations :"atomic_dec", "atomic_inc", "atomic_add", "atomic_sub" ?
Independently of what is done to atomic, what about not making the .gate field an atomic_t, but a simple "int" in the mttr code, since the only operations done on it are atomic_read and atomic_set?
Loic
Quoted 12 lines · Andi Kleen
Quoted 10 lines · On Tuesday 26 June 2007 00:05:17 Chuck Ebbert wrote:
On 06/25/2007 05:38 PM, Loic Prylli wrote:
[cc: Andi]
Quoted 6 lines
Processors synchronization in
set_mtrrrequires the .gate field to be set after .count field is properly initialized. Without an explicit barrier, the compiler was reordering those memory stores. That was sometimes causing a processor (inipi_handler) to see the .gate change and decrement .count before the latter is set byset_mtrr()(which then hangs in a infinite loop with irqs disabled).
Quoted 3 lines
Hmm, perhaps we should just put the
smp_wmbintoatomic_set(). Near all other atomic operations have memory barriers too. I think that would be the better fix.
Can we get something merged before 2.6.22-final?
The original patch seems okay...