Re: [PATCH v2] cpuidle: Use nanoseconds as the unit of time

[Rafael J. Wysocki]

On Sun, Nov 10, 2019 at 11:12 PM Doug Smythies <dsmythies@xxxxxxxxx> wrote:
>
> On 2019.11.10 10:10 Doug Smythies wrote:
> > On 2019.11.10 09:24 Rafael J. Wysocki wrote:
> >> On Sunday, November 10, 2019 5:48:21 PM CET Rafael J. Wysocki wrote:
> >>
> >> I have found a bug, which should be addressed by the patch below.
> >>
> >> If it still doesn't reduce the discrepancy, we'll need to look further.
> >>
> >> ---
> >> drivers/cpuidle/governors/menu.c |    4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> Index: linux-pm/drivers/cpuidle/governors/menu.c
> >> ===================================================================
> >> --- linux-pm.orig/drivers/cpuidle/governors/menu.c
> >> +++ linux-pm/drivers/cpuidle/governors/menu.c
> >> @@ -516,8 +516,8 @@ static void menu_update(struct cpuidle_d
> >>      new_factor -= new_factor / DECAY;
> >>
> >>      if (data->next_timer_ns > 0 && measured_ns < MAX_INTERESTING)
> >> -            new_factor += RESOLUTION * div64_u64(measured_ns,
> >> -                                                 data->next_timer_ns);
> >> +            new_factor += div64_u64(RESOLUTION * measured_ns,
> >> +                                    data->next_timer_ns);
> >>      else
> >>              /*
> >>               * we were idle so long that we count it as a perfect
> >
> > Yes, that was the exact bit of code I focused on yesterday.
> > However, my attempt to fix was different, and made no difference,
> > with the new graph being exactly on top of the old bad one.
> > I had defined new_factor as u64 and RESOLUTION as ULL.
>
> Your patch does fix the problem.
> I now also understand why my attempt did not.
>
> New data added to previous graphs. For those that don't
> want to go to the graphs, the nanosecond menu graphs are now
> almost identical to the microsecond based one.
>
> http://www.smythies.com/~doug/linux/idle/nano-second-conversion/sweep/index.html
>
> Legend:
> teo-base : linux-next 2019.11.07
> menu-base: linux-next 2019.11.07
> teo-v2   : linux-next 2019.11.07 + cpuidle: Consolidate disabled state checks + this v2
> menu-v2  : linux-next 2019.11.07 + cpuidle: Consolidate disabled state checks + this v2
> rjw1     : menu-v2 + above patch.
>
> Acked-by and tested-by Doug Smythies <dsmythies@xxxxxxxxx>

Thanks a lot!

> Disclaimer: Only teo and menu, not ladder or haltpoll governors.
>
> Additional suggestions:
>
> Header comments:
>
> > microseconds provided by drivers.  In addition to that, change
> > cpuidle_governor_latency_req() to return the idle state exit
> > latency constraint in nanoseconds.
>
> Suggest:
>
> microseconds provided by drivers.  Additionally, change
> cpuidle_governor_latency_req() to return the idle state exit
> latency constraint in nanoseconds.
>
> > With that, meeasure idle state residency (last_residency_ns in
>              ^^^^^^^^
> Suggest:
>
> Also measure idle state residency (last_residency_ns in
>
> Code:
> Suggest deletion of this note:
>
> /*
>  * Please note when changing the tuning values:
>  * If (MAX_INTERESTING-1) * RESOLUTION > UINT_MAX, the result of
>  * a scaling operation multiplication may overflow on 32 bit platforms.
>  * In that case, #define RESOLUTION as ULL to get 64 bit result:
>  * #define RESOLUTION 1024ULL
>  *
>  * The default values do not overflow.
>  */
>
> Because you have managed the extra bit requirements as part of the patch.

Good suggestion! :-)

I have folded the fix and the removal of the comment as suggested
above into to v2 and applied the resulting patch with tags from you
and Peter.

Also the changelog has been updated as suggested.

Thanks!