Skip to contentSkip to conversation

make voyager work again after the cpumask_t changes

PATCH In review James Bottomley 3 participants 7 messages Aug 28, 2003

lore
Share & tools
lore

James Bottomley

author
#0 lore raw

PATCH 1/1 make voyager work again after the cpumask_t changes

Most is just simple fixes; however, the needless change from atomic to non-atomic operations in smp_invalidate_interrupt() caused me a lot of pain to track down since it introduced some very subtle bugs.

I've also taken phys_cpu_present_map out of smp.h. Since it physid_mask_t is defined in mpspec.h anyway, and contains a duplicate definition, I don't believe it can hurt anything.

James

===== arch/i386/mach-voyager/voyager_smp.c 1.15 vs edited =====

arch/i386/mach-voyager/voyager_smp.c +13 -12
@@ -130,7 +130,7 @@
 {
 	int cpu;
 
-	for_each_cpu(cpu, mk_cpumask_const(cpu_online_map)) {
+	for_each_cpu(cpu, cpu_online_map) {
 		if(cpuset & (1<<cpu)) {
 #ifdef VOYAGER_DEBUG
 			if(!cpu_isset(cpu, cpu_online_map))

@@ -874,10 +874,10 @@
 asmlinkage void 
 smp_invalidate_interrupt(void)
 {
-	__u8 cpu = get_cpu();
+	__u8 cpu = smp_processor_id();
 
-	if (!(smp_invalidate_needed & (1UL << cpu)))
-		goto out;
+	if (!test_bit(cpu, &smp_invalidate_needed))
+		return;
 	/* This will flood messages.  Don't uncomment unless you see
 	 * Problems with cross cpu invalidation
 	VDEBUG(("VOYAGER SMP: CPU%d received INVALIDATE_CPI\n",

@@ -893,9 +893,9 @@
 		} else
 			leave_mm(cpu);
 	}
-	smp_invalidate_needed |= 1UL << cpu;
- out:
-	put_cpu_no_resched();
+	smp_mb__before_clear_bit();
+	clear_bit(cpu, &smp_invalidate_needed);
+	smp_mb__after_clear_bit();
 }
 
 /* All the new flush operations for 2.4 */

@@ -929,6 +929,7 @@
 	send_CPI(cpumask, VIC_INVALIDATE_CPI);
 
 	while (smp_invalidate_needed) {
+		mb();
 		if(--stuck == 0) {
 			printk("***WARNING*** Stuck doing invalidate CPI (CPU%d)\n", smp_processor_id());
 			break;

@@ -1464,7 +1465,7 @@
 	cpuset &= 0xff;		/* only first 8 CPUs vaild for VIC CPI */
 	if(cpuset == 0)
 		return;
-	for_each_cpu(cpu, mk_cpumask_const(cpu_online_map)) {
+	for_each_cpu(cpu, cpu_online_map) {
 		if(cpuset & (1<<cpu))
 			set_bit(cpi, &vic_cpi_mailbox[cpu]);
 	}

@@ -1578,7 +1579,7 @@
 	VDEBUG(("VOYAGER: enable_vic_irq(%d) CPU%d affinity 0x%lx\n",
 		irq, cpu, cpu_irq_affinity[cpu]));
 	spin_lock_irqsave(&vic_irq_lock, flags);
-	for_each_cpu(real_cpu, mk_cpumask_const(cpu_online_map)) {
+	for_each_cpu(real_cpu, cpu_online_map) {
 		if(!(voyager_extended_vic_processors & (1<<real_cpu)))
 			continue;
 		if(!(cpu_irq_affinity[real_cpu] & mask)) {

@@ -1723,7 +1724,7 @@
 
 			printk("VOYAGER SMP: CPU%d lost interrupt %d\n",
 			       cpu, irq);
-			for_each_cpu(real_cpu, mk_cpumask_const(mask)) {
+			for_each_cpu(real_cpu, mask) {
 
 				outb(VIC_CPU_MASQUERADE_ENABLE | real_cpu,
 				     VIC_PROCESSOR_ID);

@@ -1808,7 +1809,7 @@
 		 * bus) */
 		return;
 
-	for_each_cpu(cpu, mk_cpumask_const(cpu_online_map)) {
+	for_each_cpu(cpu, cpu_online_map) {
 		unsigned long cpu_mask = 1 << cpu;
 		
 		if(cpu_mask & real_mask) {

@@ -1874,7 +1875,7 @@
 	int old_cpu = smp_processor_id(), cpu;
 
 	/* dump the interrupt masks of each processor */
-	for_each_cpu(cpu, mk_cpumask_const(cpu_online_map)) {
+	for_each_cpu(cpu, cpu_online_map) {
 		__u16 imr, isr, irr;
 		unsigned long flags;

===== include/asm-i386/smp.h 1.28 vs edited =====

include/asm-i386/smp.h +0 -1
@@ -32,7 +32,6 @@
  */
  
 extern void smp_alloc_memory(void);
-extern physid_mask_t phys_cpu_present_map;
 extern int pic_mode;
 extern int smp_num_siblings;
 extern int cpu_sibling_map[];
#1 lore raw
Quoted 6 lines · James Bottomley

Most is just simple fixes; however, the needless change from atomic to non-atomic operations in smp_invalidate_interrupt() caused me a lot of pain to track down since it introduced some very subtle bugs. I've also taken phys_cpu_present_map out of smp.h. Since it physid_mask_t is defined in mpspec.h anyway, and contains a duplicate definition, I don't believe it can hurt anything.

Sorry about this; I'm not sure what kind of catastrophe brought about the non-atomic smp_invalidate_interrupt() bits in voyager.c

-- wli

#2 lore raw
In reply to James Bottomley
Quoted 4 lines · James Bottomley

Most is just simple fixes; however, the needless change from atomic to non-atomic operations in smp_invalidate_interrupt() caused me a lot of pain to track down since it introduced some very subtle bugs.

Yes, the generic code was like that too. It was causing lockups. Sorry, I did not realise that voyager had a private invalidatation implementation.

Officially smp_invalidate_needed should be a cpumask_t and smp_invalidate_interrupt() should be using cpu_isset() rather than open-coded bitops. For all those 64-way voyagers out there ;)

(Actually it is legitimate: you may want to run a NR_CPUS=48 kernel on a 2-way voyager just for testing purposes). I'll drop your patch in as-is, and maybe Bill can take a look at cpumaskifying it sometime?

#3 lore raw
Quoted 3 lines · James Bottomley
Quoted 3 lines

Most is just simple fixes; however, the needless change from atomic to non-atomic operations in smp_invalidate_interrupt() caused me a lot of pain to track down since it introduced some very subtle bugs.

Quoted 8 lines · Andrew Morton

Yes, the generic code was like that too. It was causing lockups. Sorry, I did not realise that voyager had a private invalidatation implementation. Officially smp_invalidate_needed should be a cpumask_t and smp_invalidate_interrupt() should be using cpu_isset() rather than open-coded bitops. For all those 64-way voyagers out there ;) (Actually it is legitimate: you may want to run a NR_CPUS=48 kernel on a 2-way voyager just for testing purposes). I'll drop your patch in as-is, and maybe Bill can take a look at cpumaskifying it sometime?

I'm not convinced it's worth it; AIUI there are architectural limits to Voyager that prevent it from ever supporting > 32x in hardware, though it could be worth doing so in tandem with an across-the-board all- subarch extension of generic i386 support.

-- wli

#4 lore raw
Quoted 7 lines · William Lee Irwin III
Quoted 4 lines

(Actually it is legitimate: you may want to run a NR_CPUS=48 kernel on a 2-way voyager just for testing purposes). I'll drop your patch in as-is, and maybe Bill can take a look at cpumaskifying it sometime?

I'm not convinced it's worth it; AIUI there are architectural limits to Voyager that prevent it from ever supporting > 32x in hardware,

Sure. But we want a kernel which was compiled with NR_CPUS>32 to still boot and run correctly on voyager.

Yes, the code as-is will happen to work, but dtrt and all that.

#5 lore raw
Quoted 2 lines · William Lee Irwin III
Quoted 2 lines

I'm not convinced it's worth it; AIUI there are architectural limits to Voyager that prevent it from ever supporting > 32x in hardware,

Quoted 3 lines · Andrew Morton

Sure. But we want a kernel which was compiled with NR_CPUS>32 to still boot and run correctly on voyager.
Yes, the code as-is will happen to work, but dtrt and all that.

Okay, I'll send in the obvious cleanup and run it by jejb first.

-- wli

author
#6 lore raw
In reply to Andrew Morton
Quoted 2 lines · Andrew Morton

Yes, the generic code was like that too. It was causing lockups. Sorry, I did not realise that voyager had a private invalidatation implementation.

It actually has to since the invalidation implementation is a property of the SMP HAL...fortunately voyager is the only subarch that has to replace the SMP HAL wholesale.

Quoted 7 lines

Officially smp_invalidate_needed should be a cpumask_t and smp_invalidate_interrupt() should be using cpu_isset() rather than open-coded bitops. For all those 64-way voyagers out there ;)

(Actually it is legitimate: you may want to run a NR_CPUS=48 kernel on a 2-way voyager just for testing purposes). I'll drop your patch in as-is, and maybe Bill can take a look at cpumaskifying it sometime?

OK.

Actually, looking at the code made me realise that we can kill the tlbstate_lock and run lockless, so I'll play with doing that too.

James