On Monday, February 09, 2015 04:44:08 PM Peter Zijlstra wrote: > On Mon, Feb 09, 2015 at 03:54:22AM +0100, Rafael J. Wysocki wrote: > > Complete patch with that modification is appended. In the next few days I'm > > going to split it into smaller parts and send along with cpuidle driver > > patches implementing ->enter_freeze. > > > > Please let me know what you think. > > > @@ -104,6 +105,21 @@ static void cpuidle_idle_call(void) > > rcu_idle_enter(); > > > > /* > > + * Suspend-to-idle ("freeze") is a system state in which all user space > > + * has been frozen, all I/O devices have been suspended and the only > > + * activity happens here and in iterrupts (if any). In that case bypass > > + * the cpuidle governor and go stratight for the deepest idle state > > + * available. Possibly also suspend the local tick and the entire > > + * timekeeping to prevent timer interrupts from kicking us out of idle > > + * until a proper wakeup interrupt happens. > > + */ > > + if (idle_should_freeze()) { > > + cpuidle_enter_freeze(); > > + local_irq_enable(); > > + goto exit_idle; > > + } > > + > > + /* > > * Ask the cpuidle framework to choose a convenient idle state. > > * Fall back to the default arch idle method on errors. > > */ > > I was hoping to not have to put that into the regular idle path; say > maybe share a single special branch with the play-dead call. People seem > to start complaining about the total amount of time it takes to just > 'run' the idle path.
Well, this check really only replaces one in cpuidle_select() and another one in cpuidle_reflect() and they both are called from the idle path, so that's a net gain rather. :-) > Now I don't think we can do that, because we need the > arch_cpu_idle_enter() nonsense for the one but not the other; also all > this really only makes sense in the cpuidle context, so nothing to be > done about that. Agreed. > In any case, you could make that: > > static inline bool idle_should_freeze(void) > { > return unlikely(suspend_freeze_state == FREEZE_STATE_ENTER); > } > > which should help a bit I suppose. Won't hurt at least. > > +static void enter_freeze_proper(struct cpuidle_driver *drv, > > + struct cpuidle_device *dev, int index) > > +{ > > + tick_freeze(); > > + drv->states[index].enter_freeze(dev, drv, index); > > This is slightly different from cpuidle_enter() in that it does not > consider the coupled states nonsense, is that on purpose? And if so, > does that want a comment? Yes, it is on purpose and the reason why is because the coupled thing re-enables interrupts (unconditionally). [And it contains a comment about that which does not make sense to me now that I read it.] I can add a comment about that, though. > > + /* > > + * timekeeping_resume() that will be called by tick_unfreeze() for > > the > > + * last CPU executing it calls functions containing RCU read-side > > + * critical sections, so tell RCU about that. > > + */ > > + RCU_NONIDLE(tick_unfreeze()); > > +} > > > But over all it looks fine to me. Cool, thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/