Re: [PATCH 2/3] cpuidle: Add some comments in the cpuidle_enter function
From: Daniel Lezcano
Date: Wed Apr 15 2015 - 12:07:22 EST
On 04/15/2015 03:46 PM, Rafael J. Wysocki wrote:
On Wed, Apr 15, 2015 at 12:00 PM, Daniel Lezcano
<daniel.lezcano@xxxxxxxxxx> wrote:
The code is a bit poor in comments. Fix that by adding some comments in the
cpuidle enter function.
Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
---
drivers/cpuidle/cpuidle.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 1220dac..5e6c6be 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -162,19 +162,50 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
trace_cpu_idle_rcuidle(index, dev->cpu);
+ /*
+ * Store the idle start time for this cpu, this information
+ * will be used by cpuidle to measure how long the cpu has
+ * been idle and by the scheduler to prevent to wake it up too
+ * early
+ */
target_state->idle_stamp = ktime_to_us(ktime_get());
+ /*
+ * The enter to the low level idle routine. This call will block
+ * until an interrupt occurs meaning it is the end of the idle
+ * period
+ */
entered_state = target_state->enter(dev, drv, index);
+ /*
+ * Measure as soon as possible the duration of the idle
+ * period. It MUST be done before re-enabling the interrupt in
+ * order to prevent to add in the idle time measurement the
+ * interrupt handling duration
That's unless ->enter() itself re-enables interrupts which it may do, right?
Except I missed a change somewhere, I believe I removed all the
interrupts re-enablement in all the driver with latest commit 554c06ba3.
This is why we were able to move the local_irq_enable in this code path
and finally remove the CPUIDLE_FLAG_TIME_VALID.
Which is why we made governors use next_timer_us as the "measured"
value if the measured value itself is greater.
I'd just say "Compute the idle duration here to avoid adding interrupt
handling time to the idle time in case an interrupt occurs as soon as
re-enabled".
Ok, sounds good.
+ */
diff = ktime_to_us(ktime_sub_us(ktime_get(), target_state->idle_stamp));
+ /*
+ * Reset the idle time stamp as the scheduler may think the cpu is idle
+ * while it is in the process of waking up
+ */
target_state->idle_stamp = 0;
trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
+ /*
+ * The cpuidle_enter_coupled uses the cpuidle_enter function.
+ * Don't re-enable the interrupts and let the enter_coupled
"let the enter_coupled function wait" (without the "to").
Ok, thanks.
+ * function to wait for all cpus to sync and to enable the
+ * interrupts again from there
+ */
And I would just say "In the coupled case interrupts will be enabled
by cpuidle_enter_coupled()".
Ok.
if (!cpuidle_state_is_coupled(dev, drv, entered_state))
local_irq_enable();
+ /*
+ * The idle duration will be casted to an integer, prevent to
"will be cast" (like "Cast Away").
Yep.
+ * overflow by setting a boundary to INT_MAX
+ */
if (diff > INT_MAX)
diff = INT_MAX;
But anyway, instead of adding that comment, why not to change the code
like this:
dev->last_residency = diff < INT_MAX ? diff : INT_MAX;
and it now should be clear what happens without the comment.
Ok.
Thanks for the review.
-- Daniel
--
<http://www.linaro.org/> Linaro.org â Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/