From: "Gautham R. Shenoy" <[email protected]>
This is a v3 of the patch series to parse the extended CEDE
information in the pseries-cpuidle driver.
The previous two versions of the patches can be found here:
v2: https://lore.kernel.org/lkml/[email protected]/
v1: https://lore.kernel.org/linuxppc-dev/[email protected]/
The change from v2 --> v1 :
* Patch 1: Got rid of some #define-s which were needed mainly for Patches4 and
5 of v1, but were retained in v2.
* Patch 2:
* Based on feedback from Michael Ellerman, rewrote the
function to parse the extended idle states by explicitly
defining the structure of the object that is returned by
ibm,get-system-parameters(CEDE_LATENCY_TOKEN) rtas-call. In
the previous versions we were passing a character array and
subsequently parsing the individual elements which can be
bug-prone. This also gets rid of the excessive (cast *)ing
that was in the previous versions.
* Marked some of the functions static and annotated some of
the functions with __init and data with __initdata. This
makes Sparse happy.
* Added comments for CEDE_LATENCY_TOKEN.
* Renamed add_pseries_idle_states() to
parse_xcede_idle_states(). Again, this is because Patch 4
and 5 from v1 are no longer there.
* Patch 3: No functional changes, but minor changes to be consistent
with Patch 1 and 2 of this series.
I have additionally tested the code on POWER8 dedicated LPAR and found
that it has no impact, since the wakeup latency of CEDE(1) is 30us
which is greater that default latency that we are assuming for
CEDE(0). So we do not need to fixup CEDE(0) latency on POWER8.
Vaidy, I have removed your Reviewed-by for v1, since the code has
changed a little bit.
Gautham R. Shenoy (3):
cpuidle-pseries: Set the latency-hint before entering CEDE
cpuidle-pseries: Add function to parse extended CEDE records
cpuidle-pseries : Fixup exit latency for CEDE(0)
drivers/cpuidle/cpuidle-pseries.c | 190 +++++++++++++++++++++++++++++++++++++-
1 file changed, 188 insertions(+), 2 deletions(-)
--
1.9.4
From: "Gautham R. Shenoy" <[email protected]>
Currently we use CEDE with latency-hint 0 as the only other idle state
on a dedicated LPAR apart from the polling "snooze" state.
The platform might support additional extended CEDE idle states, which
can be discovered through the "ibm,get-system-parameter" rtas-call
made with CEDE_LATENCY_TOKEN.
This patch adds a function to obtain information about the extended
CEDE idle states from the platform and parse the contents to populate
an array of extended CEDE states. These idle states thus discovered
will be added to the cpuidle framework in the next patch.
dmesg on a POWER8 and POWER9 LPAR, demonstrating the output of parsing
the extended CEDE latency parameters are as follows
POWER8
[ 10.093279] xcede : xcede_record_size = 10
[ 10.093285] xcede : Record 0 : hint = 1, latency = 0x3c00 tb ticks, Wake-on-irq = 1
[ 10.093291] xcede : Record 1 : hint = 2, latency = 0x4e2000 tb ticks, Wake-on-irq = 0
[ 10.093297] cpuidle : Skipping the 2 Extended CEDE idle states
POWER9
[ 5.913180] xcede : xcede_record_size = 10
[ 5.913183] xcede : Record 0 : hint = 1, latency = 0x400 tb ticks, Wake-on-irq = 1
[ 5.913188] xcede : Record 1 : hint = 2, latency = 0x3e8000 tb ticks, Wake-on-irq = 0
[ 5.913193] cpuidle : Skipping the 2 Extended CEDE idle states
Signed-off-by: Gautham R. Shenoy <[email protected]>
---
v2-->v3 : Cleaned up parse_cede_parameters(). Silenced some sparse warnings.
drivers/cpuidle/cpuidle-pseries.c | 142 ++++++++++++++++++++++++++++++++++++++
1 file changed, 142 insertions(+)
diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
index f5865a2..f528da7 100644
--- a/drivers/cpuidle/cpuidle-pseries.c
+++ b/drivers/cpuidle/cpuidle-pseries.c
@@ -21,6 +21,7 @@
#include <asm/runlatch.h>
#include <asm/idle.h>
#include <asm/plpar_wrappers.h>
+#include <asm/rtas.h>
static struct cpuidle_driver pseries_idle_driver = {
.name = "pseries_idle",
@@ -87,6 +88,137 @@ static void check_and_cede_processor(void)
}
#define NR_DEDICATED_STATES 2 /* snooze, CEDE */
+/*
+ * XCEDE : Extended CEDE states discovered through the
+ * "ibm,get-systems-parameter" rtas-call with the token
+ * CEDE_LATENCY_TOKEN
+ */
+#define MAX_XCEDE_STATES 4
+#define XCEDE_LATENCY_RECORD_SIZE 10
+#define XCEDE_LATENCY_PARAM_MAX_LENGTH (2 + 2 + \
+ (MAX_XCEDE_STATES * XCEDE_LATENCY_RECORD_SIZE))
+
+/*
+ * Section 7.3.16 System Parameters Option of PAPR version 2.8.1 has a
+ * table with all the parameters to ibm,get-system-parameters.
+ * CEDE_LATENCY_TOKEN corresponds to the token value for Cede Latency
+ * Settings Information.
+ */
+#define CEDE_LATENCY_TOKEN 45
+
+/*
+ * If the platform supports the cede latency settings
+ * information system parameter it must provide the following
+ * information in the NULL terminated parameter string:
+ *
+ * a. The first byte is the length “N” of each cede
+ * latency setting record minus one (zero indicates a length
+ * of 1 byte).
+ *
+ * b. For each supported cede latency setting a cede latency
+ * setting record consisting of the first “N” bytes as per
+ * the following table.
+ *
+ * -----------------------------
+ * | Field | Field |
+ * | Name | Length |
+ * -----------------------------
+ * | Cede Latency | 1 Byte |
+ * | Specifier Value | |
+ * -----------------------------
+ * | Maximum wakeup | |
+ * | latency in | 8 Bytes|
+ * | tb-ticks | |
+ * -----------------------------
+ * | Responsive to | |
+ * | external | 1 Byte |
+ * | interrupts | |
+ * -----------------------------
+ *
+ * This version has cede latency record size = 10.
+ *
+ * The structure xcede_latency_payload represents a) and b) with
+ * xcede_latency_record representing the table in b).
+ *
+ * xcede_latency_parameter is what gets returned by
+ * ibm,get-systems-parameter rtas-call when made with
+ * CEDE_LATENCY_TOKEN.
+ *
+ * These structures are only used to represent the data sent obtained
+ * by the rtas-call. The data is in Big-Endian.
+ */
+struct xcede_latency_record {
+ u8 hint;
+ __be64 latency_ticks;
+ u8 wake_on_irqs;
+} __packed;
+
+struct xcede_latency_payload {
+ u8 record_size;
+ struct xcede_latency_record records[MAX_XCEDE_STATES];
+} __packed;
+
+struct xcede_latency_parameter {
+ __be16 payload_size;
+ struct xcede_latency_payload payload;
+ u8 null_char;
+} __packed;
+
+static unsigned int nr_xcede_records;
+static struct xcede_latency_parameter xcede_latency_parameter __initdata;
+
+static int __init parse_cede_parameters(void)
+{
+ int ret, i;
+ u16 payload_size;
+ u8 xcede_record_size;
+ u32 total_xcede_records_size;
+ struct xcede_latency_payload *payload;
+
+ memset(&xcede_latency_parameter, 0, sizeof(xcede_latency_parameter));
+
+ ret = rtas_call(rtas_token("ibm,get-system-parameter"), 3, 1,
+ NULL, CEDE_LATENCY_TOKEN, __pa(&xcede_latency_parameter),
+ sizeof(xcede_latency_parameter));
+
+ if (ret) {
+ pr_err("xcede: Error parsing CEDE_LATENCY_TOKEN\n");
+ return ret;
+ }
+
+ payload_size = be16_to_cpu(xcede_latency_parameter.payload_size);
+ payload = &xcede_latency_parameter.payload;
+
+ xcede_record_size = payload->record_size + 1;
+
+ if (xcede_record_size != XCEDE_LATENCY_RECORD_SIZE) {
+ pr_err("xcede : Expected record-size %d. Observed size %d.\n",
+ XCEDE_LATENCY_RECORD_SIZE, xcede_record_size);
+ return -EINVAL;
+ }
+
+ pr_info("xcede : xcede_record_size = %d\n", xcede_record_size);
+
+ /*
+ * Since the payload_length includes the last NULL byte and
+ * the xcede_record_size, the remaining bytes correspond to
+ * array of all cede_latency settings.
+ */
+ total_xcede_records_size = payload_size - 2;
+ nr_xcede_records = total_xcede_records_size / xcede_record_size;
+
+ for (i = 0; i < nr_xcede_records; i++) {
+ struct xcede_latency_record *record = &payload->records[i];
+ u8 hint = record->hint;
+ u8 wake_on_irqs = record->wake_on_irqs;
+ u64 latency_ticks = be64_to_cpu(record->latency_ticks);
+
+ pr_info("xcede : Record %d : hint = %u, latency = 0x%llx tb ticks, Wake-on-irq = %u\n",
+ i, hint, latency_ticks, wake_on_irqs);
+ }
+
+ return 0;
+}
u8 cede_latency_hint[NR_DEDICATED_STATES];
static int dedicated_cede_loop(struct cpuidle_device *dev,
@@ -218,6 +350,15 @@ static int pseries_cpuidle_driver_init(void)
return 0;
}
+static void __init parse_xcede_idle_states(void)
+{
+ if (parse_cede_parameters())
+ return;
+
+ pr_info("cpuidle : Skipping the %d Extended CEDE idle states\n",
+ nr_xcede_records);
+}
+
/*
* pseries_idle_probe()
* Choose state table for shared versus dedicated partition
@@ -239,6 +380,7 @@ static int pseries_idle_probe(void)
cpuidle_state_table = shared_states;
max_idle_state = ARRAY_SIZE(shared_states);
} else {
+ parse_xcede_idle_states();
cpuidle_state_table = dedicated_states;
max_idle_state = NR_DEDICATED_STATES;
}
--
1.9.4
From: "Gautham R. Shenoy" <[email protected]>
We are currently assuming that CEDE(0) has exit latency 10us, since
there is no way for us to query from the platform. However, if the
wakeup latency of an Extended CEDE state is smaller than 10us, then we
can be sure that the exit latency of CEDE(0) cannot be more than that.
that.
In this patch, we fix the exit latency of CEDE(0) if we discover an
Extended CEDE state with wakeup latency smaller than 10us.
Benchmark results:
On POWER8, this patch does not have any impact since the advertized
latency of Extended CEDE (1) is 30us which is higher than the default
latency of CEDE (0) which is 10us.
On POWER9 we see improvement the single-threaded performance of ebizzy,
and no regression in the wakeup latency or the number of
context-switches.
ebizzy:
2 ebizzy threads bound to the same big-core. 25% improvement in the
avg records/s with patch.
x without_patch
* with_patch
N Min Max Median Avg Stddev
x 10 2491089 5834307 5398375 4244335 1596244.9
* 10 2893813 5834474 5832448 5327281.3 1055941.4
context_switch2 :
There is no major regression observed with this patch as seen from the
context_switch2 benchmark.
context_switch2 across CPU0 CPU1 (Both belong to same big-core, but different
small cores). We observe a minor 0.14% regression in the number of
context-switches (higher is better).
x without_patch
* with_patch
N Min Max Median Avg Stddev
x 500 348872 362236 354712 354745.69 2711.827
* 500 349422 361452 353942 354215.4 2576.9258
Difference at 99.0% confidence
-530.288 +/- 430.963
-0.149484% +/- 0.121485%
(Student's t, pooled s = 2645.24)
context_switch2 across CPU0 CPU8 (Different big-cores). We observe a 0.37%
improvement in the number of context-switches (higher is better).
x without_patch
* with_patch
N Min Max Median Avg Stddev
x 500 287956 294940 288896 288977.23 646.59295
* 500 288300 294646 289582 290064.76 1161.9992
Difference at 99.0% confidence
1087.53 +/- 153.194
0.376337% +/- 0.0530125%
(Student's t, pooled s = 940.299)
schbench:
No major difference could be seen until the 99.9th percentile.
Without-patch
Latency percentiles (usec)
50.0th: 29
75.0th: 39
90.0th: 49
95.0th: 59
*99.0th: 13104
99.5th: 14672
99.9th: 15824
min=0, max=17993
With-patch:
Latency percentiles (usec)
50.0th: 29
75.0th: 40
90.0th: 50
95.0th: 61
*99.0th: 13648
99.5th: 14768
99.9th: 15664
min=0, max=29812
Signed-off-by: Gautham R. Shenoy <[email protected]>
---
v2-->v3 : Made notation consistent with first two patches.
drivers/cpuidle/cpuidle-pseries.c | 41 +++++++++++++++++++++++++++++++++++++--
1 file changed, 39 insertions(+), 2 deletions(-)
diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
index f528da7..8d19820 100644
--- a/drivers/cpuidle/cpuidle-pseries.c
+++ b/drivers/cpuidle/cpuidle-pseries.c
@@ -350,13 +350,50 @@ static int pseries_cpuidle_driver_init(void)
return 0;
}
-static void __init parse_xcede_idle_states(void)
+static void __init fixup_cede0_latency(void)
{
+ int i;
+ u64 min_latency_us = dedicated_states[1].exit_latency; /* CEDE latency */
+ struct xcede_latency_payload *payload;
+
if (parse_cede_parameters())
return;
pr_info("cpuidle : Skipping the %d Extended CEDE idle states\n",
nr_xcede_records);
+
+ payload = &xcede_latency_parameter.payload;
+ 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;
+
+ if (latency_us < min_latency_us)
+ min_latency_us = latency_us;
+ }
+
+ /*
+ * By default, we assume that CEDE(0) has exit latency 10us,
+ * since there is no way for us to query from the platform.
+ *
+ * However, if the wakeup latency of an Extended CEDE state is
+ * smaller than 10us, then we can be sure that CEDE(0)
+ * requires no more than that.
+ *
+ * Perform the fix-up.
+ */
+ if (min_latency_us < dedicated_states[1].exit_latency) {
+ u64 cede0_latency = min_latency_us - 1;
+
+ if (cede0_latency <= 0)
+ cede0_latency = min_latency_us;
+
+ dedicated_states[1].exit_latency = cede0_latency;
+ dedicated_states[1].target_residency = 10 * (cede0_latency);
+ pr_info("cpuidle : Fixed up CEDE exit latency to %llu us\n",
+ cede0_latency);
+ }
+
}
/*
@@ -380,7 +417,7 @@ static int pseries_idle_probe(void)
cpuidle_state_table = shared_states;
max_idle_state = ARRAY_SIZE(shared_states);
} else {
- parse_xcede_idle_states();
+ fixup_cede0_latency();
cpuidle_state_table = dedicated_states;
max_idle_state = NR_DEDICATED_STATES;
}
--
1.9.4
On Thu, 30 Jul 2020 11:02:54 +0530, Gautham R. Shenoy wrote:
> This is a v3 of the patch series to parse the extended CEDE
> information in the pseries-cpuidle driver.
>
> The previous two versions of the patches can be found here:
>
> v2: https://lore.kernel.org/lkml/[email protected]/
>
> [...]
Applied to powerpc/next.
[1/3] cpuidle: pseries: Set the latency-hint before entering CEDE
https://git.kernel.org/powerpc/c/3af0ada7dd98c6da35c1fd7f107af3b9aa5e904c
[2/3] cpuidle: pseries: Add function to parse extended CEDE records
https://git.kernel.org/powerpc/c/054e44ba99ae36918631fcbf5f034e466c2f1b73
[3/3] cpuidle: pseries: Fixup exit latency for CEDE(0)
https://git.kernel.org/powerpc/c/d947fb4c965cdb7242f3f91124ea16079c49fa8b
cheers