Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751962AbbFZM6c (ORCPT ); Fri, 26 Jun 2015 08:58:32 -0400 Received: from foss.arm.com ([217.140.101.70]:60079 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751561AbbFZM6X (ORCPT ); Fri, 26 Jun 2015 08:58:23 -0400 Message-ID: <558D4C6B.4090702@arm.com> Date: Fri, 26 Jun 2015 13:58:19 +0100 From: Sudeep Holla User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Preeti U Murthy , Thomas Gleixner CC: Sudeep Holla , "linux-kernel@vger.kernel.org" , Suzuki Poulose , Lorenzo Pieralisi , Catalin Marinas , "Rafael J. Wysocki" Subject: Re: [PATCH] clockevents: return error from tick_broadcast_oneshot_control if !GENERIC_CLOCKEVENTS_BROADCAST References: <1435228065-2428-1-git-send-email-sudeep.holla@arm.com> <558C1E97.4020206@arm.com> <558CDE57.3050008@linux.vnet.ibm.com> <558D46C5.8080307@linux.vnet.ibm.com> In-Reply-To: <558D46C5.8080307@linux.vnet.ibm.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3535 Lines: 96 On 26/06/15 13:34, Preeti U Murthy wrote: > On 06/26/2015 01:17 PM, Thomas Gleixner wrote: >> diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c >> index d39f32cdd1b5..281ce29d295e 100644 >> --- a/kernel/time/tick-broadcast.c >> +++ b/kernel/time/tick-broadcast.c >> @@ -662,46 +662,39 @@ static void broadcast_shutdown_local(struct clock_event_device *bc, >> clockevents_switch_state(dev, CLOCK_EVT_STATE_SHUTDOWN); >> } >> >> -/** >> - * tick_broadcast_oneshot_control - Enter/exit broadcast oneshot mode >> - * @state: The target state (enter/exit) >> - * >> - * The system enters/leaves a state, where affected devices might stop >> - * Returns 0 on success, -EBUSY if the cpu is used to broadcast wakeups. >> - * >> - * Called with interrupts disabled, so clockevents_lock is not >> - * required here because the local clock event device cannot go away >> - * under us. >> - */ >> -int tick_broadcast_oneshot_control(enum tick_broadcast_state state) >> +int __tick_broadcast_oneshot_control(enum tick_broadcast_state state) >> { >> struct clock_event_device *bc, *dev; >> - struct tick_device *td; >> int cpu, ret = 0; >> ktime_t now; >> >> - /* >> - * Periodic mode does not care about the enter/exit of power >> - * states >> - */ >> - if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC) >> - return 0; >> + if (!tick_broadcast_device.evtdev) >> + return -EBUSY; >> >> - /* >> - * We are called with preemtion disabled from the depth of the >> - * idle code, so we can't be moved away. >> - */ >> - td = this_cpu_ptr(&tick_cpu_device); >> - dev = td->evtdev; >> - >> - if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) >> - return 0; >> + dev = this_cpu_ptr(&tick_cpu_device)->evtdev; >> >> raw_spin_lock(&tick_broadcast_lock); >> bc = tick_broadcast_device.evtdev; >> cpu = smp_processor_id(); >> >> if (state == TICK_BROADCAST_ENTER) { >> + /* >> + * If the current CPU owns the hrtimer broadcast >> + * mechanism, it cannot go deep idle and we do not add >> + * the CPU to the broadcast mask. We don't have to go >> + * through the EXIT path as the local timer is not >> + * shutdown. >> + */ >> + ret = broadcast_needs_cpu(bc, cpu); >> + >> + /* >> + * If the hrtimer broadcast check tells us that the >> + * cpu cannot go deep idle, or if the broadcast device >> + * is in periodic mode, we just return. >> + */ >> + if (ret || tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC) >> + goto out; > > The check on PERIODIC mode is good, but I don't see the point of moving > broadcast_needs_cpu() up above. broadcast_shutdown_local() calls > broadcast_needs_cpu() internally. > > Besides, by jumping to 'out', we will miss programming the broadcast > hrtimer in tick_broadcast_set_event() below, if the cpu happen to be the > broadcast cpu(which is why it was not allowed to go to deep idle). > I tested the updated patch and found issues. I am seeing some random behaviour(with GENERIC_CLOCKEVENTS_BROADCAST=y TICK_ONESHOT=y): 1. sometimes all the CPUs have entered deeper idle states(though very rare, finding it difficult to hit this scenario) 2. few other times I see one CPU in shallow state which matches the above explanation of missing hrtimer programming. Regards, Sudeep -- 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/