generic-ipi: Initialize call_single_queue before enabling interrupt

PATCH Superseded Takao Indoh 2 participants 3 messages Mar 25, 2011

Subsystems (2)

Live update kexec handover (KHO)

lore

Share & tools

lore

Takao Indoh

author

2011-03-25 15:53 UTC #0 lore raw

PATCH 1/1 generic-ipi: Initialize call_single_queue before enabling interrupt

Hi all,

Actually this is a v2 patch but I post with new subject. The first post is here.

[PATCH][KDUMP] Ignore spurious IPI
http://www.gossamer-threads.com/lists/linux/kernel/1356389

The problem I found is that kdump(2nd kernel) sometimes hangs up due to pending IPI from 1st kernel. Kernel panic occurs because IPI comes before call_single_queue is initialized, so this patch moves init_call_single_data() so that it can be called before local_irq_enable().

The details of the problem is below.

(1)
2nd kernel boot up

(2)
A pending IPI from 1st kernel comes after unmasking interrupts at the following point.

asmlinkage void __init start_kernel(void)
{
(snip)

    time_init();
    profile_init();
    if (!irqs_disabled())
            printk(KERN_CRIT "start_kernel(): bug: interrupts were "
                             "enabled early\n");
    early_boot_irqs_disabled = false;
    local_irq_enable(); <=======================================HERE

(3)
Kernel tries to handle the interrupt, but call_single_queue is not initialized yet at this point. As a result, in the generic_smp_call_function_single_interrupt(), NULL pointer dereference occurs when list_replace_init() tries to access &q->list.next.

Any comments would be appreciated.

Signed-off-by Takao Indoh <[email protected]>

---
 include/linux/smp.h |    1 +
 init/main.c         |    1 +
 kernel/smp.c        |   18 +++++++++++-------
 3 files changed, 13 insertions(+), 7 deletions(-)

include/linux/smp.h +1 -0

index 6dc95ca..0b81bba 100644
@@ -114,6 +114,7 @@ int on_each_cpu(smp_call_func_t func, void *info, int wait);
 void smp_prepare_boot_cpu(void);
 
 extern unsigned int setup_max_cpus;
+void init_call_single_data(void);
 
 #else /* !SMP */

init/main.c +1 -0

index 33c37c3..02a7617 100644
@@ -631,6 +631,7 @@ asmlinkage void __init start_kernel(void)
 		printk(KERN_CRIT "start_kernel(): bug: interrupts were "
 				 "enabled early\n");
 	early_boot_irqs_disabled = false;
+	init_call_single_data();
 	local_irq_enable();
 
 	/* Interrupts are enabled now so all GFP allocations are safe. */

kernel/smp.c +11 -7

index 7cbd0f2..f119610 100644
@@ -74,9 +74,19 @@ static struct notifier_block __cpuinitdata hotplug_cfd_notifier = {
 	.notifier_call		= hotplug_cfd,
 };
 
-static int __cpuinit init_call_single_data(void)
+static int __cpuinit init_hotplug_cfd(void)
 {
 	void *cpu = (void *)(long)smp_processor_id();
+
+	hotplug_cfd(&hotplug_cfd_notifier, CPU_UP_PREPARE, cpu);
+	register_cpu_notifier(&hotplug_cfd_notifier);
+
+	return 0;
+}
+early_initcall(init_hotplug_cfd);
+
+void __init init_call_single_data(void)
+{
 	int i;
 
 	for_each_possible_cpu(i) {

@@ -85,13 +95,7 @@ static int __cpuinit init_call_single_data(void)
 		raw_spin_lock_init(&q->lock);
 		INIT_LIST_HEAD(&q->list);
 	}
-
-	hotplug_cfd(&hotplug_cfd_notifier, CPU_UP_PREPARE, cpu);
-	register_cpu_notifier(&hotplug_cfd_notifier);
-
-	return 0;
 }
-early_initcall(init_call_single_data);
 
 /*
  * csd_lock/csd_unlock used to serialize access to per-cpu csd resources

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Milton Miller

2011-03-25 17:48 UTC #1 lore raw

Quoted 6 lines · Takao Indoh

Hi all,
Actually this is a v2 patch but I post with new subject. The first post is here.

Quoted 9 lines

[PATCH][KDUMP] Ignore spurious IPI
http://www.gossamer-threads.com/lists/linux/kernel/1356389
The problem I found is that kdump(2nd kernel) sometimes hangs up due to pending IPI from 1st kernel. Kernel panic occurs because IPI comes before call_single_queue is initialized, so this patch moves init_call_single_data() so that it can be called before local_irq_enable().

This is good background, but I generally prefer changelogs to start by describing what is being changed and then proceed on to why, and then the history of the patch and discussion.

Quoted 12 lines

The details of the problem is below.
(1)
2nd kernel boot up
(2)
A pending IPI from 1st kernel comes after unmasking interrupts at the following point.
asmlinkage void __init start_kernel(void)
{
(snip)

Quoted 8 lines

    time_init();
    profile_init();
    if (!irqs_disabled())
            printk(KERN_CRIT "start_kernel(): bug: interrupts were "
                             "enabled early\n");
    early_boot_irqs_disabled = false;
    local_irq_enable(); <=======================================HERE

This could be described more concisely as "when irqs are first enabled in start_kernel()".

Quoted 20 lines

(3)
Kernel tries to handle the interrupt, but call_single_queue is not initialized yet at this point. As a result, in the generic_smp_call_function_single_interrupt(), NULL pointer dereference occurs when list_replace_init() tries to access &q->list.next.
Any comments would be appreciated.
Signed-off-by Takao Indoh <[email protected]>
---
 include/linux/smp.h |    1 +
 init/main.c         |    1 +
 kernel/smp.c        |   18 +++++++++++-------
 3 files changed, 13 insertions(+), 7 deletions(-)
include/linux/smp.h +0 -0
index 6dc95ca..0b81bba 100644

include/linux/smp.h int on_each_cpu(smp_call_func_t func, void *info, int wait); · line 118

Quoted 5 lines

include/linux/smp.h +1 -0

@@ -114,6 +114,7 @@ int on_each_cpu(smp_call_func_t func, void *info, int wait);
 void smp_prepare_boot_cpu(void);
 
 extern unsigned int setup_max_cpus;
+void init_call_single_data(void);

This no longer applys (although it did to 2.6.38).

include/linux/smp.h int on_each_cpu(smp_call_func_t func, void *info, int wait); · line 120

Quoted 2 lines

include/linux/smp.h +0 -0
 #else /* !SMP */
 

I was going to point out that this will fail on up, but it looks like ba8dd03ac09f51a69c154b8cb508b701d713a2cd (Full conversion to early_initcall() interface, remove old interface) only found 2 of the previous 3 occurrances.

However, this does point out that not all CONFIG_SMP compiles are CONFIG_USE_GENERIC_SMP_HELPERS, and you need to adjust accordingly.

[ for other reviewers no this is not a simple revert of a hunk of that 2.6.27 change, this is moving the code yet earlier ].

init/main.c asmlinkage void __init start_kernel(void) · line 636

Quoted 11 lines

init/main.c +1 -0

index 33c37c3..02a7617 100644
@@ -631,6 +631,7 @@ asmlinkage void __init start_kernel(void)
 		printk(KERN_CRIT "start_kernel(): bug: interrupts were "
 				 "enabled early\n");
 	early_boot_irqs_disabled = false;
+	init_call_single_data();
 	local_irq_enable();

1) this is too late, the point of the printk and the state variable is to make sure interrupts are enabled exactly when we expect for the first time.

