[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200903155100.GA3831@drishya.in.ibm.com>
Date: Thu, 3 Sep 2020 21:21:26 +0530
From: Vaidyanathan Srinivasan <svaidy@...ux.ibm.com>
To: "Gautham R. Shenoy" <ego@...ux.vnet.ibm.com>
Cc: Michael Ellerman <mpe@...erman.id.au>,
"Rafael J. Wysocki" <rjw@...ysocki.net>,
Joel Stanley <joel@....id.au>, linux-pm@...r.kernel.org,
linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] cpuidle-pseries: Fix CEDE latency conversion from tb
to us
* Gautham R Shenoy <ego@...ux.vnet.ibm.com> [2020-09-03 14:57:27]:
> From: "Gautham R. Shenoy" <ego@...ux.vnet.ibm.com>
>
> 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@...ux.vnet.ibm.com>
Reviewed-by: Vaidyanathan Srinivasan <svaidy@...ux.ibm.com>
> ---
> 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
Powered by blists - more mailing lists