* Re: [PATCH v2] generic-ipi: Initialize call_single_queue before enabling interrupt
2011-03-29 16:35 [PATCH v2] generic-ipi: Initialize call_single_queue before enabling interrupt Takao Indoh
@ 2011-03-30 15:19 ` Américo Wang
2011-03-30 15:48 ` Neil Horman
` (3 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Américo Wang @ 2011-03-30 15:19 UTC (permalink / raw)
To: Takao Indoh
Cc: linux-kernel, kexec, Milton Miller, Jens Axboe, Paul E. McKenney,
Ingo Molnar, Vivek Goyal, Peter Zijlstra
On Wed, Mar 30, 2011 at 12:35 AM, Takao Indoh
<indou.takao@jp.fujitsu.com> wrote:
> Hi all,
>
> This patch renames init_call_single_data() to call_function_init() and
> calls it in start_kernel() so that call_single_queue can be initialized
> before enabling interrupt.
>
> There is a problem 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. The details are as follows.
> (1) 2nd kernel boot up
> (2) A pending IPI from 1st kernel comes when irqs are first enabled
> in start_kernel().
> (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.
> Therefore this patch changes the name of init_call_single_data() to
> call_function_init() and calls it before local_irq_enable() in
> start_kernel().
>
> v2:
> - Rename init_call_single_data() to call_function_init() and calls it in
> start_kernel()
> - Change insert position in start_kernel().
> - Adjust for CONFIG_SMP/CONFIG_USE_GENERIC_SMP_HELPERS options
> - Rebased to Linus's latest tree
>
Looks good to my eyes,
Reviewed-by: WANG Cong <xiyou.wangcong@gmail.com>
Thanks.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] generic-ipi: Initialize call_single_queue before enabling interrupt
2011-03-29 16:35 [PATCH v2] generic-ipi: Initialize call_single_queue before enabling interrupt Takao Indoh
2011-03-30 15:19 ` Américo Wang
@ 2011-03-30 15:48 ` Neil Horman
2011-04-06 20:47 ` Vivek Goyal
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Neil Horman @ 2011-03-30 15:48 UTC (permalink / raw)
To: Takao Indoh
Cc: linux-kernel, kexec, Jens Axboe, Peter Zijlstra, Milton Miller,
WANG Cong, Ingo Molnar, Paul E. McKenney, Vivek Goyal
On Tue, Mar 29, 2011 at 12:35:04PM -0400, Takao Indoh wrote:
> Hi all,
>
> This patch renames init_call_single_data() to call_function_init() and
> calls it in start_kernel() so that call_single_queue can be initialized
> before enabling interrupt.
>
> There is a problem 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. The details are as follows.
> (1) 2nd kernel boot up
> (2) A pending IPI from 1st kernel comes when irqs are first enabled
> in start_kernel().
> (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.
> Therefore this patch changes the name of init_call_single_data() to
> call_function_init() and calls it before local_irq_enable() in
> start_kernel().
>
> v2:
> - Rename init_call_single_data() to call_function_init() and calls it in
> start_kernel()
> - Change insert position in start_kernel().
> - Adjust for CONFIG_SMP/CONFIG_USE_GENERIC_SMP_HELPERS options
> - Rebased to Linus's latest tree
>
> v1:
> https://lkml.org/lkml/2011/3/25/317
> - Divide init_call_single_data() into two functions,
> o init_call_single_data: initialize call_single_queue
> o init_hotplug_cfd: initialize hotplug_cfd_notifier
> And call init_call_single_data before local_irq_enable() in
> start_kernel().
>
> v0:
> https://lkml.org/lkml/2011/3/23/417
> - In generic_smp_call_function_single_interrupt(), check if
> call_single_queue was initialized or not, and just return if not
> initialized.
>
> Signed-off-by: Takao Indoh <indou.takao@jp.fujitsu.com>
> ---
> include/linux/smp.h | 5 ++++-
> init/main.c | 1 +
> kernel/smp.c | 5 +----
> 3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/smp.h b/include/linux/smp.h
> index 74243c8..4fb3eac 100644
> --- a/include/linux/smp.h
> +++ b/include/linux/smp.h
> @@ -85,12 +85,15 @@ int smp_call_function_any(const struct cpumask *mask,
> * Generic and arch helpers
> */
> #ifdef CONFIG_USE_GENERIC_SMP_HELPERS
> +void __init call_function_init(void);
> void generic_smp_call_function_single_interrupt(void);
> void generic_smp_call_function_interrupt(void);
> void ipi_call_lock(void);
> void ipi_call_unlock(void);
> void ipi_call_lock_irq(void);
> void ipi_call_unlock_irq(void);
> +#else
> +static inline void call_function_init(void) { }
> #endif
>
> /*
> @@ -144,7 +147,7 @@ static inline void smp_send_reschedule(int cpu) { }
> #define smp_prepare_boot_cpu() do {} while (0)
> #define smp_call_function_many(mask, func, info, wait) \
> (up_smp_call_function(func, info))
> -static inline void init_call_single_data(void) { }
> +static inline void call_function_init(void) { }
>
> static inline int
> smp_call_function_any(const struct cpumask *mask, smp_call_func_t func,
> diff --git a/init/main.c b/init/main.c
> index 4a9479e..12821d1 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -539,6 +539,7 @@ asmlinkage void __init start_kernel(void)
> timekeeping_init();
> time_init();
> profile_init();
> + call_function_init();
> if (!irqs_disabled())
> printk(KERN_CRIT "start_kernel(): bug: interrupts were "
> "enabled early\n");
> diff --git a/kernel/smp.c b/kernel/smp.c
> index 73a1951..fb67dfa 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -74,7 +74,7 @@ static struct notifier_block __cpuinitdata hotplug_cfd_notifier = {
> .notifier_call = hotplug_cfd,
> };
>
> -static int __cpuinit init_call_single_data(void)
> +void __init call_function_init(void)
> {
> void *cpu = (void *)(long)smp_processor_id();
> int i;
> @@ -88,10 +88,7 @@ static int __cpuinit init_call_single_data(void)
>
> 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
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] generic-ipi: Initialize call_single_queue before enabling interrupt
2011-03-29 16:35 [PATCH v2] generic-ipi: Initialize call_single_queue before enabling interrupt Takao Indoh
2011-03-30 15:19 ` Américo Wang
2011-03-30 15:48 ` Neil Horman
@ 2011-04-06 20:47 ` Vivek Goyal
2011-04-06 22:47 ` Takao Indoh
2011-06-16 15:19 ` Takao Indoh
2011-06-18 21:05 ` [tip:sched/urgent] generic-ipi: Fix kexec boot crash by initializing call_single_queue before enabling interrupts tip-bot for Takao Indoh
4 siblings, 1 reply; 11+ messages in thread
From: Vivek Goyal @ 2011-04-06 20:47 UTC (permalink / raw)
To: Takao Indoh
Cc: linux-kernel, kexec, Milton Miller, Jens Axboe, Paul E. McKenney,
Ingo Molnar, WANG Cong, Peter Zijlstra
On Tue, Mar 29, 2011 at 12:35:04PM -0400, Takao Indoh wrote:
> Hi all,
>
> This patch renames init_call_single_data() to call_function_init() and
> calls it in start_kernel() so that call_single_queue can be initialized
> before enabling interrupt.
>
> There is a problem 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. The details are as follows.
> (1) 2nd kernel boot up
> (2) A pending IPI from 1st kernel comes when irqs are first enabled
> in start_kernel().
> (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.
> Therefore this patch changes the name of init_call_single_data() to
> call_function_init() and calls it before local_irq_enable() in
> start_kernel().
>
> v2:
> - Rename init_call_single_data() to call_function_init() and calls it in
> start_kernel()
> - Change insert position in start_kernel().
> - Adjust for CONFIG_SMP/CONFIG_USE_GENERIC_SMP_HELPERS options
> - Rebased to Linus's latest tree
>
> v1:
> https://lkml.org/lkml/2011/3/25/317
> - Divide init_call_single_data() into two functions,
> o init_call_single_data: initialize call_single_queue
> o init_hotplug_cfd: initialize hotplug_cfd_notifier
> And call init_call_single_data before local_irq_enable() in
> start_kernel().
>
> v0:
> https://lkml.org/lkml/2011/3/23/417
> - In generic_smp_call_function_single_interrupt(), check if
> call_single_queue was initialized or not, and just return if not
> initialized.
>
> Signed-off-by: Takao Indoh <indou.takao@jp.fujitsu.com>
> ---
> include/linux/smp.h | 5 ++++-
> init/main.c | 1 +
> kernel/smp.c | 5 +----
> 3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/smp.h b/include/linux/smp.h
> index 74243c8..4fb3eac 100644
> --- a/include/linux/smp.h
> +++ b/include/linux/smp.h
> @@ -85,12 +85,15 @@ int smp_call_function_any(const struct cpumask *mask,
> * Generic and arch helpers
> */
> #ifdef CONFIG_USE_GENERIC_SMP_HELPERS
> +void __init call_function_init(void);
> void generic_smp_call_function_single_interrupt(void);
> void generic_smp_call_function_interrupt(void);
> void ipi_call_lock(void);
> void ipi_call_unlock(void);
> void ipi_call_lock_irq(void);
> void ipi_call_unlock_irq(void);
> +#else
> +static inline void call_function_init(void) { }
> #endif
>
> /*
> @@ -144,7 +147,7 @@ static inline void smp_send_reschedule(int cpu) { }
> #define smp_prepare_boot_cpu() do {} while (0)
> #define smp_call_function_many(mask, func, info, wait) \
> (up_smp_call_function(func, info))
> -static inline void init_call_single_data(void) { }
> +static inline void call_function_init(void) { }
>
> static inline int
> smp_call_function_any(const struct cpumask *mask, smp_call_func_t func,
> diff --git a/init/main.c b/init/main.c
> index 4a9479e..12821d1 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -539,6 +539,7 @@ asmlinkage void __init start_kernel(void)
> timekeeping_init();
> time_init();
> profile_init();
> + call_function_init();
Takao,
So by calling this function before we enable interrupts we have made
sure that per cpu call_single_queue has been initialized and q->list
also has been initiliazed and it is an empty list now.
After enabling the interrupts, I am assuming we will call
generic_smp_call_function_single_interrupt().
this function does.
raw_spin_lock(&q->lock);
list_replace_init(&q->list, &list);
raw_spin_unlock(&q->lock);
while (!list_empty(&list)) {
struct call_single_data *data;
data = list_entry(list.next, struct call_single_data,
list);
list_del(&data->list);
Looking at the code of list_replace_init(), I think we will have odd
results if q->list is empty. Looks like list->next will be pointing to
&q->list?
IIUC, q->list sould be empty when we get pending IPI from previous kernel
because any function scheduled for execution must have been inserted on
previous kernel's data structures and here we are building fresh data
structures.
If that is the case, I think above code should have weared interaction.
We should think that "list" is not empty and try to execute a data item
q->list which is actually not a data item.
What am I missing here. After your patch, have to debugged it and
noticed how list_replace_init() does on empty lists and what's the
result of list_empty(list)?
Thanks
Vivek
> if (!irqs_disabled())
> printk(KERN_CRIT "start_kernel(): bug: interrupts were "
> "enabled early\n");
> diff --git a/kernel/smp.c b/kernel/smp.c
> index 73a1951..fb67dfa 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -74,7 +74,7 @@ static struct notifier_block __cpuinitdata hotplug_cfd_notifier = {
> .notifier_call = hotplug_cfd,
> };
>
> -static int __cpuinit init_call_single_data(void)
> +void __init call_function_init(void)
> {
> void *cpu = (void *)(long)smp_processor_id();
> int i;
> @@ -88,10 +88,7 @@ static int __cpuinit init_call_single_data(void)
>
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] generic-ipi: Initialize call_single_queue before enabling interrupt
2011-04-06 20:47 ` Vivek Goyal
@ 2011-04-06 22:47 ` Takao Indoh
2011-04-06 22:52 ` Takao Indoh
0 siblings, 1 reply; 11+ messages in thread
From: Takao Indoh @ 2011-04-06 22:47 UTC (permalink / raw)
To: Vivek Goyal
Cc: linux-kernel, kexec, Milton Miller, Jens Axboe, Paul E. McKenney,
Ingo Molnar, WANG Cong, Peter Zijlstra
On Wed, 6 Apr 2011 16:47:42 -0400, Vivek Goyal wrote:
>On Tue, Mar 29, 2011 at 12:35:04PM -0400, Takao Indoh wrote:
>> Hi all,
>>
>> This patch renames init_call_single_data() to call_function_init() and
>> calls it in start_kernel() so that call_single_queue can be initialized
>> before enabling interrupt.
>>
>> There is a problem 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. The details are as follows.
>> (1) 2nd kernel boot up
>> (2) A pending IPI from 1st kernel comes when irqs are first enabled
>> in start_kernel().
>> (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.
>> Therefore this patch changes the name of init_call_single_data() to
>> call_function_init() and calls it before local_irq_enable() in
>> start_kernel().
>>
>> v2:
>> - Rename init_call_single_data() to call_function_init() and calls it in
>> start_kernel()
>> - Change insert position in start_kernel().
>> - Adjust for CONFIG_SMP/CONFIG_USE_GENERIC_SMP_HELPERS options
>> - Rebased to Linus's latest tree
>>
>> v1:
>> https://lkml.org/lkml/2011/3/25/317
>> - Divide init_call_single_data() into two functions,
>> o init_call_single_data: initialize call_single_queue
>> o init_hotplug_cfd: initialize hotplug_cfd_notifier
>> And call init_call_single_data before local_irq_enable() in
>> start_kernel().
>>
>> v0:
>> https://lkml.org/lkml/2011/3/23/417
>> - In generic_smp_call_function_single_interrupt(), check if
>> call_single_queue was initialized or not, and just return if not
>> initialized.
>>
>> Signed-off-by: Takao Indoh <indou.takao@jp.fujitsu.com>
>> ---
>> include/linux/smp.h | 5 ++++-
>> init/main.c | 1 +
>> kernel/smp.c | 5 +----
>> 3 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/smp.h b/include/linux/smp.h
>> index 74243c8..4fb3eac 100644
>> --- a/include/linux/smp.h
>> +++ b/include/linux/smp.h
>> @@ -85,12 +85,15 @@ int smp_call_function_any(const struct cpumask *mask,
>> * Generic and arch helpers
>> */
>> #ifdef CONFIG_USE_GENERIC_SMP_HELPERS
>> +void __init call_function_init(void);
>> void generic_smp_call_function_single_interrupt(void);
>> void generic_smp_call_function_interrupt(void);
>> void ipi_call_lock(void);
>> void ipi_call_unlock(void);
>> void ipi_call_lock_irq(void);
>> void ipi_call_unlock_irq(void);
>> +#else
>> +static inline void call_function_init(void) { }
>> #endif
>>
>> /*
>> @@ -144,7 +147,7 @@ static inline void smp_send_reschedule(int cpu) { }
>> #define smp_prepare_boot_cpu() do {} while (0)
>> #define smp_call_function_many(mask, func, info, wait) \
>> (up_smp_call_function(func, info))
>> -static inline void init_call_single_data(void) { }
>> +static inline void call_function_init(void) { }
>>
>> static inline int
>> smp_call_function_any(const struct cpumask *mask, smp_call_func_t func,
>> diff --git a/init/main.c b/init/main.c
>> index 4a9479e..12821d1 100644
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -539,6 +539,7 @@ asmlinkage void __init start_kernel(void)
>> timekeeping_init();
>> time_init();
>> profile_init();
>> + call_function_init();
>
>Takao,
>
>So by calling this function before we enable interrupts we have made
>sure that per cpu call_single_queue has been initialized and q->list
>also has been initiliazed and it is an empty list now.
>
>After enabling the interrupts, I am assuming we will call
>generic_smp_call_function_single_interrupt().
>
>this function does.
>
> raw_spin_lock(&q->lock);
> list_replace_init(&q->list, &list);
> raw_spin_unlock(&q->lock);
>
> while (!list_empty(&list)) {
> struct call_single_data *data;
>
> data = list_entry(list.next, struct call_single_data,
>list);
> list_del(&data->list);
>
>Looking at the code of list_replace_init(), I think we will have odd
>results if q->list is empty. Looks like list->next will be pointing to
>&q->list?
>
>IIUC, q->list sould be empty when we get pending IPI from previous kernel
>because any function scheduled for execution must have been inserted on
>previous kernel's data structures and here we are building fresh data
>structures.
>
>If that is the case, I think above code should have weared interaction.
>We should think that "list" is not empty and try to execute a data item
>q->list which is actually not a data item.
>
>What am I missing here. After your patch, have to debugged it and
>noticed how list_replace_init() does on empty lists and what's the
>result of list_empty(list)?
When list_replace_init(&q->list, &list) is called,
they are changed as followed.
/* list_replace */
(A) &list->next = &q->list->next;
(B) &list->next->prev = &list;
(C) &list->prev = &q->list->prev;
(D) &list->prev->next = &list;
/* INIT_LIST_HEAD */
(E) &q->list->next = &q->list;
(F) &q->list->prev = &q->list;
So, if q->list is empty, each list is changed like this.
(Initial state)
list.next ==> &list
list.prev ==> &list
q->list.next ==> &q->list
q->list.prev ==> &q->list
(A)
list.next ==> &q->list
list.prev ==> &list
q->list.next ==> &q->list
q->list.prev ==> &q->list
(B)
list.next ==> &q->list
list.prev ==> &list
q->list.next ==> &q->list
q->list.prev ==> &list
(C)
list.next ==> &q->list
list.prev ==> &list
q->list.next ==> &q->list
q->list.prev ==> &list
(D)
list.next ==> &list
list.prev ==> &list
q->list.next ==> &q->list
q->list.prev ==> &list
(E)
list.next ==> &list
list.prev ==> &list
q->list.next ==> &q->list
q->list.prev ==> &list
(F)
list.next ==> &list
list.prev ==> &list
q->list.next ==> &q->list
q->list.prev ==> &q->list
So, list_empty(list)? is always false, if I am not missing something.
Thanks,
Takao Indoh
>
>Thanks
>Vivek
>
>
>
>> if (!irqs_disabled())
>> printk(KERN_CRIT "start_kernel(): bug: interrupts were "
>> "enabled early\n");
>> diff --git a/kernel/smp.c b/kernel/smp.c
>> index 73a1951..fb67dfa 100644
>> --- a/kernel/smp.c
>> +++ b/kernel/smp.c
>> @@ -74,7 +74,7 @@ static struct notifier_block __cpuinitdata
>> hotplug_cfd_notifier = {
>> .notifier_call = hotplug_cfd,
>> };
>>
>> -static int __cpuinit init_call_single_data(void)
>> +void __init call_function_init(void)
>> {
>> void *cpu = (void *)(long)smp_processor_id();
>> int i;
>> @@ -88,10 +88,7 @@ static int __cpuinit init_call_single_data(void)
>>
>> 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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] generic-ipi: Initialize call_single_queue before enabling interrupt
2011-04-06 22:47 ` Takao Indoh
@ 2011-04-06 22:52 ` Takao Indoh
2011-04-07 13:40 ` Vivek Goyal
0 siblings, 1 reply; 11+ messages in thread
From: Takao Indoh @ 2011-04-06 22:52 UTC (permalink / raw)
To: Vivek Goyal
Cc: linux-kernel, kexec, Milton Miller, Jens Axboe, Paul E. McKenney,
Ingo Molnar, WANG Cong, Peter Zijlstra
On Wed, 06 Apr 2011 18:47:36 -0400, Takao Indoh wrote:
>On Wed, 6 Apr 2011 16:47:42 -0400, Vivek Goyal wrote:
>
>>On Tue, Mar 29, 2011 at 12:35:04PM -0400, Takao Indoh wrote:
>>> Hi all,
>>>
>>> This patch renames init_call_single_data() to call_function_init() and
>>> calls it in start_kernel() so that call_single_queue can be initialized
>>> before enabling interrupt.
>>>
>>> There is a problem 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. The details are as follows.
>>> (1) 2nd kernel boot up
>>> (2) A pending IPI from 1st kernel comes when irqs are first enabled
>>> in start_kernel().
>>> (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.
>>> Therefore this patch changes the name of init_call_single_data() to
>>> call_function_init() and calls it before local_irq_enable() in
>>> start_kernel().
>>>
>>> v2:
>>> - Rename init_call_single_data() to call_function_init() and calls it in
>>> start_kernel()
>>> - Change insert position in start_kernel().
>>> - Adjust for CONFIG_SMP/CONFIG_USE_GENERIC_SMP_HELPERS options
>>> - Rebased to Linus's latest tree
>>>
>>> v1:
>>> https://lkml.org/lkml/2011/3/25/317
>>> - Divide init_call_single_data() into two functions,
>>> o init_call_single_data: initialize call_single_queue
>>> o init_hotplug_cfd: initialize hotplug_cfd_notifier
>>> And call init_call_single_data before local_irq_enable() in
>>> start_kernel().
>>>
>>> v0:
>>> https://lkml.org/lkml/2011/3/23/417
>>> - In generic_smp_call_function_single_interrupt(), check if
>>> call_single_queue was initialized or not, and just return if not
>>> initialized.
>>>
>>> Signed-off-by: Takao Indoh <indou.takao@jp.fujitsu.com>
>>> ---
>>> include/linux/smp.h | 5 ++++-
>>> init/main.c | 1 +
>>> kernel/smp.c | 5 +----
>>> 3 files changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/linux/smp.h b/include/linux/smp.h
>>> index 74243c8..4fb3eac 100644
>>> --- a/include/linux/smp.h
>>> +++ b/include/linux/smp.h
>>> @@ -85,12 +85,15 @@ int smp_call_function_any(const struct cpumask *mask,
>>> * Generic and arch helpers
>>> */
>>> #ifdef CONFIG_USE_GENERIC_SMP_HELPERS
>>> +void __init call_function_init(void);
>>> void generic_smp_call_function_single_interrupt(void);
>>> void generic_smp_call_function_interrupt(void);
>>> void ipi_call_lock(void);
>>> void ipi_call_unlock(void);
>>> void ipi_call_lock_irq(void);
>>> void ipi_call_unlock_irq(void);
>>> +#else
>>> +static inline void call_function_init(void) { }
>>> #endif
>>>
>>> /*
>>> @@ -144,7 +147,7 @@ static inline void smp_send_reschedule(int cpu) { }
>>> #define smp_prepare_boot_cpu() do {} while (0)
>>> #define smp_call_function_many(mask, func, info, wait) \
>>> (up_smp_call_function(func, info))
>>> -static inline void init_call_single_data(void) { }
>>> +static inline void call_function_init(void) { }
>>>
>>> static inline int
>>> smp_call_function_any(const struct cpumask *mask, smp_call_func_t func,
>>> diff --git a/init/main.c b/init/main.c
>>> index 4a9479e..12821d1 100644
>>> --- a/init/main.c
>>> +++ b/init/main.c
>>> @@ -539,6 +539,7 @@ asmlinkage void __init start_kernel(void)
>>> timekeeping_init();
>>> time_init();
>>> profile_init();
>>> + call_function_init();
>>
>>Takao,
>>
>>So by calling this function before we enable interrupts we have made
>>sure that per cpu call_single_queue has been initialized and q->list
>>also has been initiliazed and it is an empty list now.
>>
>>After enabling the interrupts, I am assuming we will call
>>generic_smp_call_function_single_interrupt().
>>
>>this function does.
>>
>> raw_spin_lock(&q->lock);
>> list_replace_init(&q->list, &list);
>> raw_spin_unlock(&q->lock);
>>
>> while (!list_empty(&list)) {
>> struct call_single_data *data;
>>
>> data = list_entry(list.next, struct call_single_data,
>>list);
>> list_del(&data->list);
>>
>>Looking at the code of list_replace_init(), I think we will have odd
>>results if q->list is empty. Looks like list->next will be pointing to
>>&q->list?
>>
>>IIUC, q->list sould be empty when we get pending IPI from previous kernel
>>because any function scheduled for execution must have been inserted on
>>previous kernel's data structures and here we are building fresh data
>>structures.
>>
>>If that is the case, I think above code should have weared interaction.
>>We should think that "list" is not empty and try to execute a data item
>>q->list which is actually not a data item.
>>
>>What am I missing here. After your patch, have to debugged it and
>>noticed how list_replace_init() does on empty lists and what's the
>>result of list_empty(list)?
>
>When list_replace_init(&q->list, &list) is called,
>they are changed as followed.
>
>/* list_replace */
>(A) &list->next = &q->list->next;
>(B) &list->next->prev = &list;
>(C) &list->prev = &q->list->prev;
>(D) &list->prev->next = &list;
>
>/* INIT_LIST_HEAD */
>(E) &q->list->next = &q->list;
>(F) &q->list->prev = &q->list;
>
>So, if q->list is empty, each list is changed like this.
>
>(Initial state)
>list.next ==> &list
>list.prev ==> &list
>q->list.next ==> &q->list
>q->list.prev ==> &q->list
>
>(A)
>list.next ==> &q->list
>list.prev ==> &list
>q->list.next ==> &q->list
>q->list.prev ==> &q->list
>
>(B)
>list.next ==> &q->list
>list.prev ==> &list
>q->list.next ==> &q->list
>q->list.prev ==> &list
>
>(C)
>list.next ==> &q->list
>list.prev ==> &list
>q->list.next ==> &q->list
>q->list.prev ==> &list
>
>(D)
>list.next ==> &list
>list.prev ==> &list
>q->list.next ==> &q->list
>q->list.prev ==> &list
>
>(E)
>list.next ==> &list
>list.prev ==> &list
>q->list.next ==> &q->list
>q->list.prev ==> &list
>
>(F)
>list.next ==> &list
>list.prev ==> &list
>q->list.next ==> &q->list
>q->list.prev ==> &q->list
>
>
>So, list_empty(list)? is always false, if I am not missing something.
No, list_empty(list)? is always *true*.
Thanks,
Takao Indoh
>>
>>Thanks
>>Vivek
>>
>>
>>
>>> if (!irqs_disabled())
>>> printk(KERN_CRIT "start_kernel(): bug: interrupts were "
>>> "enabled early\n");
>>> diff --git a/kernel/smp.c b/kernel/smp.c
>>> index 73a1951..fb67dfa 100644
>>> --- a/kernel/smp.c
>>> +++ b/kernel/smp.c
>>> @@ -74,7 +74,7 @@ static struct notifier_block __cpuinitdata
>>> hotplug_cfd_notifier = {
>>> .notifier_call = hotplug_cfd,
>>> };
>>>
>>> -static int __cpuinit init_call_single_data(void)
>>> +void __init call_function_init(void)
>>> {
>>> void *cpu = (void *)(long)smp_processor_id();
>>> int i;
>>> @@ -88,10 +88,7 @@ static int __cpuinit init_call_single_data(void)
>>>
>>> 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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] generic-ipi: Initialize call_single_queue before enabling interrupt
2011-04-06 22:52 ` Takao Indoh
@ 2011-04-07 13:40 ` Vivek Goyal
0 siblings, 0 replies; 11+ messages in thread
From: Vivek Goyal @ 2011-04-07 13:40 UTC (permalink / raw)
To: Takao Indoh
Cc: linux-kernel, kexec, Milton Miller, Jens Axboe, Paul E. McKenney,
Ingo Molnar, WANG Cong, Peter Zijlstra
On Wed, Apr 06, 2011 at 06:52:27PM -0400, Takao Indoh wrote:
> On Wed, 06 Apr 2011 18:47:36 -0400, Takao Indoh wrote:
>
> >On Wed, 6 Apr 2011 16:47:42 -0400, Vivek Goyal wrote:
> >
> >>On Tue, Mar 29, 2011 at 12:35:04PM -0400, Takao Indoh wrote:
> >>> Hi all,
> >>>
> >>> This patch renames init_call_single_data() to call_function_init() and
> >>> calls it in start_kernel() so that call_single_queue can be initialized
> >>> before enabling interrupt.
> >>>
> >>> There is a problem 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. The details are as follows.
> >>> (1) 2nd kernel boot up
> >>> (2) A pending IPI from 1st kernel comes when irqs are first enabled
> >>> in start_kernel().
> >>> (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.
> >>> Therefore this patch changes the name of init_call_single_data() to
> >>> call_function_init() and calls it before local_irq_enable() in
> >>> start_kernel().
> >>>
> >>> v2:
> >>> - Rename init_call_single_data() to call_function_init() and calls it in
> >>> start_kernel()
> >>> - Change insert position in start_kernel().
> >>> - Adjust for CONFIG_SMP/CONFIG_USE_GENERIC_SMP_HELPERS options
> >>> - Rebased to Linus's latest tree
> >>>
> >>> v1:
> >>> https://lkml.org/lkml/2011/3/25/317
> >>> - Divide init_call_single_data() into two functions,
> >>> o init_call_single_data: initialize call_single_queue
> >>> o init_hotplug_cfd: initialize hotplug_cfd_notifier
> >>> And call init_call_single_data before local_irq_enable() in
> >>> start_kernel().
> >>>
> >>> v0:
> >>> https://lkml.org/lkml/2011/3/23/417
> >>> - In generic_smp_call_function_single_interrupt(), check if
> >>> call_single_queue was initialized or not, and just return if not
> >>> initialized.
> >>>
> >>> Signed-off-by: Takao Indoh <indou.takao@jp.fujitsu.com>
[..]
> >So, list_empty(list)? is always false, if I am not missing something.
>
> No, list_empty(list)? is always *true*.
>
ok, I got it. So list_replace_init() is fine even if q->list is empty. So
when a pending IPI from previous kernel comes, q->list will be empty
and we will simply do nothing. Sounds reaosnable to me.
Acked-by: Vivek Goyal <vgoyal@redhat.com>
Thanks
Vivek
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] generic-ipi: Initialize call_single_queue before enabling interrupt
2011-03-29 16:35 [PATCH v2] generic-ipi: Initialize call_single_queue before enabling interrupt Takao Indoh
` (2 preceding siblings ...)
2011-04-06 20:47 ` Vivek Goyal
@ 2011-06-16 15:19 ` Takao Indoh
2011-06-17 8:19 ` Ingo Molnar
2011-06-18 21:05 ` [tip:sched/urgent] generic-ipi: Fix kexec boot crash by initializing call_single_queue before enabling interrupts tip-bot for Takao Indoh
4 siblings, 1 reply; 11+ messages in thread
From: Takao Indoh @ 2011-06-16 15:19 UTC (permalink / raw)
To: linux-kernel, kexec
Cc: Milton Miller, Jens Axboe, Paul E. McKenney, Ingo Molnar,
Vivek Goyal, WANG Cong, Peter Zijlstra, Andrew Morton
Just a reminder...
Thanks for acks, but I'm still waiting for inclusion.
I tested 3.0.0-rc3 and kdump still failed due to pending IPI
from 1st kernel. Need this patch to fix it.
Thanks,
Takao Indoh
On Tue, 29 Mar 2011 12:35:04 -0400, Takao Indoh wrote:
>Hi all,
>
>This patch renames init_call_single_data() to call_function_init() and
>calls it in start_kernel() so that call_single_queue can be initialized
>before enabling interrupt.
>
>There is a problem 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. The details are as follows.
>(1) 2nd kernel boot up
>(2) A pending IPI from 1st kernel comes when irqs are first enabled
> in start_kernel().
>(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.
>Therefore this patch changes the name of init_call_single_data() to
>call_function_init() and calls it before local_irq_enable() in
>start_kernel().
>
>v2:
>- Rename init_call_single_data() to call_function_init() and calls it in
> start_kernel()
>- Change insert position in start_kernel().
>- Adjust for CONFIG_SMP/CONFIG_USE_GENERIC_SMP_HELPERS options
>- Rebased to Linus's latest tree
>
>v1:
>https://lkml.org/lkml/2011/3/25/317
>- Divide init_call_single_data() into two functions,
> o init_call_single_data: initialize call_single_queue
> o init_hotplug_cfd: initialize hotplug_cfd_notifier
> And call init_call_single_data before local_irq_enable() in
> start_kernel().
>
>v0:
>https://lkml.org/lkml/2011/3/23/417
>- In generic_smp_call_function_single_interrupt(), check if
> call_single_queue was initialized or not, and just return if not
> initialized.
>
>Signed-off-by: Takao Indoh <indou.takao@jp.fujitsu.com>
>---
> include/linux/smp.h | 5 ++++-
> init/main.c | 1 +
> kernel/smp.c | 5 +----
> 3 files changed, 6 insertions(+), 5 deletions(-)
>
>diff --git a/include/linux/smp.h b/include/linux/smp.h
>index 74243c8..4fb3eac 100644
>--- a/include/linux/smp.h
>+++ b/include/linux/smp.h
>@@ -85,12 +85,15 @@ int smp_call_function_any(const struct cpumask *mask,
> * Generic and arch helpers
> */
> #ifdef CONFIG_USE_GENERIC_SMP_HELPERS
>+void __init call_function_init(void);
> void generic_smp_call_function_single_interrupt(void);
> void generic_smp_call_function_interrupt(void);
> void ipi_call_lock(void);
> void ipi_call_unlock(void);
> void ipi_call_lock_irq(void);
> void ipi_call_unlock_irq(void);
>+#else
>+static inline void call_function_init(void) { }
> #endif
>
> /*
>@@ -144,7 +147,7 @@ static inline void smp_send_reschedule(int cpu) { }
> #define smp_prepare_boot_cpu() do {} while (0)
> #define smp_call_function_many(mask, func, info, wait) \
> (up_smp_call_function(func, info))
>-static inline void init_call_single_data(void) { }
>+static inline void call_function_init(void) { }
>
> static inline int
> smp_call_function_any(const struct cpumask *mask, smp_call_func_t func,
>diff --git a/init/main.c b/init/main.c
>index 4a9479e..12821d1 100644
>--- a/init/main.c
>+++ b/init/main.c
>@@ -539,6 +539,7 @@ asmlinkage void __init start_kernel(void)
> timekeeping_init();
> time_init();
> profile_init();
>+ call_function_init();
> if (!irqs_disabled())
> printk(KERN_CRIT "start_kernel(): bug: interrupts were "
> "enabled early\n");
>diff --git a/kernel/smp.c b/kernel/smp.c
>index 73a1951..fb67dfa 100644
>--- a/kernel/smp.c
>+++ b/kernel/smp.c
>@@ -74,7 +74,7 @@ static struct notifier_block __cpuinitdata
>hotplug_cfd_notifier = {
> .notifier_call = hotplug_cfd,
> };
>
>-static int __cpuinit init_call_single_data(void)
>+void __init call_function_init(void)
> {
> void *cpu = (void *)(long)smp_processor_id();
> int i;
>@@ -88,10 +88,7 @@ static int __cpuinit init_call_single_data(void)
>
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] generic-ipi: Initialize call_single_queue before enabling interrupt
2011-06-16 15:19 ` Takao Indoh
@ 2011-06-17 8:19 ` Ingo Molnar
2011-06-17 15:11 ` Takao Indoh
0 siblings, 1 reply; 11+ messages in thread
From: Ingo Molnar @ 2011-06-17 8:19 UTC (permalink / raw)
To: Takao Indoh
Cc: linux-kernel, kexec, Milton Miller, Jens Axboe, Paul E. McKenney,
Vivek Goyal, WANG Cong, Peter Zijlstra, Andrew Morton
* Takao Indoh <indou.takao@jp.fujitsu.com> wrote:
> Just a reminder...
>
> Thanks for acks, but I'm still waiting for inclusion.
> I tested 3.0.0-rc3 and kdump still failed due to pending IPI
> from 1st kernel. Need this patch to fix it.
For any future patch that has a review discussion (like this one)
please collect Acks and resend the patch with the acks included once
the discussion results in a final patch, otherwise maintainers might
skip the patch as "still being reviewed".
I've collected the acks here and started testing your patch.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] generic-ipi: Initialize call_single_queue before enabling interrupt
2011-06-17 8:19 ` Ingo Molnar
@ 2011-06-17 15:11 ` Takao Indoh
0 siblings, 0 replies; 11+ messages in thread
From: Takao Indoh @ 2011-06-17 15:11 UTC (permalink / raw)
To: Ingo Molnar
Cc: linux-kernel, kexec, Milton Miller, Jens Axboe, Paul E. McKenney,
Vivek Goyal, WANG Cong, Peter Zijlstra, Andrew Morton
On Fri, 17 Jun 2011 10:19:11 +0200, Ingo Molnar wrote:
>
>* Takao Indoh <indou.takao@jp.fujitsu.com> wrote:
>
>> Just a reminder...
>>
>> Thanks for acks, but I'm still waiting for inclusion.
>> I tested 3.0.0-rc3 and kdump still failed due to pending IPI
>> from 1st kernel. Need this patch to fix it.
>
>For any future patch that has a review discussion (like this one)
>please collect Acks and resend the patch with the acks included once
>the discussion results in a final patch, otherwise maintainers might
>skip the patch as "still being reviewed".
>
>I've collected the acks here and started testing your patch.
Ok, understood. Thanks for your advice.
Thanks,
Takao Indoh
>
>Thanks,
>
> Ingo
^ permalink raw reply [flat|nested] 11+ messages in thread
* [tip:sched/urgent] generic-ipi: Fix kexec boot crash by initializing call_single_queue before enabling interrupts
2011-03-29 16:35 [PATCH v2] generic-ipi: Initialize call_single_queue before enabling interrupt Takao Indoh
` (3 preceding siblings ...)
2011-06-16 15:19 ` Takao Indoh
@ 2011-06-18 21:05 ` tip-bot for Takao Indoh
4 siblings, 0 replies; 11+ messages in thread
From: tip-bot for Takao Indoh @ 2011-06-18 21:05 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, hpa, mingo, axboe, a.p.zijlstra, miltonm, vgoyal,
indou.takao, xiyou.wangcong, paulmck, nhorman, tglx, mingo
Commit-ID: d8ad7d1123a960cc9f276bd499f9325c6f5e1bd1
Gitweb: http://git.kernel.org/tip/d8ad7d1123a960cc9f276bd499f9325c6f5e1bd1
Author: Takao Indoh <indou.takao@jp.fujitsu.com>
AuthorDate: Tue, 29 Mar 2011 12:35:04 -0400
Committer: Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 17 Jun 2011 10:17:12 +0200
generic-ipi: Fix kexec boot crash by initializing call_single_queue before enabling interrupts
There is a problem that kdump(2nd kernel) sometimes hangs up due
to a pending IPI from 1st kernel. Kernel panic occurs because IPI
comes before call_single_queue is initialized.
To fix the crash, rename init_call_single_data() to call_function_init()
and call it in start_kernel() so that call_single_queue can be
initialized before enabling interrupts.
The details of the crash are:
(1) 2nd kernel boots up
(2) A pending IPI from 1st kernel comes when irqs are first enabled
in start_kernel().
(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.
Therefore this patch changes the name of init_call_single_data()
to call_function_init() and calls it before local_irq_enable()
in start_kernel().
Signed-off-by: Takao Indoh <indou.takao@jp.fujitsu.com>
Reviewed-by: WANG Cong <xiyou.wangcong@gmail.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
Acked-by: Vivek Goyal <vgoyal@redhat.com>
Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Milton Miller <miltonm@bga.com>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: kexec@lists.infradead.org
Link: http://lkml.kernel.org/r/D6CBEE2F420741indou.takao@jp.fujitsu.com
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
include/linux/smp.h | 5 ++++-
init/main.c | 1 +
kernel/smp.c | 5 +----
3 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/include/linux/smp.h b/include/linux/smp.h
index 7ad824d..8cc38d3 100644
--- a/include/linux/smp.h
+++ b/include/linux/smp.h
@@ -85,12 +85,15 @@ int smp_call_function_any(const struct cpumask *mask,
* Generic and arch helpers
*/
#ifdef CONFIG_USE_GENERIC_SMP_HELPERS
+void __init call_function_init(void);
void generic_smp_call_function_single_interrupt(void);
void generic_smp_call_function_interrupt(void);
void ipi_call_lock(void);
void ipi_call_unlock(void);
void ipi_call_lock_irq(void);
void ipi_call_unlock_irq(void);
+#else
+static inline void call_function_init(void) { }
#endif
/*
@@ -134,7 +137,7 @@ static inline void smp_send_reschedule(int cpu) { }
#define smp_prepare_boot_cpu() do {} while (0)
#define smp_call_function_many(mask, func, info, wait) \
(up_smp_call_function(func, info))
-static inline void init_call_single_data(void) { }
+static inline void call_function_init(void) { }
static inline int
smp_call_function_any(const struct cpumask *mask, smp_call_func_t func,
diff --git a/init/main.c b/init/main.c
index cafba67..d7211fa 100644
--- a/init/main.c
+++ b/init/main.c
@@ -542,6 +542,7 @@ asmlinkage void __init start_kernel(void)
timekeeping_init();
time_init();
profile_init();
+ call_function_init();
if (!irqs_disabled())
printk(KERN_CRIT "start_kernel(): bug: interrupts were "
"enabled early\n");
diff --git a/kernel/smp.c b/kernel/smp.c
index 73a1951..fb67dfa 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -74,7 +74,7 @@ static struct notifier_block __cpuinitdata hotplug_cfd_notifier = {
.notifier_call = hotplug_cfd,
};
-static int __cpuinit init_call_single_data(void)
+void __init call_function_init(void)
{
void *cpu = (void *)(long)smp_processor_id();
int i;
@@ -88,10 +88,7 @@ static int __cpuinit init_call_single_data(void)
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
^ permalink raw reply related [flat|nested] 11+ messages in thread