Skip to contentSkip to conversation

[2.5] fix preempt-issues with smp_call_function()

PATCH In review Thomas Schlichter 5 participants 13 messages Feb 25, 2003 – Feb 26, 2003

Subsystems (6)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 [2.5] fix preempt-issues with smp_call_function()

Hello,

here is a patch to solve all (I hope I missed none) possible problems that could occur on SMP machines running a preemptible kernel when smp_call_function() calls a function which should be also executed on the current processor.

This patch is based on the one Dave Jones sent to the LKML last friday and applies to the linux kernel version 2.5.63.

Thank you for any response...

Thomas Schlichter

arch/alpha/kernel/process.c +1 -4
diff -urP linux-2.5.63/arch/alpha/kernel/process.c linux-2.5.63_patched/arch/alpha/kernel/process.c
@@ -155,10 +155,7 @@
 	struct halt_info args;
 	args.mode = mode;
 	args.restart_cmd = restart_cmd;
-#ifdef CONFIG_SMP
-	smp_call_function(common_shutdown_1, &args, 1, 0);
-#endif
-	common_shutdown_1(&args);
+	on_each_cpu(common_shutdown_1, &args, 1, 0);
 }
 
 void
arch/alpha/kernel/smp.c +17 -6
diff -urP linux-2.5.63/arch/alpha/kernel/smp.c linux-2.5.63_patched/arch/alpha/kernel/smp.c
@@ -899,10 +899,8 @@
 smp_imb(void)
 {
 	/* Must wait other processors to flush their icache before continue. */
-	if (smp_call_function(ipi_imb, NULL, 1, 1))
+	if (on_each_cpu(ipi_imb, NULL, 1, 1))
 		printk(KERN_CRIT "smp_imb: timed out\n");
-
-	imb();
 }
 
 static void
@@ -916,11 +914,9 @@
 {
 	/* Although we don't have any data to pass, we do want to
 	   synchronize with the other processors.  */
-	if (smp_call_function(ipi_flush_tlb_all, NULL, 1, 1)) {
+	if (on_each_cpu(ipi_flush_tlb_all, NULL, 1, 1)) {
 		printk(KERN_CRIT "flush_tlb_all: timed out\n");
 	}
-
-	tbia();
 }
 
 #define asn_locked() (cpu_data[smp_processor_id()].asn_lock)
@@ -938,6 +934,8 @@
 void
 flush_tlb_mm(struct mm_struct *mm)
 {
+	preempt_disable();
+
 	if (mm == current->active_mm) {
 		flush_tlb_current(mm);
 		if (atomic_read(&mm->mm_users) <= 1) {
@@ -948,6 +946,7 @@
 				if (mm->context[cpu])
 					mm->context[cpu] = 0;
 			}
+			preempt_enable();
 			return;
 		}
 	}
@@ -955,6 +954,8 @@
 	if (smp_call_function(ipi_flush_tlb_mm, mm, 1, 1)) {
 		printk(KERN_CRIT "flush_tlb_mm: timed out\n");
 	}
+
+	preempt_enable();
 }
 
 struct flush_tlb_page_struct {
@@ -981,6 +982,8 @@
 	struct flush_tlb_page_struct data;
 	struct mm_struct *mm = vma->vm_mm;
 
+	preempt_disable();
+
 	if (mm == current->active_mm) {
 		flush_tlb_current_page(mm, vma, addr);
 		if (atomic_read(&mm->mm_users) <= 1) {
@@ -991,6 +994,7 @@
 				if (mm->context[cpu])
 					mm->context[cpu] = 0;
 			}
+			preempt_enable();
 			return;
 		}
 	}
@@ -1002,6 +1006,8 @@
 	if (smp_call_function(ipi_flush_tlb_page, &data, 1, 1)) {
 		printk(KERN_CRIT "flush_tlb_page: timed out\n");
 	}
+
+	preempt_enable();
 }
 
 void
@@ -1030,6 +1036,8 @@
 	if ((vma->vm_flags & VM_EXEC) == 0)
 		return;
 
+	preempt_disable();
+
 	if (mm == current->active_mm) {
 		__load_new_mm_context(mm);
 		if (atomic_read(&mm->mm_users) <= 1) {
@@ -1040,6 +1048,7 @@
 				if (mm->context[cpu])
 					mm->context[cpu] = 0;
 			}
+			preempt_enable();
 			return;
 		}
 	}
@@ -1047,6 +1056,8 @@
 	if (smp_call_function(ipi_flush_icache_page, mm, 1, 1)) {
 		printk(KERN_CRIT "flush_icache_page: timed out\n");
 	}
+
+	preempt_enable();
 }
 
 #ifdef CONFIG_DEBUG_SPINLOCK
arch/i386/kernel/io_apic.c +2 -4
diff -urP linux-2.5.63/arch/i386/kernel/io_apic.c linux-2.5.63_patched/arch/i386/kernel/io_apic.c
@@ -1376,8 +1376,7 @@
 
 void print_all_local_APICs (void)
 {
-	smp_call_function(print_local_APIC, NULL, 1, 1);
-	print_local_APIC(NULL);
+	on_each_cpu(print_local_APIC, NULL, 1, 1);
 }
 
 void /*__init*/ print_PIC(void)
@@ -1843,8 +1842,7 @@
 	 */ 
 	printk(KERN_INFO "activating NMI Watchdog ...");
 
-	smp_call_function(enable_NMI_through_LVT0, NULL, 1, 1);
-	enable_NMI_through_LVT0(NULL);
+	on_each_cpu(enable_NMI_through_LVT0, NULL, 1, 1);
 
 	printk(" done.\n");
 }
arch/i386/kernel/ldt.c +2 -2
diff -urP linux-2.5.63/arch/i386/kernel/ldt.c linux-2.5.63_patched/arch/i386/kernel/ldt.c
@@ -55,13 +55,13 @@
 	wmb();
 
 	if (reload) {
+		preempt_disable();
 		load_LDT(pc);
 #ifdef CONFIG_SMP
-		preempt_disable();
 		if (current->mm->cpu_vm_mask != (1 << smp_processor_id()))
 			smp_call_function(flush_ldt, 0, 1, 1);
-		preempt_enable();
 #endif
+		preempt_enable();
 	}
 	if (oldsize) {
 		if (oldsize*LDT_ENTRY_SIZE > PAGE_SIZE)
arch/i386/kernel/microcode.c +1 -2
diff -urP linux-2.5.63/arch/i386/kernel/microcode.c linux-2.5.63_patched/arch/i386/kernel/microcode.c
@@ -183,11 +183,10 @@
 	int i, error = 0, err;
 	struct microcode *m;
 
-	if (smp_call_function(do_update_one, NULL, 1, 1) != 0) {
+	if (on_each_cpu(do_update_one, NULL, 1, 1) != 0) {
 		printk(KERN_ERR "microcode: IPI timeout, giving up\n");
 		return -EIO;
 	}
-	do_update_one(NULL);
 
 	for (i=0; i<NR_CPUS; i++) {
 		err = update_req[i].err;
arch/i386/kernel/smp.c +2 -11
diff -urP linux-2.5.63/arch/i386/kernel/smp.c linux-2.5.63_patched/arch/i386/kernel/smp.c
@@ -436,7 +436,7 @@
 	preempt_enable();
 }
 
-static inline void do_flush_tlb_all_local(void)
+static void do_flush_tlb_all(void* info)
 {
 	unsigned long cpu = smp_processor_id();
 
@@ -445,18 +445,9 @@
 		leave_mm(cpu);
 }
 
-static void flush_tlb_all_ipi(void* info)
-{
-	do_flush_tlb_all_local();
-}
-
 void flush_tlb_all(void)
 {
-	preempt_disable();
-	smp_call_function (flush_tlb_all_ipi,0,1,1);
-
-	do_flush_tlb_all_local();
-	preempt_enable();
+	on_each_cpu(do_flush_tlb_all, 0, 1, 1);
 }
 
 /*
arch/i386/kernel/sysenter.c +1 -2
diff -urP linux-2.5.63/arch/i386/kernel/sysenter.c linux-2.5.63_patched/arch/i386/kernel/sysenter.c
@@ -95,8 +95,7 @@
 		return 0;
 
 	memcpy((void *) page, sysent, sizeof(sysent));
-	enable_sep_cpu(NULL);
-	smp_call_function(enable_sep_cpu, NULL, 1, 1);
+	on_each_cpu(enable_sep_cpu, NULL, 1, 1);
 	return 0;
 }
 
arch/i386/mach-voyager/voyager_smp.c +3 -11
diff -urP linux-2.5.63/arch/i386/mach-voyager/voyager_smp.c linux-2.5.63_patched/arch/i386/mach-voyager/voyager_smp.c
@@ -1209,8 +1209,8 @@
 		smp_call_function_interrupt();
 }
 
-static inline void
-do_flush_tlb_all_local(void)
+static void
+do_flush_tlb_all(void* info)
 {
 	unsigned long cpu = smp_processor_id();
 
@@ -1220,19 +1220,11 @@
 }
 
 
-static void
-flush_tlb_all_function(void* info)
-{
-	do_flush_tlb_all_local();
-}
-
 /* flush the TLB of every active CPU in the system */
 void
 flush_tlb_all(void)
 {
-	smp_call_function (flush_tlb_all_function, 0, 1, 1);
-
-	do_flush_tlb_all_local();
+	on_each_cpu(do_flush_tlb_all, 0, 1, 1);
 }
 
 /* used to set up the trampoline for other CPUs when the memory manager
arch/i386/mm/pageattr.c +2 -5
diff -urP linux-2.5.63/arch/i386/mm/pageattr.c linux-2.5.63_patched/arch/i386/mm/pageattr.c
@@ -130,11 +130,8 @@
 } 
 
 static inline void flush_map(void)
-{	
-#ifdef CONFIG_SMP 
-	smp_call_function(flush_kernel_map, NULL, 1, 1);
-#endif	
-	flush_kernel_map(NULL);
+{
+	on_each_cpu(flush_kernel_map, NULL, 1, 1);
 }
 
 struct deferred_page { 
arch/i386/oprofile/nmi_int.c +4 -8
diff -urP linux-2.5.63/arch/i386/oprofile/nmi_int.c linux-2.5.63_patched/arch/i386/oprofile/nmi_int.c
@@ -95,8 +95,7 @@
 	 * without actually triggering any NMIs as this will
 	 * break the core code horrifically.
 	 */
-	smp_call_function(nmi_cpu_setup, NULL, 0, 1);
-	nmi_cpu_setup(0);
+	on_each_cpu(nmi_cpu_setup, NULL, 0, 1);
 	set_nmi_callback(nmi_callback);
 	oprofile_pmdev = set_nmi_pm_callback(oprofile_pm_callback);
 	return 0;
@@ -148,8 +147,7 @@
 {
 	unset_nmi_pm_callback(oprofile_pmdev);
 	unset_nmi_callback();
-	smp_call_function(nmi_cpu_shutdown, NULL, 0, 1);
-	nmi_cpu_shutdown(0);
+	on_each_cpu(nmi_cpu_shutdown, NULL, 0, 1);
 }
 
  
@@ -162,8 +160,7 @@
 
 static int nmi_start(void)
 {
-	smp_call_function(nmi_cpu_start, NULL, 0, 1);
-	nmi_cpu_start(0);
+	on_each_cpu(nmi_cpu_start, NULL, 0, 1);
 	return 0;
 }
  
@@ -177,8 +174,7 @@
  
 static void nmi_stop(void)
 {
-	smp_call_function(nmi_cpu_stop, NULL, 0, 1);
-	nmi_cpu_stop(0);
+	on_each_cpu(nmi_cpu_stop, NULL, 0, 1);
 }
 
 
arch/ia64/kernel/smp.c +5 -5
diff -urP linux-2.5.63/arch/ia64/kernel/smp.c linux-2.5.63_patched/arch/ia64/kernel/smp.c
@@ -206,18 +206,18 @@
 void
 smp_flush_tlb_all (void)
 {
-	smp_call_function((void (*)(void *))local_flush_tlb_all, 0, 1, 1);
-	local_flush_tlb_all();
+	on_each_cpu((void (*)(void *))local_flush_tlb_all, 0, 1, 1);
 }
 
 void
 smp_flush_tlb_mm (struct mm_struct *mm)
 {
-	local_finish_flush_tlb_mm(mm);
-
 	/* this happens for the common case of a single-threaded fork():  */
 	if (likely(mm == current->active_mm && atomic_read(&mm->mm_users) == 1))
+	{
+		local_finish_flush_tlb_mm(mm);
 		return;
+	}
 
 	/*
 	 * We could optimize this further by using mm->cpu_vm_mask to track which CPUs
@@ -226,7 +226,7 @@
 	 * anyhow, and once a CPU is interrupted, the cost of local_flush_tlb_all() is
 	 * rather trivial.
 	 */
-	smp_call_function((void (*)(void *))local_finish_flush_tlb_mm, mm, 1, 1);
+	on_each_cpu((void (*)(void *))local_finish_flush_tlb_mm, mm, 1, 1);
 }
 
 /*
arch/mips64/kernel/smp.c +13 -2
diff -urP linux-2.5.63/arch/mips64/kernel/smp.c linux-2.5.63_patched/arch/mips64/kernel/smp.c
@@ -195,8 +195,7 @@
 
 void flush_tlb_all(void)
 {
-	smp_call_function(flush_tlb_all_ipi, 0, 1, 1);
-	_flush_tlb_all();
+	on_each_cpu(flush_tlb_all_ipi, 0, 1, 1);
 }
 
 static void flush_tlb_mm_ipi(void *mm)
@@ -219,6 +218,8 @@
 
 void flush_tlb_mm(struct mm_struct *mm)
 {
+	preempt_disable();
+
 	if ((atomic_read(&mm->mm_users) != 1) || (current->mm != mm)) {
 		smp_call_function(flush_tlb_mm_ipi, (void *)mm, 1, 1);
 	} else {
@@ -228,6 +229,8 @@
 				CPU_CONTEXT(i, mm) = 0;
 	}
 	_flush_tlb_mm(mm);
+
+	preempt_enable();
 }
 
 struct flush_tlb_data {
@@ -246,6 +249,8 @@
 
 void flush_tlb_range(struct vm_area_struct *vma, unsigned long start, unsigned long end)
 {
+	preempt_disable();
+
 	if ((atomic_read(&mm->mm_users) != 1) || (current->mm != mm)) {
 		struct flush_tlb_data fd;
 
@@ -260,6 +265,8 @@
 				CPU_CONTEXT(i, mm) = 0;
 	}
 	_flush_tlb_range(mm, start, end);
+
+	preempt_enable();
 }
 
 static void flush_tlb_page_ipi(void *info)
@@ -271,6 +278,8 @@
 
 void flush_tlb_page(struct vm_area_struct *vma, unsigned long page)
 {
+	preempt_disable();
+
 	if ((atomic_read(&vma->vm_mm->mm_users) != 1) || (current->mm != vma->vm_mm)) {
 		struct flush_tlb_data fd;
 
@@ -284,5 +293,7 @@
 				CPU_CONTEXT(i, vma->vm_mm) = 0;
 	}
 	_flush_tlb_page(vma, page);
+
+	preempt_enable();
 }
 
arch/parisc/kernel/cache.c +1 -2
diff -urP linux-2.5.63/arch/parisc/kernel/cache.c linux-2.5.63_patched/arch/parisc/kernel/cache.c
@@ -39,8 +39,7 @@
 void
 flush_data_cache(void)
 {
-	smp_call_function((void (*)(void *))flush_data_cache_local, NULL, 1, 1);
-	flush_data_cache_local();
+	on_each_cpu((void (*)(void *))flush_data_cache_local, NULL, 1, 1);
 }
 #endif
 
arch/parisc/kernel/irq.c +3 -8
diff -urP linux-2.5.63/arch/parisc/kernel/irq.c linux-2.5.63_patched/arch/parisc/kernel/irq.c
@@ -61,20 +61,17 @@
 
 static spinlock_t irq_lock = SPIN_LOCK_UNLOCKED;  /* protect IRQ regions */
 
-#ifdef CONFIG_SMP
 static void cpu_set_eiem(void *info)
 {
 	set_eiem((unsigned long) info);
 }
-#endif
 
 static inline void disable_cpu_irq(void *unused, int irq)
 {
 	unsigned long eirr_bit = EIEM_MASK(irq);
 
 	cpu_eiem &= ~eirr_bit;
-	set_eiem(cpu_eiem);
-        smp_call_function(cpu_set_eiem, (void *) cpu_eiem, 1, 1);
+        on_each_cpu(cpu_set_eiem, (void *) cpu_eiem, 1, 1);
 }
 
 static void enable_cpu_irq(void *unused, int irq)
@@ -83,8 +80,7 @@
 
 	mtctl(eirr_bit, 23);	/* clear EIRR bit before unmasking */
 	cpu_eiem |= eirr_bit;
-        smp_call_function(cpu_set_eiem, (void *) cpu_eiem, 1, 1);
-	set_eiem(cpu_eiem);
+        on_each_cpu(cpu_set_eiem, (void *) cpu_eiem, 1, 1);
 }
 
 /* mask and disable are the same at the CPU level
@@ -100,8 +96,7 @@
 	** handle *any* unmasked pending interrupts.
 	** ie We don't need to check for pending interrupts here.
 	*/
-        smp_call_function(cpu_set_eiem, (void *) cpu_eiem, 1, 1);
-	set_eiem(cpu_eiem);
+        on_each_cpu(cpu_set_eiem, (void *) cpu_eiem, 1, 1);
 }
 
 /*
arch/parisc/kernel/smp.c +2 -3
diff -urP linux-2.5.63/arch/parisc/kernel/smp.c linux-2.5.63_patched/arch/parisc/kernel/smp.c
@@ -401,7 +401,7 @@
 __setup("maxcpus=", maxcpus);
 
 /*
- * Flush all other CPU's tlb and then mine.  Do this with smp_call_function()
+ * Flush all other CPU's tlb and then mine.  Do this with on_each_cpu()
  * as we want to ensure all TLB's flushed before proceeding.
  */
 
