Messages in this thread Patch in this message | | | Date | Tue, 7 Jun 2005 08:40:32 -0700 | From | Ashok Raj <> | Subject | Re: [patch 4/5] try2: x86_64: Dont use broadcast shortcut to make it cpu hotplug safe. |
| |
On Tue, Jun 07, 2005 at 03:13:30PM +0800, Shaohua Li wrote: > With the patch. smp_call_function still has race. It accesses > cpu_online_map twice. First calculate online cpu counter and second, > send the ipi, so it's not atomic. We should do something like this: > > Thanks, > Shaohua >
Correct, i though this was taken care earlier because i was holding call_lock but i forgot that i just removed it based on zwane's feedback.
I re-introduced that with the comment so i dont forget the purpose.
attached patch should fix that by holding call_lock before setting, so we exclude current upcomming cpu from on-going smp_call_function() transactions.
x86_64-hold-call-lock-when-setting-online-map:
Need to hold call_lock when setting cpu_online_map for a new cpu. __smp_call_function() reads num_cpus_online() to find out how many consumers to wait. These counts are done at different times, and unless we keep writes off cpu_online_map gaurded, these counts could be different. Worst case a new cpu would also participate, this basically keeps the new cpu off currently ongoing smp_call_functions().
Signed-off-by: Ashok Raj <ashok.raj@intel.com> --------------------------------------------------- arch/x86_64/kernel/smp.c | 10 ++++++++++ arch/x86_64/kernel/smpboot.c | 12 ++++++++++++ include/asm-x86_64/smp.h | 2 ++ 3 files changed, 24 insertions(+)
Index: linux-2.6.12-rc6-mm1/arch/x86_64/kernel/smp.c =================================================================== --- linux-2.6.12-rc6-mm1.orig/arch/x86_64/kernel/smp.c +++ linux-2.6.12-rc6-mm1/arch/x86_64/kernel/smp.c @@ -283,6 +283,16 @@ struct call_data_struct { static struct call_data_struct * call_data; +void lock_ipi_call_lock(void) +{ + spin_lock_irq(&call_lock); +} + +void unlock_ipi_call_lock(void) +{ + spin_unlock_irq(&call_lock); +} + /* * this function sends a 'generic call function' IPI to all other CPUs * in the system. Index: linux-2.6.12-rc6-mm1/arch/x86_64/kernel/smpboot.c =================================================================== --- linux-2.6.12-rc6-mm1.orig/arch/x86_64/kernel/smpboot.c +++ linux-2.6.12-rc6-mm1/arch/x86_64/kernel/smpboot.c @@ -448,9 +448,21 @@ void __cpuinit start_secondary(void) enable_APIC_timer(); /* + * We need to hold call_lock, so there is no inconsistency + * between the time smp_call_function() determines number of + * IPI receipients, and the time when the determination is made + * for which cpus receive the IPI in genapic_flat.c. Holding this + * lock helps us to not include this cpu in a currently in progress + * smp_call_function(). + */ + lock_ipi_call_lock(); + + /* * Allow the master to continue. */ cpu_set(smp_processor_id(), cpu_online_map); + unlock_ipi_call_lock(); + mb(); /* Wait for TSC sync to not schedule things before. Index: linux-2.6.12-rc6-mm1/include/asm-x86_64/smp.h =================================================================== --- linux-2.6.12-rc6-mm1.orig/include/asm-x86_64/smp.h +++ linux-2.6.12-rc6-mm1/include/asm-x86_64/smp.h @@ -43,6 +43,8 @@ extern cpumask_t cpu_callout_map; extern void smp_alloc_memory(void); extern volatile unsigned long smp_invalidate_needed; extern int pic_mode; +extern void lock_ipi_call_lock(void); +extern void unlock_ipi_call_lock(void); extern int smp_num_siblings; extern void smp_flush_tlb(void); extern void smp_message_irq(int cpl, void *dev_id, struct pt_regs *regs); - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |