On Tue, Feb 28, 2012 at 7:50 AM, Rob Lee [off-list ref] wrote:

On Mon, Feb 27, 2012 at 6:49 PM, Turquette, Mike [off-list ref] wrote:

quoted

On Sun, Feb 26, 2012 at 8:47 PM, Robert Lee [off-list ref] wrote:

quoted

+/**
+ * cpuidle_enter_wrap - performing timekeeping and irq around enter function
+ * @dev: pointer to a valid cpuidle_device object
+ * @drv: pointer to a valid cpuidle_driver object
+ * @index: index of the target cpuidle state.
+ */
+static inline int cpuidle_wrap_enter(struct cpuidle_device *dev,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct cpuidle_driver *drv, int index,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? int (*enter)(struct cpuidle_device *dev,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct cpuidle_driver *drv, int index))
+{
+ ? ? ? ktime_t time_start, time_end;
+ ? ? ? s64 diff;
+
+ ? ? ? time_start = ktime_get();
+
+ ? ? ? index = enter(dev, drv, index);
+
+ ? ? ? time_end = ktime_get();
+
+ ? ? ? local_irq_enable();
+
+ ? ? ? diff = ktime_to_us(ktime_sub(time_end, time_start));
+ ? ? ? if (diff > INT_MAX)
+ ? ? ? ? ? ? ? diff = INT_MAX;
+
+ ? ? ? dev->last_residency = (int) diff;
+
+ ? ? ? return index;
+}

Hi Rob,

In a previous series I brought up the idea of not accounting for time
if a C-state transition fails. ?My post on that thread can be found
here:
http://article.gmane.org/gmane.linux.ports.arm.kernel/149293/

How do you feel about adding something like the following?

if (IS_ERR(index))
? ? ? ?dev->last_residency = 0;
? ? ? ?return index;

Obviously it will up to the platforms to figure out how to propagate
that error up from their respective low power code.

To be completely clear on what you're asking for, from
cpuidle_idle_call in drivers/cpuidle/cpuidle.c:

...
? ? ? ?target_state = &drv->states[next_state];

? ? ? ?trace_power_start(POWER_CSTATE, next_state, dev->cpu);
? ? ? ?trace_cpu_idle(next_state, dev->cpu);

? ? ? ?entered_state = target_state->enter(dev, drv, next_state);

? ? ? ?trace_power_end(dev->cpu);
? ? ? ?trace_cpu_idle(PWR_EVENT_EXIT, dev->cpu);

? ? ? ?if (entered_state >= 0) {
? ? ? ? ? ? ? ?/* Update cpuidle counters */
? ? ? ? ? ? ? ?/* This can be moved to within driver enter routine
? ? ? ? ? ? ? ? * but that results in multiple copies of same code.
? ? ? ? ? ? ? ? */
? ? ? ? ? ? ? ?dev->states_usage[entered_state].time +=
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?(unsigned long long)dev->last_residency;
? ? ? ? ? ? ? ?dev->states_usage[entered_state].usage++;
? ? ? ?}
...

Note the "if (entered_state >= 0)". ?This ultimately prevents the
cpuidle device time accounting upon an negative value being returned.
So are you asking for the if(IS_ERR(index)) check to prevent the
unnecessary last_residency time calculation in the wrapper, or to make
sure a last_residency is zero upon failure? ?(or both?)

It seems like a bug (or lack or documentation at best) in the code
that exists today to not zero out dev->last_residency upon a negative
return value as this value is used by the governors upon the next
idle. ?So to ensure last_residency is 0 upon failure, I think it'd be
best to add that to an new else statement immediately following the
"if (entered_state >=))" so that any platform cpuidle driver that
returns a negative will have the last_residency zeroed out, not just
those that use en_core_tk_irqen.

+ Cc: Jon Hunter

Hi Rob,

I didn't review the code carefully enough to catch the 'if
(entered_state >= 0)' part, but that seems like a graceful way to
solve this problem by appending the 'else' statement on there and
seeting last_residency to zero.

I brought this topic up internally and Jon suggested that the 'usage'
statistics that are reported in sysfs should also reflect failed
versus successful C-state transitions, which is a great idea.  This
could simply be achieved by renaming the current 'usage' count to
something like 'transitions_attempted' and then conditionally
increment a new counter within the 'if (entered_state >= 0)' block,
perhaps named, 'transition_succeeded'.

This way the old 'usage' count paradigm is as accurate as the new
time-keeping code.  Being able to easily observe which C-state tend to
fail the most would be invaluable in tuning idle policy for maximum
effectiveness.

Thoughts?

Regards,
Mike

quoted

Regards,
Mike