@@ -410,8 +410,7 @@
 void
 smp_flush_tlb_all(void)
 {
-	smp_call_function((void (*)(void *))flush_tlb_all_local, NULL, 1, 1);
-	flush_tlb_all_local();
+	on_each_cpu((void (*)(void *))flush_tlb_all_local, NULL, 1, 1);
 }
 
 
arch/parisc/mm/init.c +1 -2
diff -urP linux-2.5.63/arch/parisc/mm/init.c linux-2.5.63_patched/arch/parisc/mm/init.c
@@ -974,8 +974,7 @@
 	    do_recycle++;
 	}
 	spin_unlock(&sid_lock);
-	smp_call_function((void (*)(void *))flush_tlb_all_local, NULL, 1, 1);
-	flush_tlb_all_local();
+	on_each_cpu((void (*)(void *))flush_tlb_all_local, NULL, 1, 1);
 	if (do_recycle) {
 	    spin_lock(&sid_lock);
 	    recycle_sids(recycle_ndirty,recycle_dirty_array);
arch/ppc/kernel/temp.c +2 -8
diff -urP linux-2.5.63/arch/ppc/kernel/temp.c linux-2.5.63_patched/arch/ppc/kernel/temp.c
@@ -194,10 +194,7 @@
 
 	/* schedule ourselves to be run again */
 	mod_timer(&tau_timer, jiffies + shrink_timer) ;
-#ifdef CONFIG_SMP
-	smp_call_function(tau_timeout, NULL, 1, 0);
-#endif
-	tau_timeout(NULL);
+	on_each_cpu(tau_timeout, NULL, 1, 0);
 }
 
 /*
@@ -239,10 +236,7 @@
 	tau_timer.expires = jiffies + shrink_timer;
 	add_timer(&tau_timer);
 	
-#ifdef CONFIG_SMP
-	smp_call_function(TAU_init_smp, NULL, 1, 0);
-#endif
-	TAU_init_smp(NULL);
+	on_each_cpu(TAU_init_smp, NULL, 1, 0);
 	
 	printk("Thermal assist unit ");
 #ifdef CONFIG_TAU_INT
arch/s390/kernel/smp.c +6 -12
diff -urP linux-2.5.63/arch/s390/kernel/smp.c linux-2.5.63_patched/arch/s390/kernel/smp.c
@@ -228,8 +228,7 @@
 void machine_restart_smp(char * __unused) 
 {
 	cpu_restart_map = cpu_online_map;
-        smp_call_function(do_machine_restart, NULL, 0, 0);
-	do_machine_restart(NULL);
+        on_each_cpu(do_machine_restart, NULL, 0, 0);
 }
 
 static void do_machine_halt(void * __unused)
@@ -247,8 +246,7 @@
 
 void machine_halt_smp(void)
 {
-        smp_call_function(do_machine_halt, NULL, 0, 0);
-	do_machine_halt(NULL);
+        on_each_cpu(do_machine_halt, NULL, 0, 0);
 }
 
 static void do_machine_power_off(void * __unused)
@@ -266,8 +264,7 @@
 
 void machine_power_off_smp(void)
 {
-        smp_call_function(do_machine_power_off, NULL, 0, 0);
-	do_machine_power_off(NULL);
+        on_each_cpu(do_machine_power_off, NULL, 0, 0);
 }
 
 /*
@@ -339,8 +336,7 @@
 
 void smp_ptlb_all(void)
 {
-        smp_call_function(smp_ptlb_callback, NULL, 0, 1);
-	local_flush_tlb();
+        on_each_cpu(smp_ptlb_callback, NULL, 0, 1);
 }
 
 /*
@@ -400,8 +396,7 @@
 	parms.end_ctl = cr;
 	parms.orvals[cr] = 1 << bit;
 	parms.andvals[cr] = 0xFFFFFFFF;
-	smp_call_function(smp_ctl_bit_callback, &parms, 0, 1);
-        __ctl_set_bit(cr, bit);
+	on_each_cpu(smp_ctl_bit_callback, &parms, 0, 1);
 }
 
 /*
@@ -414,8 +409,7 @@
 	parms.end_ctl = cr;
 	parms.orvals[cr] = 0x00000000;
 	parms.andvals[cr] = ~(1 << bit);
-	smp_call_function(smp_ctl_bit_callback, &parms, 0, 1);
-        __ctl_clear_bit(cr, bit);
+	on_each_cpu(smp_ctl_bit_callback, &parms, 0, 1);
 }
 
 /*
arch/s390x/kernel/smp.c +5 -10
diff -urP linux-2.5.63/arch/s390x/kernel/smp.c linux-2.5.63_patched/arch/s390x/kernel/smp.c
@@ -227,8 +227,7 @@
 void machine_restart_smp(char * __unused) 
 {
 	cpu_restart_map = cpu_online_map;
-        smp_call_function(do_machine_restart, NULL, 0, 0);
-	do_machine_restart(NULL);
+        on_each_cpu(do_machine_restart, NULL, 0, 0);
 }
 
 static void do_machine_halt(void * __unused)
@@ -246,8 +245,7 @@
 
 void machine_halt_smp(void)
 {
-        smp_call_function(do_machine_halt, NULL, 0, 0);
-	do_machine_halt(NULL);
+        on_each_cpu(do_machine_halt, NULL, 0, 0);
 }
 
 static void do_machine_power_off(void * __unused)
@@ -265,8 +263,7 @@
 
 void machine_power_off_smp(void)
 {
-        smp_call_function(do_machine_power_off, NULL, 0, 0);
-	do_machine_power_off(NULL);
+        on_each_cpu(do_machine_power_off, NULL, 0, 0);
 }
 
 /*
@@ -383,8 +380,7 @@
 	parms.end_ctl = cr;
 	parms.orvals[cr] = 1 << bit;
 	parms.andvals[cr] = -1L;
-	smp_call_function(smp_ctl_bit_callback, &parms, 0, 1);
-        __ctl_set_bit(cr, bit);
+	on_each_cpu(smp_ctl_bit_callback, &parms, 0, 1);
 }
 
 /*
@@ -397,8 +393,7 @@
 	parms.end_ctl = cr;
 	parms.orvals[cr] = 0;
 	parms.andvals[cr] = ~(1L << bit);
-	smp_call_function(smp_ctl_bit_callback, &parms, 0, 1);
-        __ctl_clear_bit(cr, bit);
+	on_each_cpu(smp_ctl_bit_callback, &parms, 0, 1);
 }
 
 
arch/x86_64/kernel/bluesmoke.c +1 -13
diff -urP linux-2.5.63/arch/x86_64/kernel/bluesmoke.c linux-2.5.63_patched/arch/x86_64/kernel/bluesmoke.c
@@ -111,11 +111,7 @@
 {
 	u32 low, high;
 	int i;
-	unsigned int *cpu = info;
 
-	BUG_ON (*cpu != smp_processor_id());
-
-	preempt_disable();
 	for (i=0; i<banks; i++) {
 		rdmsr(MSR_IA32_MC0_STATUS+i*4, low, high);
 
@@ -130,20 +126,12 @@
 			wmb();
 		}
 	}
-	preempt_enable();
 }
 
 
 static void mce_timerfunc (unsigned long data)
 {
-	unsigned int i;
-
-	for (i=0; i<smp_num_cpus; i++) {
-		if (i == smp_processor_id())
-			mce_checkregs(&i);
-		else
-			smp_call_function (mce_checkregs, &i, 1, 1);
-	}
+	on_each_cpu (mce_checkregs, NULL, 1, 1);
 
 	/* Refresh the timer. */
 	mce_timer.expires = jiffies + MCE_RATE;
