From patchwork Tue Jun 17 09:18:47 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Srivatsa S. Bhat" X-Patchwork-Id: 4365311 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id B3FCCBEEAA for ; Tue, 17 Jun 2014 09:23:37 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 76DB620204 for ; Tue, 17 Jun 2014 09:23:36 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 290AA201DE for ; Tue, 17 Jun 2014 09:23:35 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1WwpZW-0007p6-C6; Tue, 17 Jun 2014 09:20:54 +0000 Received: from e28smtp09.in.ibm.com ([122.248.162.9]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1WwpZS-0007me-OT for linux-arm-kernel@lists.infradead.org; Tue, 17 Jun 2014 09:20:52 +0000 Received: from /spool/local by e28smtp09.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 17 Jun 2014 14:50:24 +0530 Received: from d28dlp02.in.ibm.com (9.184.220.127) by e28smtp09.in.ibm.com (192.168.1.139) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Tue, 17 Jun 2014 14:50:21 +0530 Received: from d28relay04.in.ibm.com (d28relay04.in.ibm.com [9.184.220.61]) by d28dlp02.in.ibm.com (Postfix) with ESMTP id 09B03394005B for ; Tue, 17 Jun 2014 14:50:20 +0530 (IST) Received: from d28av05.in.ibm.com (d28av05.in.ibm.com [9.184.220.67]) by d28relay04.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id s5H9KX9F65142826 for ; Tue, 17 Jun 2014 14:50:33 +0530 Received: from d28av05.in.ibm.com (localhost [127.0.0.1]) by d28av05.in.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s5H9KGxD013476 for ; Tue, 17 Jun 2014 14:50:17 +0530 Received: from srivatsabhat.in.ibm.com (srivatsabhat.in.ibm.com [9.124.35.231]) by d28av05.in.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with ESMTP id s5H9KFmp013396; Tue, 17 Jun 2014 14:50:15 +0530 Message-ID: <53A007F7.6020103@linux.vnet.ibm.com> Date: Tue, 17 Jun 2014 14:48:47 +0530 From: "Srivatsa S. Bhat" User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 MIME-Version: 1.0 To: Sachin Kamat , linux-kernel@vger.kernel.org, "linux-arm-kernel@lists.infradead.org" , linux-samsung-soc@vger.kernel.org, akpm@linux-foundation.org Subject: Re: Boot warnings on exynos5420 based boards References: In-Reply-To: X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 14061709-2674-0000-0000-000000730229 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20140617_022051_266509_DC3365EB X-CRM114-Status: GOOD ( 34.61 ) X-Spam-Score: -3.3 (---) Cc: nicolas.pitre@linaro.org, fweisbec@gmail.com, Sachin Kamat , Arnd Bergmann X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-2.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi Sachin, On 06/17/2014 01:39 PM, Sachin Kamat wrote: > Hi, > > I observe the below warnings while trying to boot Exynos5420 based boards > since yesterday's linux-next (next-20140616) using multi_v7_defconfig. Looks I guess you meant next-20140617. > like it is triggered by the commit 56e6921829 ("CPU hotplug, smp: > flush any pending IPI callbacks before CPU offline"). Any ideas? > > > ************************************************************* > [ 0.046521] Exynos MCPM support installed > [ 0.048939] CPU1: Booted secondary processor > [ 0.065005] CPU1: update cpu_capacity 1535 > [ 0.065011] CPU1: thread -1, cpu 1, socket 0, mpidr 80000001 > [ 0.065660] CPU2: Booted secondary processor > [ 0.085005] CPU2: update cpu_capacity 1535 > [ 0.085012] CPU2: thread -1, cpu 2, socket 0, mpidr 80000002 > [ 0.085662] CPU3: Booted secondary processor > [ 0.105005] CPU3: update cpu_capacity 1535 > [ 0.105011] CPU3: thread -1, cpu 3, socket 0, mpidr 80000003 > [ 1.105031] CPU4: failed to come online > [ 1.105081] ------------[ cut here ]------------ > [ 1.105104] WARNING: CPU: 0 PID: 1 at kernel/smp.c:228 > flush_smp_call_function_queue+0xc0/0x178() > [ 1.105112] Modules linked in: > [ 1.105129] CPU: 0 PID: 1 Comm: swapper/0 Not tainted > 3.15.0-next-20140616-00002-g38f9385a061b #2035 > [ 1.105157] [] (unwind_backtrace) from [] > (show_stack+0x10/0x14) > [ 1.105179] [] (show_stack) from [] > (dump_stack+0x8c/0x9c) > [ 1.105198] [] (dump_stack) from [] > (warn_slowpath_common+0x70/0x8c) > [ 1.105216] [] (warn_slowpath_common) from [] > (warn_slowpath_null+0x1c/0x24) > [ 1.105235] [] (warn_slowpath_null) from [] > (flush_smp_call_function_queue+0xc0/0x178) > [ 1.105253] [] (flush_smp_call_function_queue) from > [] (hotplug_cfd+0x98/0xd8) > [ 1.105269] [] (hotplug_cfd) from [] > (notifier_call_chain+0x44/0x84) > [ 1.105285] [] (notifier_call_chain) from [] > (_cpu_up+0x120/0x170) > [ 1.105302] [] (_cpu_up) from [] (cpu_up+0x70/0x94) > [ 1.105319] [] (cpu_up) from [] (smp_init+0xac/0xb0) > [ 1.105337] [] (smp_init) from [] > (kernel_init_freeable+0x90/0x1dc) > [ 1.105353] [] (kernel_init_freeable) from [] > (kernel_init+0xc/0xe8) > [ 1.105368] [] (kernel_init) from [] > (ret_from_fork+0x14/0x3c) > [ 1.105389] ---[ end trace bc66942e4ab63168 ]--- Argh! I had put the switch-case handling for CPU_DYING at the 'wrong' place, since I hadn't noticed that CPU_UP_CANCELED silently falls-through to CPU_DEAD. This is what happens when people don't explicitly write "fall-through" in the comments in a switch-case statement :-( Below is an updated patch, please let me know how it goes. (You'll have to revert c47a9d7cca first, and then 56e692182, before trying this patch). [c47a9d7cca - CPU hotplug, smp: Execute any pending IPI callbacks before CPU offline] [56e692182 - CPU hotplug, smp: flush any pending IPI callbacks before CPU offline] Andrew, can you please use this patch instead? Thanks a lot! ------------------------------------------------------------------------------ From: Srivatsa S. Bhat [PATCH] CPU hotplug, smp: Execute any pending IPI callbacks before CPU offline There is a race between the CPU offline code (within stop-machine) and the smp-call-function code, which can lead to getting IPIs on the outgoing CPU, *after* it has gone offline. Specifically, this can happen when using smp_call_function_single_async() to send the IPI, since this API allows sending asynchronous IPIs from IRQ disabled contexts. The exact race condition is described below. During CPU offline, in stop-machine, we don't enforce any rule in the _DISABLE_IRQ stage, regarding the order in which the outgoing CPU and the other CPUs disable their local interrupts. Due to this, we can encounter a situation in which an IPI is sent by one of the other CPUs to the outgoing CPU (while it is *still* online), but the outgoing CPU ends up noticing it only *after* it has gone offline. CPU 1 CPU 2 (Online CPU) (CPU going offline) Enter _PREPARE stage Enter _PREPARE stage Enter _DISABLE_IRQ stage = Got a device interrupt, and | Didn't notice the IPI the interrupt handler sent an | since interrupts were IPI to CPU 2 using | disabled on this CPU. smp_call_function_single_async() | = Enter _DISABLE_IRQ stage Enter _RUN stage Enter _RUN stage = Busy loop with interrupts | Invoke take_cpu_down() disabled. | and take CPU 2 offline = Enter _EXIT stage Enter _EXIT stage Re-enable interrupts Re-enable interrupts The pending IPI is noted immediately, but alas, the CPU is offline at this point. This of course, makes the smp-call-function IPI handler code running on CPU 2 unhappy and it complains about "receiving an IPI on an offline CPU". One real example of the scenario on CPU 1 is the block layer's complete-request call-path: __blk_complete_request() [interrupt-handler] raise_blk_irq() smp_call_function_single_async() However, if we look closely, the block layer does check that the target CPU is online before firing the IPI. So in this case, it is actually the unfortunate ordering/timing of events in the stop-machine phase that leads to receiving IPIs after the target CPU has gone offline. In reality, getting a late IPI on an offline CPU is not too bad by itself (this can happen even due to hardware latencies in IPI send-receive). It is a bug only if the target CPU really went offline without executing all the callbacks queued on its list. (Note that a CPU is free to execute its pending smp-call-function callbacks in a batch, without waiting for the corresponding IPIs to arrive for each one of those callbacks). So, fixing this issue can be broken up into two parts: 1. Ensure that a CPU goes offline only after executing all the callbacks queued on it. 2. Modify the warning condition in the smp-call-function IPI handler code such that it warns only if an offline CPU got an IPI *and* that CPU had gone offline with callbacks still pending in its queue. Achieving part 1 is straight-forward - just flush (execute) all the queued callbacks on the outgoing CPU in the CPU_DYING stage[1], including those callbacks for which the source CPU's IPIs might not have been received on the outgoing CPU yet. Once we do this, an IPI that arrives late on the CPU going offline (either due to the race mentioned above, or due to hardware latencies) will be completely harmless, since the outgoing CPU would have executed all the queued callbacks before going offline. Overall, this fix (parts 1 and 2 put together) additionally guarantees that we will see a warning only when the *IPI-sender code* is buggy - that is, if it queues the callback _after_ the target CPU has gone offline. [1]. The CPU_DYING part needs a little more explanation: by the time we execute the CPU_DYING notifier callbacks, the CPU would have already been marked offline. But we want to flush out the pending callbacks at this stage, ignoring the fact that the CPU is offline. So restructure the IPI handler code so that we can by-pass the "is-cpu-offline?" check in this particular case. (Of course, the right solution here is to fix CPU hotplug to mark the CPU offline _after_ invoking the CPU_DYING notifiers, but this requires a lot of audit to ensure that this change doesn't break any existing code; hence lets go with the solution proposed above until that is done). [akpm@linux-foundation.org: coding-style fixes] Signed-off-by: Srivatsa S. Bhat Suggested-by: Frederic Weisbecker Cc: "Paul E. McKenney" Cc: Borislav Petkov Cc: Christoph Hellwig Cc: Frederic Weisbecker Cc: Gautham R Shenoy Cc: Ingo Molnar Cc: Mel Gorman Cc: Mike Galbraith Cc: Oleg Nesterov Cc: Peter Zijlstra Cc: Rafael J. Wysocki Cc: Rik van Riel Cc: Rusty Russell Cc: Srivatsa S. Bhat Cc: Steven Rostedt Cc: Tejun Heo Cc: Thomas Gleixner Signed-off-by: Andrew Morton --- Update: Move the switch-case handling for CPU_DYING such that CPU_UP_CANCELED falls-through to CPU_DEAD (instead of CPU_DYING). Also, add an explicit comment mentioning this. kernel/smp.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 49 insertions(+), 8 deletions(-) diff --git a/kernel/smp.c b/kernel/smp.c index 306f818..80c33f8 100644 --- a/kernel/smp.c +++ b/kernel/smp.c @@ -29,6 +29,8 @@ static DEFINE_PER_CPU_SHARED_ALIGNED(struct call_function_data, cfd_data); static DEFINE_PER_CPU_SHARED_ALIGNED(struct llist_head, call_single_queue); +static void flush_smp_call_function_queue(bool warn_cpu_offline); + static int hotplug_cfd(struct notifier_block *nfb, unsigned long action, void *hcpu) { @@ -51,12 +53,27 @@ hotplug_cfd(struct notifier_block *nfb, unsigned long action, void *hcpu) #ifdef CONFIG_HOTPLUG_CPU case CPU_UP_CANCELED: case CPU_UP_CANCELED_FROZEN: + /* Fall-through to the CPU_DEAD[_FROZEN] case. */ case CPU_DEAD: case CPU_DEAD_FROZEN: free_cpumask_var(cfd->cpumask); free_percpu(cfd->csd); break; + + case CPU_DYING: + case CPU_DYING_FROZEN: + /* + * The IPIs for the smp-call-function callbacks queued by other + * CPUs might arrive late, either due to hardware latencies or + * because this CPU disabled interrupts (inside stop-machine) + * before the IPIs were sent. So flush out any pending callbacks + * explicitly (without waiting for the IPIs to arrive), to + * ensure that the outgoing CPU doesn't go offline with work + * still pending. + */ + flush_smp_call_function_queue(false); + break; #endif }; @@ -177,23 +194,47 @@ static int generic_exec_single(int cpu, struct call_single_data *csd, return 0; } -/* - * Invoked by arch to handle an IPI for call function single. Must be - * called from the arch with interrupts disabled. +/** + * generic_smp_call_function_single_interrupt - Execute SMP IPI callbacks + * + * Invoked by arch to handle an IPI for call function single. + * Must be called with interrupts disabled. */ void generic_smp_call_function_single_interrupt(void) { + flush_smp_call_function_queue(true); +} + +/** + * flush_smp_call_function_queue - Flush pending smp-call-function callbacks + * + * @warn_cpu_offline: If set to 'true', warn if callbacks were queued on an + * offline CPU. Skip this check if set to 'false'. + * + * Flush any pending smp-call-function callbacks queued on this CPU. This is + * invoked by the generic IPI handler, as well as by a CPU about to go offline, + * to ensure that all pending IPI callbacks are run before it goes completely + * offline. + * + * Loop through the call_single_queue and run all the queued callbacks. + * Must be called with interrupts disabled. + */ +static void flush_smp_call_function_queue(bool warn_cpu_offline) +{ + struct llist_head *head; struct llist_node *entry; struct call_single_data *csd, *csd_next; static bool warned; - entry = llist_del_all(&__get_cpu_var(call_single_queue)); + WARN_ON(!irqs_disabled()); + + head = &__get_cpu_var(call_single_queue); + entry = llist_del_all(head); entry = llist_reverse_order(entry); - /* - * Shouldn't receive this interrupt on a cpu that is not yet online. - */ - if (unlikely(!cpu_online(smp_processor_id()) && !warned)) { + /* There shouldn't be any pending callbacks on an offline CPU. */ + if (unlikely(warn_cpu_offline && !cpu_online(smp_processor_id()) && + !warned && !llist_empty(head))) { warned = true; WARN(1, "IPI on offline CPU %d\n", smp_processor_id());