2) please follow the naming conventions and name this xxx_init. Actually I prefer xxx be the name of the subsystem (call_function) instead of the name of the data structure (call_single_data).

Quoted 11 lines

init/main.c +0 -0

 	/* Interrupts are enabled now so all GFP allocations are safe. */

kernel/smp.c +1 -1

index 7cbd0f2..f119610 100644
@@ -74,9 +74,19 @@ static struct notifier_block __cpuinitdata hotplug_cfd_notifier = {
 	.notifier_call		= hotplug_cfd,
 };
 
-static int __cpuinit init_call_single_data(void)
+static int __cpuinit init_hotplug_cfd(void)

kernel/smp.c static struct notifier_block __cpuinitdata hotplug_cfd_notifier = { · line 87

Quoted 10 lines

kernel/smp.c +8 -0

 {
 	void *cpu = (void *)(long)smp_processor_id();
+
+	hotplug_cfd(&hotplug_cfd_notifier, CPU_UP_PREPARE, cpu);
+	register_cpu_notifier(&hotplug_cfd_notifier);
+
+	return 0;
+}
+early_initcall(init_hotplug_cfd);
+

Why did you seperate this from the rest of the function? I haven't tested the patch but don't why this needs to be delayed. If there is some reason I don't see, please describe it in the change log.

Quoted 10 lines

kernel/smp.c +2 -0

+void __init init_call_single_data(void)
+{
 	int i;
 
 	for_each_possible_cpu(i) {

@@ -85,13 +95,7 @@ static int __cpuinit init_call_single_data(void)
 		raw_spin_lock_init(&q->lock);
 		INIT_LIST_HEAD(&q->list);
 	}

kernel/smp.c static int __cpuinit init_call_single_data(void) · line 101

Quoted 10 lines

kernel/smp.c +0 -6

-
-	hotplug_cfd(&hotplug_cfd_notifier, CPU_UP_PREPARE, cpu);
-	register_cpu_notifier(&hotplug_cfd_notifier);
-
-	return 0;
 }
-early_initcall(init_call_single_data);
 
 /*
  * csd_lock/csd_unlock used to serialize access to per-cpu csd resources

milton

_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec

Takao Indoh

author

2011-03-25 22:08 UTC #2 lore raw

Quoted 88 lines · Milton Miller

Quoted 6 lines · Takao Indoh
Hi all,
Actually this is a v2 patch but I post with new subject. The first post is here.
Quoted 9 lines
[PATCH][KDUMP] Ignore spurious IPI
http://www.gossamer-threads.com/lists/linux/kernel/1356389
The problem I found is that kdump(2nd kernel) sometimes hangs up due to pending IPI from 1st kernel. Kernel panic occurs because IPI comes before call_single_queue is initialized, so this patch moves init_call_single_data() so that it can be called before local_irq_enable().
This is good background, but I generally prefer changelogs to start by describing what is being changed and then proceed on to why, and then the history of the patch and discussion.
Quoted 12 lines
The details of the problem is below.
(1)
2nd kernel boot up
(2)
A pending IPI from 1st kernel comes after unmasking interrupts at the following point.
asmlinkage void __init start_kernel(void)
{
(snip)
Quoted 8 lines
    time_init();
    profile_init();
    if (!irqs_disabled())
            printk(KERN_CRIT "start_kernel(): bug: interrupts were "
                             "enabled early\n");
    early_boot_irqs_disabled = false;
    local_irq_enable(); <=======================================HERE
This could be described more concisely as "when irqs are first enabled in start_kernel()".
Quoted 20 lines
(3)
Kernel tries to handle the interrupt, but call_single_queue is not initialized yet at this point. As a result, in the generic_smp_call_function_single_interrupt(), NULL pointer dereference occurs when list_replace_init() tries to access &q->list.next.
Any comments would be appreciated.
Signed-off-by Takao Indoh <[email protected]>
---
 include/linux/smp.h |    1 +
 init/main.c         |    1 +
 kernel/smp.c        |   18 +++++++++++-------
 3 files changed, 13 insertions(+), 7 deletions(-)
include/linux/smp.h +0 -0
index 6dc95ca..0b81bba 100644
include/linux/smp.h int on_each_cpu(smp_call_func_t func, void *info, int · line 119
Quoted 6 lines
include/linux/smp.h +1 -0
@@ -114,6 +114,7 @@ int on_each_cpu(smp_call_func_t func, void *info, int 
 wait);
 void smp_prepare_boot_cpu(void);
 
 extern unsigned int setup_max_cpus;
+void init_call_single_data(void);
 
This no longer applys (although it did to 2.6.38).
include/linux/smp.h int on_each_cpu(smp_call_func_t func, void *info, int wait); · line 119
Quoted 2 lines
include/linux/smp.h +0 -0
 #else /* !SMP */
I was going to point out that this will fail on up, but it looks like ba8dd03ac09f51a69c154b8cb508b701d713a2cd (Full conversion to early_initcall() interface, remove old interface) only found 2 of the previous 3 occurrances.
However, this does point out that not all CONFIG_SMP compiles are CONFIG_USE_GENERIC_SMP_HELPERS, and you need to adjust accordingly.
[ for other reviewers no this is not a simple revert of a hunk of that 2.6.27 change, this is moving the code yet earlier ].

Quoted 15 lines

init/main.c asmlinkage void __init start_kernel(void) · line 636
Quoted 11 lines
init/main.c +1 -0
index 33c37c3..02a7617 100644
@@ -631,6 +631,7 @@ asmlinkage void __init start_kernel(void)
 		printk(KERN_CRIT "start_kernel(): bug: interrupts were "
 				 "enabled early\n");
 	early_boot_irqs_disabled = false;
+	init_call_single_data();
 	local_irq_enable();
 
1) this is too late, the point of the printk and the state variable is to make sure interrupts are enabled exactly when we expect for the first time.

