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

[Doug Smythies]

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>

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.

... Doug