On 06/23/2016 05:18 AM, Balbir Singh wrote:
>
>
> On 23/06/16 05:36, Shreyas B. Prabhu wrote:
>> Snooze is a poll idle state in powernv and pseries platforms. Snooze
>> has a timeout so that if a cpu stays in snooze for more than target
>> residency of the next available idle state, then it would exit thereby
>> giving chance to the cpuidle governor to re-evaluate and
>> promote the cpu to a deeper idle state. Therefore whenever snooze exits
>> due to this timeout, its last_residency will be target_residency of next
>> deeper state.
>>
>> commit e93e59ce5b85 ("cpuidle: Replace ktime_get() with local_clock()")
>> changed the math around last_residency calculation. Specifically, while
>> converting last_residency value from nanoseconds to microseconds it does
>> right shift by 10. Due to this, in snooze timeout exit scenarios
>> last_residency calculated is roughly 2.3% less than target_residency of
>> next available state. This pattern is picked up get_typical_interval()
>> in the menu governor and therefore expected_interval in menu_select() is
>> frequently less than the target_residency of any state but snooze.
>>
>> Due to this we are entering snooze at a higher rate, thereby affecting
>> the single thread performance.
>> Since the math around last_residency is not meant to be precise, fix this
>> issue setting snooze timeout to 105% of target_residency of next
>> available idle state.
>>
>> This also adds comment around why snooze timeout is necessary.
>>
>> Reported-by: Anton Blanchard <an...@samba.org>
>> Signed-off-by: Shreyas B. Prabhu <shre...@linux.vnet.ibm.com>
>> ---
>> drivers/cpuidle/cpuidle-powernv.c | 14 ++++++++++++++
>> drivers/cpuidle/cpuidle-pseries.c | 13 +++++++++++++
>> 2 files changed, 27 insertions(+)
>>
>> diff --git a/drivers/cpuidle/cpuidle-powernv.c
>> b/drivers/cpuidle/cpuidle-powernv.c
>> index e12dc30..5835491 100644
>> --- a/drivers/cpuidle/cpuidle-powernv.c
>> +++ b/drivers/cpuidle/cpuidle-powernv.c
>> @@ -268,10 +268,24 @@ static int powernv_idle_probe(void)
>> cpuidle_state_table = powernv_states;
>> /* Device tree can indicate more idle states */
>> max_idle_state = powernv_add_idle_states();
>> +
>> + /*
>> + * Staying in snooze for a long period can degrade the
>> + * perfomance of the sibling cpus. Set timeout for snooze such
>> + * that if the cpu stays in snooze longer than target residency
>> + * of the next available idle state then exit from snooze. This
>> + * gives a chance to the cpuidle governor to re-evaluate and
>> + * promote it to deeper idle states.
>> + */
>> if (max_idle_state > 1) {
>> snooze_timeout_en = true;
>> snooze_timeout = powernv_states[1].target_residency *
>> tb_ticks_per_usec;
>> + /*
>> + * Give a 5% margin since target residency related math
>> + * is not precise in cpuidle core.
>> + */
>
> Is this due to the microsecond conversion mentioned above? It would be nice to
> have it in the comment. Does
>
> (powernv_states[1].target_residency + tb_ticks_per_usec) / tb_ticks_per_usec
> solve
> your rounding issues, assuming the issue is really rounding or maybe it is due
> to the shift by 10, could you please elaborate on what related math is not
> precise? That would explain to me why I missed understanding your changes.
>
>> + snooze_timeout += snooze_timeout / 20;
>
> For now 5% is sufficient, but do you want to check to assert to check if
>
> snooze_timeout (in microseconds) / tb_ticks_per_usec >
> powernv_states[i].target_residency?
>
This is not a rounding issue. As I mentioned in the commit message, this
is because of the last_residency calculation in cpuidle.c.
To elaborate, last residency calculation is done in the following way
after commit e93e59ce5b85 ("cpuidle: Replace ktime_get() with
local_clock()") -
cpuidle_enter_state()
{
[...]
time_start = local_clock();
[enter idle state]
time_end = local_clock();
/*
* local_clock() returns the time in nanosecond, let's shift
* by 10 (divide by 1024) to have microsecond based time.
*/
diff = (time_end - time_start) >> 10;
[...]
dev->last_residency = (int) diff;
}
Because of >>10 as opposed to /1000, last_residency is lesser by 2.3%
In snooze timeout exit scenarios because of this, last_residency
calculated is 2.3% less than target_residency of next available state.
This affects get_typical_interval() in the menu governor and therefore
expected_interval in menu_select() is frequently less than the
target_residency of any state but snooze.
I'll expand the comments as you suggested.
Thanks,
Shreyas
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