Ok, so the following place is correct?

profile_init();
+ call_function_init();
if (!irqs_disabled())
printk(KERN_CRIT "start_kernel(): bug: interrupts were "
"enabled early\n");
early_boot_irqs_disabled = false; local_irq_enable();

Quoted 15 lines

2) please follow the naming conventions and name this xxx_init. Actually I prefer xxx be the name of the subsystem (call_function) instead of the name of the data structure (call_single_data).
Quoted 10 lines
init/main.c +0 -0
 	/* Interrupts are enabled now so all GFP allocations are safe. */
kernel/smp.c +0 -0
index 7cbd0f2..f119610 100644
@@ -74,9 +74,19 @@ static struct notifier_block __cpuinitdata 
 hotplug_cfd_notifier = {
 	.notifier_call		= hotplug_cfd,
 };
 

Quoted 16 lines

kernel/smp.c static struct notifier_block __cpuinitdata hotplug_cfd_notifier = { · line 87
Quoted 12 lines
kernel/smp.c +9 -1
-static int __cpuinit init_call_single_data(void)
+static int __cpuinit init_hotplug_cfd(void)
 {
 	void *cpu = (void *)(long)smp_processor_id();
+
+	hotplug_cfd(&hotplug_cfd_notifier, CPU_UP_PREPARE, cpu);
+	register_cpu_notifier(&hotplug_cfd_notifier);
+
+	return 0;
+}
+early_initcall(init_hotplug_cfd);
+
Why did you seperate this from the rest of the function? I haven't tested the patch but don't why this needs to be delayed. If there is some reason I don't see, please describe it in the change log.

There is not special reason. I just thought I should minimize the change to prevent regression because I cannot do hotplug test. Ok, next time I'll just change the name from init_call_single_data to call_function_init instead of separating it.

It seems that Peter Zijlstra added this hotplug code at 8969a5ed, so I added him to Cc.

Thanks for your review.

Thanks,
Takao Indoh

Quoted 23 lines

Quoted 11 lines

kernel/smp.c +2 -0

+void __init init_call_single_data(void)
+{
 	int i;

kernel/smp.c +0 -0

 	for_each_possible_cpu(i) {

@@ -85,13 +95,7 @@ static int __cpuinit init_call_single_data(void)
 		raw_spin_lock_init(&q->lock);
 		INIT_LIST_HEAD(&q->list);
 	}

kernel/smp.c static int __cpuinit init_call_single_data(void) · line 101

Quoted 10 lines

kernel/smp.c +0 -6

-
-	hotplug_cfd(&hotplug_cfd_notifier, CPU_UP_PREPARE, cpu);
-	register_cpu_notifier(&hotplug_cfd_notifier);
-
-	return 0;
 }
-early_initcall(init_call_single_data);
 
 /*
  * csd_lock/csd_unlock used to serialize access to per-cpu csd resources

milton