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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
