Re: [PATCH] MTRR: Fix race causing set_mtrr to go into infinite loop
On 06/25/2007 06:34 PM, Andi Kleen wrote: > On Tuesday 26 June 2007 00:05:17 Chuck Ebbert wrote: >> On 06/25/2007 05:38 PM, Loic Prylli wrote: >> >> [cc: Andi] >> >>> 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). > > 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. Can we get something merged before 2.6.22-final? The original patch seems okay... - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] MTRR: Fix race causing set_mtrr to go into infinite loop
On 06/25/2007 06:34 PM, Andi Kleen wrote: On Tuesday 26 June 2007 00:05:17 Chuck Ebbert wrote: On 06/25/2007 05:38 PM, Loic Prylli wrote: [cc: Andi] 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). 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. Can we get something merged before 2.6.22-final? The original patch seems okay... - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] MTRR: Fix race causing set_mtrr to go into infinite loop
On 6/25/2007 6:34 PM, Andi Kleen wrote: > On Tuesday 26 June 2007 00:05:17 Chuck Ebbert wrote: > >> On 06/25/2007 05:38 PM, Loic Prylli wrote: >> >> [cc: Andi] >> >> >>> 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). >>> > > 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 > 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] MTRR: Fix race causing set_mtrr to go into infinite loop
On Tuesday 26 June 2007 00:05:17 Chuck Ebbert wrote: > On 06/25/2007 05:38 PM, Loic Prylli wrote: > > [cc: Andi] > > > 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). 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] MTRR: Fix race causing set_mtrr to go into infinite loop
On 06/25/2007 05:38 PM, Loic Prylli wrote: [cc: Andi] > 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). > > Signed-off-by: Loic Prylli <[EMAIL PROTECTED]> > --- > arch/i386/kernel/cpu/mtrr/main.c |4 > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/arch/i386/kernel/cpu/mtrr/main.c > b/arch/i386/kernel/cpu/mtrr/main.c > index 55b0051..75dc6d5 100644 > --- a/arch/i386/kernel/cpu/mtrr/main.c > +++ b/arch/i386/kernel/cpu/mtrr/main.c > @@ -229,6 +229,8 @@ static void set_mtrr(unsigned int reg, unsigned long base, > data.smp_size = size; > data.smp_type = type; > atomic_set(, num_booting_cpus() - 1); > + /* make sure data.count is visible before unleashing other CPUs */ > + smp_wmb(); > atomic_set(,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(, num_booting_cpus() - 1); > + smp_wmb(); > atomic_set(,1); > > /* do our MTRR business */ > @@ -260,6 +263,7 @@ static void set_mtrr(unsigned int reg, unsigned long base, > cpu_relax(); > > atomic_set(, num_booting_cpus() - 1); > + smp_wmb(); > atomic_set(,0); > > /* - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] 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). Signed-off-by: Loic Prylli <[EMAIL PROTECTED]> --- arch/i386/kernel/cpu/mtrr/main.c |4 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/arch/i386/kernel/cpu/mtrr/main.c b/arch/i386/kernel/cpu/mtrr/main.c index 55b0051..75dc6d5 100644 --- a/arch/i386/kernel/cpu/mtrr/main.c +++ b/arch/i386/kernel/cpu/mtrr/main.c @@ -229,6 +229,8 @@ static void set_mtrr(unsigned int reg, unsigned long base, data.smp_size = size; data.smp_type = type; atomic_set(, num_booting_cpus() - 1); + /* make sure data.count is visible before unleashing other CPUs */ + smp_wmb(); atomic_set(,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(, num_booting_cpus() - 1); + smp_wmb(); atomic_set(,1); /* do our MTRR business */ @@ -260,6 +263,7 @@ static void set_mtrr(unsigned int reg, unsigned long base, cpu_relax(); atomic_set(, num_booting_cpus() - 1); + smp_wmb(); atomic_set(,0); /* -- 1.5.2.2 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] 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). Signed-off-by: Loic Prylli [EMAIL PROTECTED] --- arch/i386/kernel/cpu/mtrr/main.c |4 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/arch/i386/kernel/cpu/mtrr/main.c b/arch/i386/kernel/cpu/mtrr/main.c index 55b0051..75dc6d5 100644 --- a/arch/i386/kernel/cpu/mtrr/main.c +++ b/arch/i386/kernel/cpu/mtrr/main.c @@ -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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] MTRR: Fix race causing set_mtrr to go into infinite loop
On 06/25/2007 05:38 PM, Loic Prylli wrote: [cc: Andi] 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). Signed-off-by: Loic Prylli [EMAIL PROTECTED] --- arch/i386/kernel/cpu/mtrr/main.c |4 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/arch/i386/kernel/cpu/mtrr/main.c b/arch/i386/kernel/cpu/mtrr/main.c index 55b0051..75dc6d5 100644 --- a/arch/i386/kernel/cpu/mtrr/main.c +++ b/arch/i386/kernel/cpu/mtrr/main.c @@ -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); /* - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] MTRR: Fix race causing set_mtrr to go into infinite loop
On Tuesday 26 June 2007 00:05:17 Chuck Ebbert wrote: On 06/25/2007 05:38 PM, Loic Prylli wrote: [cc: Andi] 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). 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] MTRR: Fix race causing set_mtrr to go into infinite loop
On 6/25/2007 6:34 PM, Andi Kleen wrote: On Tuesday 26 June 2007 00:05:17 Chuck Ebbert wrote: On 06/25/2007 05:38 PM, Loic Prylli wrote: [cc: Andi] 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). 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 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