arch/x86_64/kernel/io_apic.c +1 -2
diff -urP linux-2.5.63/arch/x86_64/kernel/io_apic.c linux-2.5.63_patched/arch/x86_64/kernel/io_apic.c
@@ -928,8 +928,7 @@
 
 void print_all_local_APICs (void)
 {
-	smp_call_function(print_local_APIC, NULL, 1, 1);
-	print_local_APIC(NULL);
+	on_each_cpu(print_local_APIC, NULL, 1, 1);
 }
 
 void /*__init*/ print_PIC(void)
arch/x86_64/kernel/ldt.c +2 -2
diff -urP linux-2.5.63/arch/x86_64/kernel/ldt.c linux-2.5.63_patched/arch/x86_64/kernel/ldt.c
@@ -60,13 +60,13 @@
 	pc->size = mincount;
 	wmb();
 	if (reload) {
+		preempt_disable();
 		load_LDT(pc);
 #ifdef CONFIG_SMP
-		preempt_disable();
 		if (current->mm->cpu_vm_mask != (1<<smp_processor_id()))
 			smp_call_function(flush_ldt, 0, 1, 1);
-		preempt_enable();
 #endif
+		preempt_enable();
 	}
 	if (oldsize) {
 		if (oldsize*LDT_ENTRY_SIZE > PAGE_SIZE)
arch/x86_64/kernel/smp.c +2 -11
diff -urP linux-2.5.63/arch/x86_64/kernel/smp.c linux-2.5.63_patched/arch/x86_64/kernel/smp.c
@@ -328,7 +328,7 @@
 	preempt_enable();
 }
 
