From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752936AbbDBKme (ORCPT ); Thu, 2 Apr 2015 06:42:34 -0400 Received: from mail-wg0-f47.google.com ([74.125.82.47]:36037 "EHLO mail-wg0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752606AbbDBKmb (ORCPT ); Thu, 2 Apr 2015 06:42:31 -0400 Date: Thu, 2 Apr 2015 12:42:27 +0200 From: Ingo Molnar To: Preeti U Murthy Cc: peterz@infradead.org, mpe@ellerman.id.au, tglx@linutronix.de, rjw@rjwysocki.net, nicolas.pitre@linaro.org, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH V2] clockevents: Fix cpu down race for hrtimer based broadcasting Message-ID: <20150402104226.GB21105@gmail.com> References: <20150330092410.24979.59887.stgit@preeti.in.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150330092410.24979.59887.stgit@preeti.in.ibm.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Preeti U Murthy wrote: > It was found when doing a hotplug stress test on POWER, that the machine > either hit softlockups or rcu_sched stall warnings. The issue was > traced to commit 7cba160ad789a powernv/cpuidle: Redesign idle states > management, which exposed the cpu down race with hrtimer based broadcast > mode(Commit 5d1638acb9f6(tick: Introduce hrtimer based broadcast). This > is explained below. > > Assume CPU1 is the CPU which holds the hrtimer broadcasting duty before > it is taken down. > > CPU0 CPU1 > > cpu_down() take_cpu_down() > disable_interrupts() > > cpu_die() > > while(CPU1 != CPU_DEAD) { > msleep(100); > switch_to_idle(); > stop_cpu_timer(); > schedule_broadcast(); > } > > tick_cleanup_cpu_dead() > take_over_broadcast() > > So after CPU1 disabled interrupts it cannot handle the broadcast hrtimer > anymore, so CPU0 will be stuck forever. > > Fix this by explicitly taking over broadcast duty before cpu_die(). > This is a temporary workaround. What we really want is a callback in the > clockevent device which allows us to do that from the dying CPU by > pushing the hrtimer onto a different cpu. That might involve an IPI and > is definitely more complex than this immediate fix. So why not use a suitable CPU_DOWN* notifier for this, instead of open coding it all into a random place in the hotplug machinery? Also, I improved the changelog (attached below), but decided against applying it until these questions are cleared - please use that for future versions of this patch. Thanks, Ingo ===================> >>From 413fbf5193b330c5f478ef7aaeaaee08907a993e Mon Sep 17 00:00:00 2001 From: Preeti U Murthy Date: Mon, 30 Mar 2015 14:59:19 +0530 Subject: [PATCH] clockevents: Fix cpu_down() race for hrtimer based broadcasting It was found when doing a hotplug stress test on POWER, that the machine either hit softlockups or rcu_sched stall warnings. The issue was traced to commit: 7cba160ad789 ("powernv/cpuidle: Redesign idle states management") which exposed the cpu_down() race with hrtimer based broadcast mode: 5d1638acb9f6 ("tick: Introduce hrtimer based broadcast") The race is the following: Assume CPU1 is the CPU which holds the hrtimer broadcasting duty before it is taken down. CPU0 CPU1 cpu_down() take_cpu_down() disable_interrupts() cpu_die() while (CPU1 != CPU_DEAD) { msleep(100); switch_to_idle(); stop_cpu_timer(); schedule_broadcast(); } tick_cleanup_cpu_dead() take_over_broadcast() So after CPU1 disabled interrupts it cannot handle the broadcast hrtimer anymore, so CPU0 will be stuck forever. Fix this by explicitly taking over broadcast duty before cpu_die(). This is a temporary workaround. What we really want is a callback in the clockevent device which allows us to do that from the dying CPU by pushing the hrtimer onto a different cpu. That might involve an IPI and is definitely more complex than this immediate fix. Changelog was picked up from: https://lkml.org/lkml/2015/2/16/213 Suggested-by: Thomas Gleixner Tested-by: Nicolas Pitre Signed-off-by: Preeti U. Murthy Cc: linuxppc-dev@lists.ozlabs.org Cc: mpe@ellerman.id.au Cc: nicolas.pitre@linaro.org Cc: peterz@infradead.org Cc: rjw@rjwysocki.net Fixes: http://linuxppc.10917.n7.nabble.com/offlining-cpus-breakage-td88619.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wi0-x232.google.com (mail-wi0-x232.google.com [IPv6:2a00:1450:400c:c05::232]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4BF3B1A03D6 for ; Thu, 2 Apr 2015 21:42:33 +1100 (AEDT) Received: by wizk4 with SMTP id k4so10610049wiz.1 for ; Thu, 02 Apr 2015 03:42:30 -0700 (PDT) Sender: Ingo Molnar Date: Thu, 2 Apr 2015 12:42:27 +0200 From: Ingo Molnar To: Preeti U Murthy Subject: Re: [PATCH V2] clockevents: Fix cpu down race for hrtimer based broadcasting Message-ID: <20150402104226.GB21105@gmail.com> References: <20150330092410.24979.59887.stgit@preeti.in.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <20150330092410.24979.59887.stgit@preeti.in.ibm.com> Cc: nicolas.pitre@linaro.org, peterz@infradead.org, rjw@rjwysocki.net, linux-kernel@vger.kernel.org, tglx@linutronix.de, linuxppc-dev@lists.ozlabs.org List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , * Preeti U Murthy wrote: > It was found when doing a hotplug stress test on POWER, that the machine > either hit softlockups or rcu_sched stall warnings. The issue was > traced to commit 7cba160ad789a powernv/cpuidle: Redesign idle states > management, which exposed the cpu down race with hrtimer based broadcast > mode(Commit 5d1638acb9f6(tick: Introduce hrtimer based broadcast). This > is explained below. > > Assume CPU1 is the CPU which holds the hrtimer broadcasting duty before > it is taken down. > > CPU0 CPU1 > > cpu_down() take_cpu_down() > disable_interrupts() > > cpu_die() > > while(CPU1 != CPU_DEAD) { > msleep(100); > switch_to_idle(); > stop_cpu_timer(); > schedule_broadcast(); > } > > tick_cleanup_cpu_dead() > take_over_broadcast() > > So after CPU1 disabled interrupts it cannot handle the broadcast hrtimer > anymore, so CPU0 will be stuck forever. > > Fix this by explicitly taking over broadcast duty before cpu_die(). > This is a temporary workaround. What we really want is a callback in the > clockevent device which allows us to do that from the dying CPU by > pushing the hrtimer onto a different cpu. That might involve an IPI and > is definitely more complex than this immediate fix. So why not use a suitable CPU_DOWN* notifier for this, instead of open coding it all into a random place in the hotplug machinery? Also, I improved the changelog (attached below), but decided against applying it until these questions are cleared - please use that for future versions of this patch. Thanks, Ingo ===================> >>From 413fbf5193b330c5f478ef7aaeaaee08907a993e Mon Sep 17 00:00:00 2001 From: Preeti U Murthy Date: Mon, 30 Mar 2015 14:59:19 +0530 Subject: [PATCH] clockevents: Fix cpu_down() race for hrtimer based broadcasting It was found when doing a hotplug stress test on POWER, that the machine either hit softlockups or rcu_sched stall warnings. The issue was traced to commit: 7cba160ad789 ("powernv/cpuidle: Redesign idle states management") which exposed the cpu_down() race with hrtimer based broadcast mode: 5d1638acb9f6 ("tick: Introduce hrtimer based broadcast") The race is the following: Assume CPU1 is the CPU which holds the hrtimer broadcasting duty before it is taken down. CPU0 CPU1 cpu_down() take_cpu_down() disable_interrupts() cpu_die() while (CPU1 != CPU_DEAD) { msleep(100); switch_to_idle(); stop_cpu_timer(); schedule_broadcast(); } tick_cleanup_cpu_dead() take_over_broadcast() So after CPU1 disabled interrupts it cannot handle the broadcast hrtimer anymore, so CPU0 will be stuck forever. Fix this by explicitly taking over broadcast duty before cpu_die(). This is a temporary workaround. What we really want is a callback in the clockevent device which allows us to do that from the dying CPU by pushing the hrtimer onto a different cpu. That might involve an IPI and is definitely more complex than this immediate fix. Changelog was picked up from: https://lkml.org/lkml/2015/2/16/213 Suggested-by: Thomas Gleixner Tested-by: Nicolas Pitre Signed-off-by: Preeti U. Murthy Cc: linuxppc-dev@lists.ozlabs.org Cc: mpe@ellerman.id.au Cc: nicolas.pitre@linaro.org Cc: peterz@infradead.org Cc: rjw@rjwysocki.net Fixes: http://linuxppc.10917.n7.nabble.com/offlining-cpus-breakage-td88619.html