prevent ia64 from invoking irq handlers on offline CPUs

PATCH In review Paul E. McKenney 2 participants 7 messages Aug 31, 2008

Share & tools

Paul E. McKenney

author

2008-08-31 17:28 UTC #0 lore raw

PATCH 1/1 prevent ia64 from invoking irq handlers on offline CPUs

Make ia64 refrain from clearing a given to-be-offlined CPU's bit in the cpu_online_mask until it has processed pending irqs. This change prevents other CPUs from being blindsided by an apparently offline CPU nevertheless changing globally visible state.

Signed-off-by Paul E. McKenney <[email protected]>

---

 smpboot.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

arch/ia64/kernel/smpboot.c +1 -2

index bcea81e..7a90f42 100644
@@ -741,8 +741,6 @@ int __cpu_disable(void)
 			return -EBUSY;
 	}
 
-	cpu_clear(cpu, cpu_online_map);
-
 	if (migrate_platform_irqs(cpu)) {
 		cpu_set(cpu, cpu_online_map);
 		return (-EBUSY);

@@ -751,6 +749,7 @@ int __cpu_disable(void)
 	remove_siblinginfo(cpu);
 	cpu_clear(cpu, cpu_online_map);
 	fixup_irqs();
+	cpu_clear(cpu, cpu_online_map);
 	local_flush_tlb_all();
 	cpu_clear(cpu, cpu_callin_map);
 	return 0;

Vegard Nossum

2008-08-31 17:46 UTC #1 lore raw

Quoted 25 lines · Paul E. McKenney

Make ia64 refrain from clearing a given to-be-offlined CPU's bit in the cpu_online_mask until it has processed pending irqs. This change prevents other CPUs from being blindsided by an apparently offline CPU nevertheless changing globally visible state.
Signed-off-by Paul E. McKenney <[email protected]>
---
 smpboot.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
arch/ia64/kernel/smpboot.c +0 -2
index bcea81e..7a90f42 100644
@@ -741,8 +741,6 @@ int __cpu_disable(void)
                       return -EBUSY;
       }
 
-       cpu_clear(cpu, cpu_online_map);
-
       if (migrate_platform_irqs(cpu)) {
               cpu_set(cpu, cpu_online_map);
               return (-EBUSY);

arch/ia64/kernel/smpboot.c int __cpu_disable(void) · line 755

Quoted 7 lines

arch/ia64/kernel/smpboot.c +1 -0

@@ -751,6 +749,7 @@ int __cpu_disable(void)
       remove_siblinginfo(cpu);
       cpu_clear(cpu, cpu_online_map);
       fixup_irqs();
+       cpu_clear(cpu, cpu_online_map);
       local_flush_tlb_all();
       cpu_clear(cpu, cpu_callin_map);
       return 0;

Hm. Maybe my eyes are going bad. But isn't that now clearing the cpu both before and after calling fixup_irqs()?

Vegard

Signature · 4 lines

"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Paul E. McKenney

author

2008-08-31 17:51 UTC #2 lore raw

Quoted 21 lines · Vegard Nossum

Quoted 19 lines · Paul E. McKenney
Make ia64 refrain from clearing a given to-be-offlined CPU's bit in the cpu_online_mask until it has processed pending irqs. This change prevents other CPUs from being blindsided by an apparently offline CPU nevertheless changing globally visible state.
Signed-off-by Paul E. McKenney <[email protected]>
---
 smpboot.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
arch/ia64/kernel/smpboot.c +0 -0
index bcea81e..7a90f42 100644
@@ -741,8 +741,6 @@ int __cpu_disable(void)
                       return -EBUSY;
       }
 

Quoted 16 lines

arch/ia64/kernel/smpboot.c int __cpu_disable(void) · line 755

Quoted 13 lines

arch/ia64/kernel/smpboot.c +1 -2