-static inline void do_flush_tlb_all_local(void)
+static void do_flush_tlb_all(void* info)
 {
 	unsigned long cpu = smp_processor_id();
 
@@ -337,18 +337,9 @@
 		leave_mm(cpu);
 }
 
-static void flush_tlb_all_ipi(void* info)
-{
-	do_flush_tlb_all_local();
-}
-
 void flush_tlb_all(void)
 {
-	preempt_disable();
-	smp_call_function (flush_tlb_all_ipi,0,1,1);
-
-	do_flush_tlb_all_local();
-	preempt_enable();
+	on_each_cpu(do_flush_tlb_all, 0, 1, 1);
 }
 
 void smp_kdb_stop(void)
arch/x86_64/mm/pageattr.c +1 -6
diff -urP linux-2.5.63/arch/x86_64/mm/pageattr.c linux-2.5.63_patched/arch/x86_64/mm/pageattr.c
@@ -123,12 +123,7 @@
 
 static inline void flush_map(unsigned long address)
 {	
-	preempt_disable();
-#ifdef CONFIG_SMP 
-	smp_call_function(flush_kernel_map, (void *)address, 1, 1);
-#endif	
-	flush_kernel_map((void *)address);
-	preempt_enable();
+	on_each_cpu(flush_kernel_map, (void *)address, 1, 1);
 }
 
 struct deferred_page { 
drivers/char/agp/agp.h +1 -2
diff -urP linux-2.5.63/drivers/char/agp/agp.h linux-2.5.63_patched/drivers/char/agp/agp.h
@@ -42,9 +42,8 @@
 
 static void __attribute__((unused)) global_cache_flush(void)
 {
-	if (smp_call_function(ipi_handler, NULL, 1, 1) != 0)
+	if (on_each_cpu(ipi_handler, NULL, 1, 1) != 0)
 		panic(PFX "timed out waiting for the other CPUs!\n");
-	flush_agp_cache();
 }
 #else
 static inline void global_cache_flush(void)
drivers/s390/char/sclp.c +1 -2
diff -urP linux-2.5.63/drivers/s390/char/sclp.c linux-2.5.63_patched/drivers/s390/char/sclp.c
@@ -481,8 +481,7 @@
 do_machine_quiesce(void)
 {
 	cpu_quiesce_map = cpu_online_map;
-	smp_call_function(do_load_quiesce_psw, NULL, 0, 0);
-	do_load_quiesce_psw(NULL);
+	on_each_cpu(do_load_quiesce_psw, NULL, 0, 0);
 }
 #else
 static void
fs/buffer.c +1 -4
diff -urP linux-2.5.63/fs/buffer.c linux-2.5.63_patched/fs/buffer.c
@@ -1403,10 +1403,7 @@
 	
 static void invalidate_bh_lrus(void)
 {
-	preempt_disable();
-	invalidate_bh_lru(NULL);
-	smp_call_function(invalidate_bh_lru, NULL, 1, 1);
-	preempt_enable();
+	on_each_cpu(invalidate_bh_lru, NULL, 1, 1);
 }
 
 void set_bh_page(struct buffer_head *bh,
include/asm-parisc/cacheflush.h +1 -7
diff -urP linux-2.5.63/include/asm-parisc/cacheflush.h linux-2.5.63_patched/include/asm-parisc/cacheflush.h
@@ -25,16 +25,10 @@
 
 extern void flush_cache_all_local(void);
 
-#ifdef CONFIG_SMP
 static inline void flush_cache_all(void)
 {
-	smp_call_function((void (*)(void *))flush_cache_all_local, NULL, 1, 1);
-	flush_cache_all_local();
+	on_each_cpu((void (*)(void *))flush_cache_all_local, NULL, 1, 1);
 }
-#else
-#define flush_cache_all flush_cache_all_local
-#endif
-
 
 /* The following value needs to be tuned and probably scaled with the
  * cache size.
include/linux/smp.h +22 -1
diff -urP linux-2.5.63/include/linux/smp.h linux-2.5.63_patched/include/linux/smp.h
@@ -10,9 +10,10 @@
 
 #ifdef CONFIG_SMP
 
+#include <linux/preempt.h>
 #include <linux/kernel.h>
 #include <linux/compiler.h>
-#include <linux/threads.h>
+#include <linux/thread_info.h>
 #include <asm/smp.h>
 #include <asm/bug.h>
 
@@ -54,6 +55,25 @@
 			      int retry, int wait);
 
 /*
+ * Call a function on all processors
+ */
+static inline int on_each_cpu(void (*func) (void *info), void *info,
+			      int retry, int wait)
+{
+	int ret = 0;
+
+	preempt_disable();
+
+	if(num_online_cpus() > 1)
+		ret = smp_call_function(func, info, retry, wait);
+	func(info);
+
+	preempt_enable();
+
+	return ret;
+}
+
+/*
  * True once the per process idle is forked
  */
 extern int smp_threads_ready;
@@ -96,6 +116,7 @@
 #define hard_smp_processor_id()			0
 #define smp_threads_ready			1
 #define smp_call_function(func,info,retry,wait)	({ 0; })
+#define on_each_cpu(func,info,retry,wait)	({ func(info); 0; })
 static inline void smp_send_reschedule(int cpu) { }
 static inline void smp_send_reschedule_all(void) { }
 #define cpu_online_map				1
mm/slab.c +4 -0
diff -urP linux-2.5.63/mm/slab.c linux-2.5.63_patched/mm/slab.c
@@ -1116,12 +1116,16 @@
 static void smp_call_function_all_cpus(void (*func) (void *arg), void *arg)
 {
 	check_irq_on();
+	preempt_disable();
+
 	local_irq_disable();
 	func(arg);
 	local_irq_enable();
 
 	if (smp_call_function(func, arg, 1, 1))
 		BUG();
+
+	preempt_enable();
 }
 
 static void free_block (kmem_cache_t* cachep, void** objpp, int len);
#1 lore raw
Quoted 8 lines · Thomas Schlichter

Hello,

here is a patch to solve all (I hope I missed none) possible problems that could occur on SMP machines running a preemptible kernel when smp_call_function() calls a function which should be also executed on the current processor.

Patch looks pretty good, thanks. Fixes a real bug.

I worry a little about the s390/s390x change. smp_ctl_set_bit() and smp_ctl_clear_bit(). The functions which are being called on local and remote are fairly different. I'm sure it's OK, but... I changed that bit to open-code the preempt_disable/enable.

The arch/x86_64/kernel/bluesmoke.c change looks right. Seems that someone didn't understand the smp_call_function() API in there.

#2 lore raw
In reply to Thomas Schlichter
Quoted 7 lines · Thomas Schlichter

here is a patch to solve all (I hope I missed none) possible problems that could occur on SMP machines running a preemptible kernel when smp_call_function() calls a function which should be also executed on the current processor.

This patch is based on the one Dave Jones sent to the LKML last friday and applies to the linux kernel version 2.5.63.

Just one comment. You moved quite a few of the preempt_disable/enable pairs outside of the CONFIG_SMP checks. The issue we're working against here is to try and prevent preemption and ending up on a different CPU. As this cannot happen if CONFIG_SMP=n, I don't see why you've done this.

Dave

#3 lore raw
Quoted 8 lines · Dave Jones
Quoted 5 lines · Thomas Schlichter

here is a patch to solve all (I hope I missed none) possible problems that could occur on SMP machines running a preemptible kernel when smp_call_function() calls a function which should be also executed on the current processor.

Quoted 8 lines
Quoted 2 lines

This patch is based on the one Dave Jones sent to the LKML last friday and applies to the linux kernel version 2.5.63.

Just one comment. You moved quite a few of the preempt_disable/enable pairs outside of the CONFIG_SMP checks. The issue we're working against here is to try and prevent preemption and ending up on a different CPU. As this cannot happen if CONFIG_SMP=n, I don't see why you've done this.

Just in two places.

 arch/i386/kernel/ldt.c   |    6 ++++--
 arch/x86_64/kernel/ldt.c |    6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)
arch/i386/kernel/ldt.c renamed from arch/i386/kernel/ldt.c~on_each_cpu-ldt-cleanup +4 -2
diff -puN arch/i386/kernel/ldt.c~on_each_cpu-ldt-cleanup arch/i386/kernel/ldt.c
@@ -55,13 +55,15 @@ static int alloc_ldt(mm_context_t *pc, i
 	wmb();
 
 	if (reload) {
+#ifdef CONFIG_SMP
 		preempt_disable();
 		load_LDT(pc);
-#ifdef CONFIG_SMP
 		if (current->mm->cpu_vm_mask != (1 << smp_processor_id()))
 			smp_call_function(flush_ldt, 0, 1, 1);
-#endif
 		preempt_enable();
+#else
+		load_LDT(pc);
+#endif
 	}
 	if (oldsize) {
 		if (oldsize*LDT_ENTRY_SIZE > PAGE_SIZE)
arch/x86_64/kernel/ldt.c renamed from arch/x86_64/kernel/ldt.c~on_each_cpu-ldt-cleanup +4 -2
diff -puN arch/x86_64/kernel/ldt.c~on_each_cpu-ldt-cleanup arch/x86_64/kernel/ldt.c
@@ -60,13 +60,15 @@ static int alloc_ldt(mm_context_t *pc, i
 	pc->size = mincount;
 	wmb();
 	if (reload) {
+#ifdef CONFIG_SMP
 		preempt_disable();
 		load_LDT(pc);
-#ifdef CONFIG_SMP
 		if (current->mm->cpu_vm_mask != (1<<smp_processor_id()))
 			smp_call_function(flush_ldt, 0, 1, 1);
-#endif
 		preempt_enable();
+#else
+		load_LDT(pc);
+#endif
 	}
 	if (oldsize) {
 		if (oldsize*LDT_ENTRY_SIZE > PAGE_SIZE)

_

#4 lore raw
Quoted 4 lines · Dave Jones

Just one comment. You moved quite a few of the preempt_disable/enable pairs outside of the CONFIG_SMP checks. The issue we're working against here is to try and prevent preemption and ending up on a different CPU. As this cannot happen if CONFIG_SMP=n, I don't see why you've done this.

Quoted 1 line · Andrew Morton

Just in two places.

[snip]

Yes, thanks for delivering this better patch!
My approach wanted just to be the most simple possibility... ;-)

Thomas

#5 lore raw
In reply to Andrew Morton
Quoted 5 lines · Andrew Morton
Quoted 4 lines

Just one comment. You moved quite a few of the preempt_disable/enable pairs outside of the CONFIG_SMP checks. The issue we're working against here is to try and prevent preemption and ending up on a different CPU. As this cannot happen if CONFIG_SMP=n, I don't see why you've done this.

Just in two places.

Ok, slight exaggeration 8-) Looks good to me.

btw, (unrelated) shouldn't smp_call_function be doing magick checks with cpu_online() ?

Dave

#6 lore raw
Quoted 3 lines · Dave Jones

btw, (unrelated) shouldn't smp_call_function be doing magick checks with cpu_online() ?

Looks OK? It sprays the IPI out to all the other CPUs in cpu_online_map, and waits for num_online_cpus()-1 CPUs to answer.

All very racy in the presence of CPUs going offline, but that's all over the place. Depends how the offlining will be done I guess.

#7 lore raw
Quoted 4 lines · Andrew Morton
Quoted 2 lines

btw, (unrelated) shouldn't smp_call_function be doing magick checks with cpu_online() ?

Looks OK? It sprays the IPI out to all the other CPUs in cpu_online_map, and waits for num_online_cpus()-1 CPUs to answer.

Doh, of course.
Ugh, mornings.

Dave

#8 lore raw
In reply to Andrew Morton
Quoted 2 lines · Dave Jones

btw, (unrelated) shouldn't smp_call_function be doing magick checks with cpu_online() ?

Quoted 5 lines · Andrew Morton

Looks OK? It sprays the IPI out to all the other CPUs in cpu_online_map, and waits for num_online_cpus()-1 CPUs to answer.

All very racy in the presence of CPUs going offline, but that's all over the place. Depends how the offlining will be done I guess.

Well, now I see the check for num_online_cpus() != 1 in smp_call_function(), too, so the check in on_each_cpu() is not needed and possibly better this patch should apply to the include/linux/smp.h file...

Thomas

include/linux/smp.h Wed Feb 26 11:41:45 2003 renamed from include/linux/smp.h.orig Mon Feb 24 20:05:33 2003 +21 -1
@@ -10,9 +10,10 @@
 
 #ifdef CONFIG_SMP
 
+#include <linux/preempt.h>
 #include <linux/kernel.h>
 #include <linux/compiler.h>
-#include <linux/threads.h>
+#include <linux/thread_info.h>
 #include <asm/smp.h>
 #include <asm/bug.h>
 
@@ -54,6 +55,24 @@
                              int retry, int wait);
 
 /*
+ * Call a function on all processors
+ */
+static inline int on_each_cpu(void (*func) (void *info), void *info,
+                             int retry, int wait)
+{
+       int ret;
+
+       preempt_disable();
+
+       ret = smp_call_function(func, info, retry, wait);
+       func(info);
+
+       preempt_enable();
+
+       return ret;
+}
+
+/*
  * True once the per process idle is forked
  */
 extern int smp_threads_ready;
@@ -96,6 +115,7 @@
 #define hard_smp_processor_id()                        0
 #define smp_threads_ready                      1
 #define smp_call_function(func,info,retry,wait)        ({ 0; })
+#define on_each_cpu(func,info,retry,wait)      ({ func(info); 0; })
 static inline void smp_send_reschedule(int cpu) { }
 static inline void smp_send_reschedule_all(void) { }
 #define cpu_online_map                         1
#9 lore raw
In reply to Andrew Morton
Quoted 7 lines · Andrew Morton
Quoted 3 lines · Dave Jones

btw, (unrelated) shouldn't smp_call_function be doing magick checks with cpu_online() ?

Looks OK? It sprays the IPI out to all the other CPUs in cpu_online_map, and waits for num_online_cpus()-1 CPUs to answer.

You cannot do that by counting without a lot of care. IPI messages do not have guaranteed "once only" semantics. On an error a resend can and has been observed to cause a reissue of an IPI on PII/PIII setups

#10 lore raw
Quoted 7 lines · Alan Cox
Quoted 5 lines · Andrew Morton
Quoted 3 lines · Dave Jones

btw, (unrelated) shouldn't smp_call_function be doing magick checks with cpu_online() ?

Quoted 6 lines
Quoted 2 lines

Looks OK? It sprays the IPI out to all the other CPUs in cpu_online_map, and waits for num_online_cpus()-1 CPUs to answer.

You cannot do that by counting without a lot of care. IPI messages do not have guaranteed "once only" semantics. On an error a resend can and has been observed to cause a reissue of an IPI on PII/PIII setups

If that resend results in delivery of an actual extra interrupt, the resent-to CPU can start playing with stuff which used to be on the sender's stack and the box goes splat.

Didn't sct have a fix for that?

#11 lore raw
Quoted 5 lines · Andrew Morton

If that resend results in delivery of an actual extra interrupt, the resent-to CPU can start playing with stuff which used to be on the sender's stack and the box goes splat.

Didn't sct have a fix for that?

Yes but it was never merged mainstream for some reason. I think it kind of got away

#12 lore raw
In reply to Andrew Morton
Quoted 5 lines
Quoted 2 lines

btw, (unrelated) shouldn't smp_call_function be doing magick checks with cpu_online() ?

Looks OK? It sprays the IPI out to all the other CPUs in cpu_online_map, and waits for num_online_cpus()-1 CPUs to answer.

Incidentally, would be nice if this thing had a timeout and bugged out when one didn't reply, rather than just wedging the whole CPU for ever. Yes, it's normally some other bug that makes the other CPU not reply, but would make diags a damned sight easier.

M.