From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1030218AbbD1NHI (ORCPT ); Tue, 28 Apr 2015 09:07:08 -0400 Received: from v094114.home.net.pl ([79.96.170.134]:57431 "HELO v094114.home.net.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S965502AbbD1NHD (ORCPT ); Tue, 28 Apr 2015 09:07:03 -0400 From: "Rafael J. Wysocki" To: Linus Walleij Cc: "Rafael J. Wysocki" , Sudeep Holla , Daniel Lezcano , Linux PM list , Thomas Gleixner , Peter Zijlstra , Ingo Molnar , Linux Kernel Mailing List , ACPI Devel Maling List Subject: Re: [PATCH 16/20] sched/idle: Use explicit broadcast oneshot control function Date: Tue, 28 Apr 2015 15:31:54 +0200 Message-ID: <8051605.iKEaLUIlGb@vostro.rjw.lan> User-Agent: KMail/4.11.5 (Linux/4.0.0+; KDE/4.11.5; x86_64; ; ) In-Reply-To: References: <2112147.0kYCHhbEJT@vostro.rjw.lan> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="utf-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tuesday, April 28, 2015 02:37:10 PM Linus Walleij wrote: > On Tue, Apr 28, 2015 at 2:19 PM, Rafael J. Wysocki wrote: > > Sudeep: > >> At-least I observed issue only when I am using hardware broadcast timer. > >> It doesn't hang when I am using hrtimer as broadcast timer in which case > >> one of the cpu will be not enter deeper idle states that lose timer. > >> I will rerun on v4.1-rc1 and post the complete log. > > > > So the bug here is that cpuidle_enter() enables interrupts, so the > > assumption about them being not enabled made by > > tick_broadcast_oneshot_control() is actually not valid. > > > > It looks like we need to acquire the clockevents_lock at least in this > > particular case. Let me see where to put it and I'll send a patch for > > testing. > > Aha that looks very much like it. Put me on the patch and I'll > take it for a spin. OK, so something like the below for starters (the _irqsave variant is used to avoid adding one more WARN_ON(irqs_disabled()) in there). I haven't tested it, but then I can't reproduce the original issue in the first place. --- include/linux/clockchips.h | 2 ++ kernel/sched/idle.c | 2 +- kernel/time/clockevents.c | 15 +++++++++++++++ 3 files changed, 18 insertions(+), 1 deletion(-) Index: linux-pm/include/linux/clockchips.h =================================================================== --- linux-pm.orig/include/linux/clockchips.h +++ linux-pm/include/linux/clockchips.h @@ -198,9 +198,11 @@ extern int tick_receive_broadcast(void); # if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && defined(CONFIG_TICK_ONESHOT) extern void tick_setup_hrtimer_broadcast(void); extern int tick_check_broadcast_expired(void); +extern void tick_broadcast_exit_idle(void); # else static inline int tick_check_broadcast_expired(void) { return 0; } static inline void tick_setup_hrtimer_broadcast(void) { } +static inline void tick_broadcast_exit_idle(void) { } # endif extern int clockevents_notify(unsigned long reason, void *arg); Index: linux-pm/kernel/time/clockevents.c =================================================================== --- linux-pm.orig/kernel/time/clockevents.c +++ linux-pm/kernel/time/clockevents.c @@ -735,6 +735,21 @@ static ssize_t sysfs_unbind_tick_dev(str static DEVICE_ATTR(unbind_device, 0200, NULL, sysfs_unbind_tick_dev); #ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST +/** + * tick_broadcast_exit_idle - Broadcast oneshot mode exit for the idle loop. + * + * The idle loop exits the broadcast oneshot mode with interrupts enabled, so + * clockevents_lock needs to be acquired around that. + */ +void tick_broadcast_exit_idle(void) +{ + unsigned long flags; + + raw_spin_lock_irqsave(&clockevents_lock, flags); + tick_broadcast_exit(); + raw_spin_unlock_irqrestore(&clockevents_lock, flags); +} + static struct device tick_bc_dev = { .init_name = "broadcast", .id = 0, Index: linux-pm/kernel/sched/idle.c =================================================================== --- linux-pm.orig/kernel/sched/idle.c +++ linux-pm/kernel/sched/idle.c @@ -175,7 +175,7 @@ static void cpuidle_idle_call(void) idle_set_state(this_rq(), NULL); if (broadcast) - tick_broadcast_exit(); + tick_broadcast_exit_idle(); /* * Give the governor an opportunity to reflect on the outcome