-       cpu_clear(cpu, cpu_online_map);
-
       if (migrate_platform_irqs(cpu)) {
               cpu_set(cpu, cpu_online_map);
               return (-EBUSY);

@@ -751,6 +749,7 @@ int __cpu_disable(void)
       remove_siblinginfo(cpu);
       cpu_clear(cpu, cpu_online_map);
       fixup_irqs();
+       cpu_clear(cpu, cpu_online_map);
       local_flush_tlb_all();
       cpu_clear(cpu, cpu_callin_map);
       return 0;

Hm. Maybe my eyes are going bad. But isn't that now clearing the cpu both before and after calling fixup_irqs()?

I don't believe so... I removed the earlier one and added the later one. Or is there another one hidden somewhere that I missed?

Thanx, Paul

Vegard Nossum

2008-08-31 17:53 UTC #3 lore raw

Quoted 3 lines · Paul E. McKenney

Quoted 3 lines
arch/ia64/kernel/smpboot.c int __cpu_disable(void) · line 750
Quoted 3 lines
arch/ia64/kernel/smpboot.c +0 -0
@@ -751,6 +749,7 @@ int __cpu_disable(void)
       remove_siblinginfo(cpu);
       cpu_clear(cpu, cpu_online_map);

Here ^^^

Quoted 11 lines

Quoted 8 lines
arch/ia64/kernel/smpboot.c int __cpu_disable(void) · line 755
Quoted 5 lines
arch/ia64/kernel/smpboot.c +1 -0
       fixup_irqs();
+       cpu_clear(cpu, cpu_online_map);
       local_flush_tlb_all();
       cpu_clear(cpu, cpu_callin_map);
       return 0;
Hm. Maybe my eyes are going bad. But isn't that now clearing the cpu both before and after calling fixup_irqs()?
I don't believe so... I removed the earlier one and added the later one. Or is there another one hidden somewhere that I missed?

See marker above. It's not removed, is it?

Vegard

Signature · 4 lines

"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Paul E. McKenney

author

2008-08-31 18:06 UTC #4 lore raw

Quoted 14 lines · Vegard Nossum

Quoted 3 lines · Paul E. McKenney
Quoted 3 lines
Review int __cpu_disable(void) · line 750
Quoted 3 lines
(quoted diff) +0 -0
@@ -751,6 +749,7 @@ int __cpu_disable(void)
       remove_siblinginfo(cpu);
       cpu_clear(cpu, cpu_online_map);
Here ^^^
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
(quoted diff) +1 -0
       fixup_irqs();
+       cpu_clear(cpu, cpu_online_map);
       local_flush_tlb_all();
       cpu_clear(cpu, cpu_callin_map);
       return 0;

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
Hm. Maybe my eyes are going bad. But isn't that now clearing the cpu both before and after calling fixup_irqs()?
I don't believe so... I removed the earlier one and added the later one. Or is there another one hidden somewhere that I missed?
See marker above. It's not removed, is it?

Right you are!!! There were two to begin with, and I moved the first one only. The attached updated patch removes the extra one.

Thanx, Paul

Signed-off-by Paul E. McKenney <[email protected]>

---

arch/ia64/kernel/smpboot.c +1 -3

index bcea81e..d8f05e5 100644
@@ -741,16 +741,14 @@ int __cpu_disable(void)
 			return -EBUSY;
 	}
 
-	cpu_clear(cpu, cpu_online_map);
-
 	if (migrate_platform_irqs(cpu)) {
 		cpu_set(cpu, cpu_online_map);
 		return (-EBUSY);
 	}
 
 	remove_siblinginfo(cpu);
-	cpu_clear(cpu, cpu_online_map);
 	fixup_irqs();
+	cpu_clear(cpu, cpu_online_map);
 	local_flush_tlb_all();
 	cpu_clear(cpu, cpu_callin_map);
 	return 0;

Vegard Nossum

2008-08-31 19:08 UTC #5 lore raw

Quoted 18 lines · Paul E. McKenney

Quoted 17 lines · Vegard Nossum
Quoted 3 lines · Paul E. McKenney
Quoted 3 lines
Review int __cpu_disable(void) · line 750
Quoted 3 lines
(quoted diff) +0 -0
@@ -751,6 +749,7 @@ int __cpu_disable(void)
       remove_siblinginfo(cpu);
       cpu_clear(cpu, cpu_online_map);
Here ^^^
Quoted 9 lines
Quoted 8 lines
arch/ia64/kernel/smpboot.c int __cpu_disable(void) · line 754
Quoted 5 lines
arch/ia64/kernel/smpboot.c +1 -0
       fixup_irqs();
+       cpu_clear(cpu, cpu_online_map);
       local_flush_tlb_all();
       cpu_clear(cpu, cpu_callin_map);
       return 0;
Hm. Maybe my eyes are going bad. But isn't that now clearing the cpu both before and after calling fixup_irqs()?

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
I don't believe so... I removed the earlier one and added the later one. Or is there another one hidden somewhere that I missed?
See marker above. It's not removed, is it?
Right you are!!! There were two to begin with, and I moved the first one only. The attached updated patch removes the extra one.

Good, I am not blind yet! (Though I did have to look more than once.)

On the other hand... Paul; human after all!

Vegard

Signature · 4 lines

"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Paul E. McKenney

author

2008-08-31 20:08 UTC #6 lore raw

Quoted 23 lines · Vegard Nossum

Quoted 21 lines · Paul E. McKenney
Quoted 20 lines · Vegard Nossum
Quoted 3 lines · Paul E. McKenney
Quoted 3 lines
Review int __cpu_disable(void) · line 750
Quoted 3 lines
(quoted diff) +0 -0
@@ -751,6 +749,7 @@ int __cpu_disable(void)
       remove_siblinginfo(cpu);
       cpu_clear(cpu, cpu_online_map);
Here ^^^
Quoted 11 lines
Quoted 8 lines
arch/ia64/kernel/smpboot.c int __cpu_disable(void) · line 754
Quoted 5 lines
arch/ia64/kernel/smpboot.c +1 -0
       fixup_irqs();
+       cpu_clear(cpu, cpu_online_map);
       local_flush_tlb_all();
       cpu_clear(cpu, cpu_callin_map);
       return 0;
Hm. Maybe my eyes are going bad. But isn't that now clearing the cpu both before and after calling fixup_irqs()?
I don't believe so... I removed the earlier one and added the later one. Or is there another one hidden somewhere that I missed?

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
See marker above. It's not removed, is it?
Right you are!!! There were two to begin with, and I moved the first one only. The attached updated patch removes the extra one.
Good, I am not blind yet! (Though I did have to look more than once.)
On the other hand... Paul; human after all!

Very much so, I am afraid! ;-)

Thanx, Paul