| Message ID | 1599125247-28488-1-git-send-email-ego@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1d3ee7df009a46440c58508b8819213c09503acd |
| Headers | show |
| Series | [v2] cpuidle-pseries: Fix CEDE latency conversion from tb to us | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (35f066fda170dde0a31f1447547a5d30b83c3920) |
| snowpatch_ozlabs/build-ppc64le | warning | Upstream build failed, couldn't test patch |
| snowpatch_ozlabs/build-ppc64be | warning | Upstream build failed, couldn't test patch |
| snowpatch_ozlabs/build-ppc64e | warning | Upstream build failed, couldn't test patch |
| snowpatch_ozlabs/build-pmac32 | warning | Upstream build failed, couldn't test patch |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 1 warnings, 0 checks, 28 lines checked |
| snowpatch_ozlabs/needsstable | fail |
* Gautham R Shenoy <ego@linux.vnet.ibm.com> [2020-09-03 14:57:27]: > From: "Gautham R. Shenoy" <ego@linux.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@linux.vnet.ibm.com> Reviewed-by: Vaidyanathan Srinivasan <svaidy@linux.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
On Thu, 3 Sep 2020 14:57:27 +0530, Gautham R. Shenoy wrote: > 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. > > [...] Applied to powerpc/fixes. [1/1] cpuidle: pseries: Fix CEDE latency conversion from tb to us https://git.kernel.org/powerpc/c/1d3ee7df009a46440c58508b8819213c09503acd cheers
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); + + if (latency_us == 0) + pr_warn("cpuidle: xcede record %d has an unrealistic latency of 0us.\n", i); 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; dedicated_states[1].exit_latency = cede0_latency; dedicated_states[1].target_residency = 10 * (cede0_latency);