Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756613Ab1DFWxN (ORCPT ); Wed, 6 Apr 2011 18:53:13 -0400 Received: from fgwmail6.fujitsu.co.jp ([192.51.44.36]:44778 "EHLO fgwmail6.fujitsu.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755966Ab1DFWxM (ORCPT ); Wed, 6 Apr 2011 18:53:12 -0400 X-SecurityPolicyCheck: OK by SHieldMailChecker v1.5.1 From: Takao Indoh To: Vivek Goyal Cc: linux-kernel@vger.kernel.org, kexec@lists.infradead.org, Milton Miller , Jens Axboe , "Paul E. McKenney" , Ingo Molnar , WANG Cong , Peter Zijlstra Subject: Re: [PATCH v2] generic-ipi: Initialize call_single_queue before enabling interrupt Date: Wed, 06 Apr 2011 18:52:27 -0400 MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-Mailer: HidemaruMail 5.38 (WinNT,501) In-Reply-To: References: <20110406204742.GH18777@redhat.com> Message-Id: X-Antivirus: avast! (VPS 110406-1, 2011/04/06), Outbound message X-Antivirus-Status: Clean Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7538 Lines: 238 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 >>> --- >>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/