Re: [PATCH v2] cpuidle-pseries: Fix CEDE latency conversion from tb to us

[Vaidyanathan Srinivasan]

* Gautham R Shenoy <ego@xxxxxxxxxxxxxxxxxx> [2020-09-03 14:57:27]:

> From: "Gautham R. Shenoy" <ego@xxxxxxxxxxxxxxxxxx>
> 
> commit d947fb4c965c ("cpuidle: pseries: Fixup exit latency for
> CEDE(0)") sets the exit latency of CEDE(0) based on the latency values
> of the Extended CEDE states advertised by the platform. The values
> advertised by the platform are in timebase ticks. However the cpuidle
> framework requires the latency values in microseconds.
> 
> If the tb-ticks value advertised by the platform correspond to a value
> smaller than 1us, during the conversion from tb-ticks to microseconds,
> in the current code, the result becomes zero. This is incorrect as it
> puts a CEDE state on par with the snooze state.
> 
> This patch fixes this by rounding up the result obtained while
> converting the latency value from tb-ticks to microseconds. It also
> prints a warning in case we discover an extended-cede state with
> wakeup latency to be 0. In such a case, ensure that CEDE(0) has a
> non-zero wakeup latency.
> 
> Fixes: commit d947fb4c965c ("cpuidle: pseries: Fixup exit latency for
> CEDE(0)")
> 
> Signed-off-by: Gautham R. Shenoy <ego@xxxxxxxxxxxxxxxxxx>

Reviewed-by: Vaidyanathan Srinivasan <svaidy@xxxxxxxxxxxxx>

> ---
> v1-->v2: Added a warning if a CEDE state has 0 wakeup latency (Suggested by Joel Stanley)
>          Also added code to ensure that CEDE(0) has a non-zero wakeup latency.	 
>  drivers/cpuidle/cpuidle-pseries.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
> index ff6d99e..a2b5c6f 100644
> --- a/drivers/cpuidle/cpuidle-pseries.c
> +++ b/drivers/cpuidle/cpuidle-pseries.c
> @@ -361,7 +361,10 @@ static void __init fixup_cede0_latency(void)
>  	for (i = 0; i < nr_xcede_records; i++) {
>  		struct xcede_latency_record *record = &payload->records[i];
>  		u64 latency_tb = be64_to_cpu(record->latency_ticks);
> -		u64 latency_us = tb_to_ns(latency_tb) / NSEC_PER_USEC;
> +		u64 latency_us = DIV_ROUND_UP_ULL(tb_to_ns(latency_tb), NSEC_PER_USEC);

This would fix the issue of rounding down to zero.

> +
> +		if (latency_us == 0)
> +			pr_warn("cpuidle: xcede record %d has an unrealistic latency of 0us.\n", i);

+1  This should not happen.

>  		if (latency_us < min_latency_us)
>  			min_latency_us = latency_us;
> @@ -378,10 +381,14 @@ static void __init fixup_cede0_latency(void)
>  	 * Perform the fix-up.
>  	 */
>  	if (min_latency_us < dedicated_states[1].exit_latency) {
> -		u64 cede0_latency = min_latency_us - 1;
> +		/*
> +		 * We set a minimum of 1us wakeup latency for cede0 to
> +		 * distinguish it from snooze
> +		 */
> +		u64 cede0_latency = 1;
> 
> -		if (cede0_latency <= 0)
> -			cede0_latency = min_latency_us;
> +		if (min_latency_us > cede0_latency)
> +			cede0_latency = min_latency_us - 1;

Good checks to expect cede latency of 1us or more.

>  		dedicated_states[1].exit_latency = cede0_latency;
>  		dedicated_states[1].target_residency = 10 * (cede0_latency);

--Vaidy