[2/3] cpuidle: Add some comments in the cpuidle_enter function

Message ID 1429092024-20498-2-git-send-email-daniel.lezcano@linaro.org
State New
Headers show

Commit Message

Daniel Lezcano April 15, 2015, 10 a.m.
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@linaro.org>
---
 drivers/cpuidle/cpuidle.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

Comments

Daniel Lezcano April 15, 2015, 4:07 p.m. | #1
On 04/15/2015 03:46 PM, Rafael J. Wysocki wrote:
> On Wed, Apr 15, 2015 at 12:00 PM, Daniel Lezcano
> <daniel.lezcano@linaro.org> 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@linaro.org>
>> ---
>>   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

Patch

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
+	 */
 	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
+	 * function to wait for all cpus to sync and to enable the
+	 * interrupts again from there
+	 */
 	if (!cpuidle_state_is_coupled(dev, drv, entered_state))
 		local_irq_enable();
 
+	/*
+	 * The idle duration will be casted to an integer, prevent to
+	 * overflow by setting a boundary to INT_MAX
+	 */
 	if (diff > INT_MAX)
 		diff = INT_MAX;