From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932441AbbDOQHT (ORCPT ); Wed, 15 Apr 2015 12:07:19 -0400 Received: from mail-wi0-f174.google.com ([209.85.212.174]:38004 "EHLO mail-wi0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755195AbbDOQHI (ORCPT ); Wed, 15 Apr 2015 12:07:08 -0400 Message-ID: <552E8CAB.5060401@linaro.org> Date: Wed, 15 Apr 2015 18:07:07 +0200 From: Daniel Lezcano User-Agent: Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: "Rafael J. Wysocki" CC: Peter Zijlstra , "Rafael J. Wysocki" , Linux Kernel Mailing List , "linux-pm@vger.kernel.org" , nicolas.pitre@linaro.org Subject: Re: [PATCH 2/3] cpuidle: Add some comments in the cpuidle_enter function References: <1429092024-20498-1-git-send-email-daniel.lezcano@linaro.org> <1429092024-20498-2-git-send-email-daniel.lezcano@linaro.org> In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/15/2015 03:46 PM, Rafael J. Wysocki wrote: > On Wed, Apr 15, 2015 at 12:00 PM, Daniel Lezcano > 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 >> --- >> 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 -- Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog