[PATCH 0/5] cpuidle-pseries: Parse extended CEDE information for idle.

[PATCH 0/5] cpuidle-pseries: Parse extended CEDE information for idle.

[Gautham R. Shenoy]

From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>

Hi,

On pseries Dedicated Linux LPARs, apart from the polling snooze idle
state, we currently have the CEDE idle state which cedes the CPU to
the hypervisor with latency-hint = 0.

However, the PowerVM hypervisor supports additional extended CEDE
states, which can be queried through the "ibm,get-systems-parameter"
rtas-call with the CEDE_LATENCY_TOKEN. The hypervisor maps these
extended CEDE states to appropriate platform idle-states in order to
provide energy-savings as well as shifting power to the active
units. On existing pseries LPARs today we have extended CEDE with
latency-hints {1,2} supported.

In Patches 1-3 of this patchset, we add the code to parse the CEDE
latency records provided by the hypervisor. We use this information to
determine the wakeup latency of the regular CEDE (which we have been
so far hardcoding to 10us while experimentally it is much lesser ~
1us), by looking at the wakeup latency provided by the hypervisor for
Extended CEDE states. Since the platform currently advertises Extended
CEDE 1 to have wakeup latency of 2us, we can be sure that the wakeup
latency of the regular CEDE is no more than this.

Patch 4 (currently marked as RFC), expose the extended CEDE states
parsed above to the cpuidle framework, provided that they can wakeup
on an interrupt. On current platforms only Extended CEDE 1 fits the
bill, but this is going to change in future platforms where even
Extended CEDE 2 may be responsive to external interrupts.

Patch 5 (currently marked as RFC), filters out Extended CEDE 1 since
it offers no added advantage over the normal CEDE.

With Patches 1-3, we see an improvement in the single-threaded
performance on ebizzy.

2 ebizzy threads bound to the same big-core. 25% improvement in the
avg records/s (higher the better) with patches 1-3.
x without_patches
* with_patches
    N           Min           Max        Median           Avg        Stddev
x  10       2491089       5834307       5398375       4244335     1596244.9
*  10       2893813       5834474       5832448     5327281.3     1055941.4

We do not observe any major regression in either the context_switch2
benchmark or the schbench 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

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

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



Gautham R. Shenoy (5):
  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)
  cpuidle-pseries : Include extended CEDE states in cpuidle framework
  cpuidle-pseries: Block Extended CEDE(1) which adds no additional
    value.

 drivers/cpuidle/cpuidle-pseries.c | 268 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 266 insertions(+), 2 deletions(-)

-- 
1.9.4

[PATCH 1/5] cpuidle-pseries: Set the latency-hint before entering CEDE

[Gautham R. Shenoy]

From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>

As per the PAPR, each H_CEDE call is associated with a latency-hint to
be passed in the VPA field "cede_latency_hint". The CEDE states that
we were implicitly entering so far is CEDE with latency-hint = 0.

This patch explicitly sets the latency hint corresponding to the CEDE
state that we are currently entering. While at it, we save the
previous hint, to be restored once we wakeup from CEDE. This will be
required in the future when we expose extended-cede states through the
cpuidle framework, where each of them will have a different
cede-latency hint.

Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
 drivers/cpuidle/cpuidle-pseries.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
index 4a37252..39d4bb6 100644
--- a/drivers/cpuidle/cpuidle-pseries.c
+++ b/drivers/cpuidle/cpuidle-pseries.c
@@ -105,19 +105,27 @@ static void check_and_cede_processor(void)
 	}
 }
 
+#define NR_CEDE_STATES		1  /* CEDE with latency-hint 0 */
+#define NR_DEDICATED_STATES	(NR_CEDE_STATES + 1) /* Includes snooze */
+
+u8 cede_latency_hint[NR_DEDICATED_STATES];
 static int dedicated_cede_loop(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv,
 				int index)
 {
+	u8 old_latency_hint;
 
 	pseries_idle_prolog();
 	get_lppaca()->donate_dedicated_cpu = 1;
+	old_latency_hint = get_lppaca()->cede_latency_hint;
+	get_lppaca()->cede_latency_hint = cede_latency_hint[index];
 
 	HMT_medium();
 	check_and_cede_processor();
 
 	local_irq_disable();
 	get_lppaca()->donate_dedicated_cpu = 0;
+	get_lppaca()->cede_latency_hint = old_latency_hint;
 
 	pseries_idle_epilog();
 
@@ -149,7 +157,7 @@ static int shared_cede_loop(struct cpuidle_device *dev,
 /*
  * States for dedicated partition case.
  */
-static struct cpuidle_state dedicated_states[] = {
+static struct cpuidle_state dedicated_states[NR_DEDICATED_STATES] = {
 	{ /* Snooze */
 		.name = "snooze",
 		.desc = "snooze",
-- 
1.9.4

[PATCH 2/5] cpuidle-pseries: Add function to parse extended CEDE records

[Gautham R. Shenoy]

From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>

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 POWER9 LPAR, demonstrating the output of parsing the
extended CEDE latency parameters.

[    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 <ego@linux.vnet.ibm.com>
---
 drivers/cpuidle/cpuidle-pseries.c | 129 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 127 insertions(+), 2 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
index 39d4bb6..c13549b 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>
 
 struct cpuidle_driver pseries_idle_driver = {
 	.name             = "pseries_idle",
@@ -105,9 +106,120 @@ static void check_and_cede_processor(void)
 	}
 }
 
-#define NR_CEDE_STATES		1  /* CEDE with latency-hint 0 */
+struct xcede_latency_records {
+	u8  latency_hint;
+	u64 wakeup_latency_tb_ticks;
+	u8  responsive_to_irqs;
+};
+
+/*
+ * 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))
+
+#define CEDE_LATENCY_TOKEN		45
+
+#define NR_CEDE_STATES		(MAX_XCEDE_STATES + 1) /* CEDE with latency-hint 0 */
 #define NR_DEDICATED_STATES	(NR_CEDE_STATES + 1) /* Includes snooze */
 
+struct xcede_latency_records xcede_records[MAX_XCEDE_STATES];
+unsigned int nr_xcede_records;
+char xcede_parameters[XCEDE_LATENCY_PARAM_MAX_LENGTH];
+
+static int parse_cede_parameters(void)
+{
+	int ret = -1, i;
+	u16 payload_length;
+	u8 xcede_record_size;
+	u32 total_xcede_records_size;
+	char *payload;
+
+	memset(xcede_parameters, 0, XCEDE_LATENCY_PARAM_MAX_LENGTH);
+
+	ret = rtas_call(rtas_token("ibm,get-system-parameter"), 3, 1,
+			NULL, CEDE_LATENCY_TOKEN, __pa(xcede_parameters),
+			XCEDE_LATENCY_PARAM_MAX_LENGTH);
+
+	if (ret) {
+		pr_err("xcede: Error parsing CEDE_LATENCY_TOKEN\n");
+		return ret;
+	}
+
+	payload_length = be16_to_cpu(*(__be16 *)(&xcede_parameters[0]));
+	payload = &xcede_parameters[2];
+
+	/*
+	 * 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.
+	 */
+	xcede_record_size = (u8)payload[0] + 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_length - 2;
+	nr_xcede_records = total_xcede_records_size / xcede_record_size;
+
+	payload++;
+	for (i = 0; i < nr_xcede_records; i++) {
+		struct xcede_latency_records *record = &xcede_records[i];
+
+		record->latency_hint = (u8)payload[0];
+		record->wakeup_latency_tb_ticks  =
+			be64_to_cpu(*(__be64 *)(&payload[1]));
+		record->responsive_to_irqs = (u8)payload[9];
+		payload += xcede_record_size;
+		pr_info("xcede : Record %d : hint = %u, latency =0x%llx tb-ticks, Wake-on-irq = %u\n",
+			i, record->latency_hint,
+			record->wakeup_latency_tb_ticks,
+			record->responsive_to_irqs);
+	}
+
+	return 0;
+}
+
 u8 cede_latency_hint[NR_DEDICATED_STATES];
 static int dedicated_cede_loop(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv,
@@ -238,6 +350,19 @@ static int pseries_cpuidle_driver_init(void)
 	return 0;
 }
 
+static int add_pseries_idle_states(void)
+{
+	int nr_states = 2; /* By default we have snooze, CEDE */
+
+	if (parse_cede_parameters())
+		return nr_states;
+
+	pr_info("cpuidle : Skipping the %d Extended CEDE idle states\n",
+		nr_xcede_records);
+
+	return nr_states;
+}
+
 /*
  * pseries_idle_probe()
  * Choose state table for shared versus dedicated partition
@@ -260,7 +385,7 @@ static int pseries_idle_probe(void)
 			max_idle_state = ARRAY_SIZE(shared_states);
 		} else {
 			cpuidle_state_table = dedicated_states;
-			max_idle_state = ARRAY_SIZE(dedicated_states);
+			max_idle_state = add_pseries_idle_states();
 		}
 	} else
 		return -ENODEV;
-- 
1.9.4

[PATCH 3/5] cpuidle-pseries : Fixup exit latency for CEDE(0)

[Gautham R. Shenoy]

From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>

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. The new
value is 1us lesser than the smallest wakeup latency among the
Extended CEDE states.

Benchmark results:

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

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

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 <ego@linux.vnet.ibm.com>
---
 drivers/cpuidle/cpuidle-pseries.c | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
index c13549b..502f906 100644
--- a/drivers/cpuidle/cpuidle-pseries.c
+++ b/drivers/cpuidle/cpuidle-pseries.c
@@ -353,12 +353,42 @@ static int pseries_cpuidle_driver_init(void)
 static int add_pseries_idle_states(void)
 {
 	int nr_states = 2; /* By default we have snooze, CEDE */
+	int i;
+	u64 min_latency_us = dedicated_states[1].exit_latency; /* CEDE latency */
 
 	if (parse_cede_parameters())
 		return nr_states;
 
-	pr_info("cpuidle : Skipping the %d Extended CEDE idle states\n",
-		nr_xcede_records);
+	for (i = 0; i < nr_xcede_records; i++) {
+		u64 latency_tb = xcede_records[i].wakeup_latency_tb_ticks;
+		u64 latency_us = tb_to_ns(latency_tb) / NSEC_PER_USEC;
+
+		if (latency_us < min_latency_us)
+			min_latency_us = latency_us;
+	}
+
+	/*
+	 * 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 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);
+	}
 
 	return nr_states;
 }
-- 
1.9.4

[PATCH 4/5] cpuidle-pseries : Include extended CEDE states in cpuidle framework

[Gautham R. Shenoy]

From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>

This patch exposes those extended CEDE states to the cpuidle framework
which are responsive to external interrupts and do not need an H_PROD.

Since as per the PAPR, all the extended CEDE states are non-responsive
to timers, we indicate this to the cpuidle subsystem via the
CPUIDLE_FLAG_TIMER_STOP flag for all those extende CEDE states which
can wake up on external interrupts.

With the patch, we are able to see the extended CEDE state with
latency hint = 1 exposed via the cpuidle framework.

	$ cpupower idle-info
	CPUidle driver: pseries_idle
	CPUidle governor: menu
	analyzing CPU 0:

	Number of idle states: 3
	Available idle states: snooze CEDE XCEDE1
	snooze:
	Flags/Description: snooze
	Latency: 0
	Usage: 33429446
	Duration: 27006062
	CEDE:
	Flags/Description: CEDE
	Latency: 1
	Usage: 10272
	Duration: 110786770
	XCEDE1:
	Flags/Description: XCEDE1
	Latency: 12
	Usage: 26445
	Duration: 1436433815

Benchmark results:
TLDR: Over all we do not see any additional benefit from having XCEDE1 over
CEDE.

ebizzy :
2 threads bound to a big-core. With this patch, we see a 3.39%
regression compared to with only CEDE0 latency fixup.
x With only CEDE0 latency fixup
* With CEDE0 latency fixup + CEDE1
    N           Min           Max        Median           Avg        Stddev
x  10       2893813       5834474       5832448     5327281.3     1055941.4
*  10       2907329       5834923       5831398     5146614.6     1193874.8

context_switch2:
With the context_switch2 there are no observable regressions in the
results.

context_switch2 CPU0 CPU1 (Same Big-core, different small-cores).
No difference with and without patch.
x without_patch
* with_patch
    N           Min           Max        Median           Avg        Stddev
x 500        343644        348778        345444     345584.02     1035.1658
* 500        344310        347646        345776     345877.22     802.19501

context_switch2 CPU0 CPU8 (different big-cores). Minor 0.05% improvement
with patch
x without_patch
* with_patch
    N           Min           Max        Median           Avg        Stddev
x 500        287562        288756        288162     288134.76     262.24328
* 500        287874        288960        288306     288274.66     187.57034

schbench:
No regressions observed with schbench

Without 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

With Patch:
Latency percentiles (usec)
	50.0th: 30
	75.0th: 40
	90.0th: 51
	95.0th: 59
	*99.0th: 13616
	99.5th: 14512
	99.9th: 15696
	min=0, max=15996

Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
 drivers/cpuidle/cpuidle-pseries.c | 50 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
index 502f906..6f893cd 100644
--- a/drivers/cpuidle/cpuidle-pseries.c
+++ b/drivers/cpuidle/cpuidle-pseries.c
@@ -362,9 +362,59 @@ static int add_pseries_idle_states(void)
 	for (i = 0; i < nr_xcede_records; i++) {
 		u64 latency_tb = xcede_records[i].wakeup_latency_tb_ticks;
 		u64 latency_us = tb_to_ns(latency_tb) / NSEC_PER_USEC;
+		char name[CPUIDLE_NAME_LEN];
+		unsigned int latency_hint = xcede_records[i].latency_hint;
+		u64 residency_us;
+
+		if (!xcede_records[i].responsive_to_irqs) {
+			pr_info("cpuidle : Skipping XCEDE%d. Not responsive to IRQs\n",
+				latency_hint);
+			continue;
+		}
 
 		if (latency_us < min_latency_us)
 			min_latency_us = latency_us;
+		snprintf(name, CPUIDLE_NAME_LEN, "XCEDE%d", latency_hint);
+
+		/*
+		 * As per the section 14.14.1 of PAPR version 2.8.1
+		 * says that alling H_CEDE with the value of the cede
+		 * latency specifier set greater than zero allows the
+		 * processor timer facility to be disabled (so as not
+		 * to cause gratuitous wake-ups - the use of H_PROD,
+		 * or other external interrupt is required to wake the
+		 * processor in this case).
+		 *
+		 * So, inform the cpuidle-subsystem that the timer
+		 * will be stopped for these states.
+		 *
+		 * Also, bump up the latency by 10us, since cpuidle
+		 * would use timer-offload framework which will need
+		 * to send an IPI to wakeup a CPU whose timer has
+		 * expired.
+		 */
+		if (latency_hint > 0) {
+			dedicated_states[nr_states].flags = CPUIDLE_FLAG_TIMER_STOP;
+			latency_us += 10;
+		}
+
+		/*
+		 * Thumb rule : Reside in the XCEDE state for at least
+		 * 10x the time required to enter and exit that state.
+		 */
+		residency_us = latency_us * 10;
+
+		strlcpy(dedicated_states[nr_states].name, (const char *)name,
+			CPUIDLE_NAME_LEN);
+		strlcpy(dedicated_states[nr_states].desc, (const char *)name,
+			CPUIDLE_NAME_LEN);
+		dedicated_states[nr_states].exit_latency = latency_us;
+		dedicated_states[nr_states].target_residency = residency_us;
+		dedicated_states[nr_states].enter = &dedicated_cede_loop;
+		cede_latency_hint[nr_states] = latency_hint;
+		pr_info("cpuidle : Added %s. latency-hint = %d\n",
+			name, latency_hint);
+		nr_states++;
 	}
 
 	/*
-- 
1.9.4

[PATCH 5/5] cpuidle-pseries: Block Extended CEDE(1) which adds no additional value.

[Gautham R. Shenoy]

From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>

The Extended CEDE state with latency-hint = 1 is only different from
normal CEDE (with latency-hint = 0) in that a CPU in Extended CEDE(1)
does not wakeup on timer events. Both CEDE and Extended CEDE(1) map to
the same hardware idle state. Since we already get SMT folding from
the normal CEDE, the Extended CEDE(1) doesn't provide any additional
value. This patch blocks Extended CEDE(1).

Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
 drivers/cpuidle/cpuidle-pseries.c | 57 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 54 insertions(+), 3 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
index 6f893cd..be0b8b2 100644
--- a/drivers/cpuidle/cpuidle-pseries.c
+++ b/drivers/cpuidle/cpuidle-pseries.c
@@ -350,6 +350,43 @@ static int pseries_cpuidle_driver_init(void)
 	return 0;
 }
 
+#define XCEDE1_HINT	1
+#define ERR_NO_VALUE_ADD	(-1)
+#define ERR_NO_EE_WAKEUP	(-2)
+
+/*
+ * Returns 0 if the Extende CEDE state with @hint is not blocked in
+ * cpuidle framework.
+ *
+ * Returns ERR_NO_EE_WAKEUP if the Extended CEDE state is blocked due
+ * to not being responsive to external interrupts.
+ *
+ * Returns ERR_NO_VALUE_ADD if the Extended CEDE state does not provide
+ * added value addition over the normal CEDE.
+ */
+static int cpuidle_xcede_blocked(u8 hint, u64 latency_us, u8 responsive_to_irqs)
+{
+
+	/*
+	 * We will only allow extended CEDE states that are responsive
+	 * to irqs do not require an H_PROD to be woken up.
+	 */
+	if (!responsive_to_irqs)
+		return ERR_NO_EE_WAKEUP;
+
+	/*
+	 * We already obtain SMT folding benefits from CEDE (which is
+	 * CEDE with hint 0). Furthermore, CEDE is also responsive to
+	 * timer-events, while XCEDE1 requires an external
+	 * interrupt/H_PROD to be woken up. Hence, block XCEDE1 since
+	 * it adds no further value.
+	 */
+	if (hint == XCEDE1_HINT)
+		return ERR_NO_VALUE_ADD;
+
+	return 0;
+}
+
 static int add_pseries_idle_states(void)
 {
 	int nr_states = 2; /* By default we have snooze, CEDE */
@@ -365,15 +402,29 @@ static int add_pseries_idle_states(void)
 		char name[CPUIDLE_NAME_LEN];
 		unsigned int latency_hint = xcede_records[i].latency_hint;
 		u64 residency_us;
+		int rc;
+
+		if (latency_us < min_latency_us)
+			min_latency_us = latency_us;
+
+		rc = cpuidle_xcede_blocked(latency_hint, latency_us,
+					   xcede_records[i].responsive_to_irqs);
 
-		if (!xcede_records[i].responsive_to_irqs) {
+		if (rc) {
+			switch (rc) {
+			case ERR_NO_VALUE_ADD:
+				pr_info("cpuidle : Skipping XCEDE%d. No additional value-add\n",
+					latency_hint);
+				break;
+			case ERR_NO_EE_WAKEUP:
 			pr_info("cpuidle : Skipping XCEDE%d. Not responsive to IRQs\n",
 				latency_hint);
+			break;
+			}
+
 			continue;
 		}
 
-		if (latency_us < min_latency_us)
-			min_latency_us = latency_us;
 		snprintf(name, CPUIDLE_NAME_LEN, "XCEDE%d", latency_hint);
 
 		/*
-- 
1.9.4

Re: [PATCH 0/5] cpuidle-pseries: Parse extended CEDE information for idle.

[Gautham R Shenoy]

Hi,

On Tue, Jul 07, 2020 at 04:41:34PM +0530, Gautham R. Shenoy wrote:
> From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
> 
> Hi,
> 
> 
> 
> 
> Gautham R. Shenoy (5):
>   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)
>   cpuidle-pseries : Include extended CEDE states in cpuidle framework
>   cpuidle-pseries: Block Extended CEDE(1) which adds no additional
>     value.

Forgot to mention that these patches are on top of Nathan's series to
remove extended CEDE offline and bogus topology update code :
https://lore.kernel.org/linuxppc-dev/20200612051238.1007764-1-nathanl@linux.ibm.com/

> 
>  drivers/cpuidle/cpuidle-pseries.c | 268 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 266 insertions(+), 2 deletions(-)
> 
> -- 
> 1.9.4
> 

Re: [PATCH 0/5] cpuidle-pseries: Parse extended CEDE information for idle.

[Vaidyanathan Srinivasan]

* Gautham R Shenoy <ego@linux.vnet.ibm.com> [2020-07-07 16:41:34]:

> From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
> 
> Hi,
> 
> On pseries Dedicated Linux LPARs, apart from the polling snooze idle
> state, we currently have the CEDE idle state which cedes the CPU to
> the hypervisor with latency-hint = 0.
> 
> However, the PowerVM hypervisor supports additional extended CEDE
> states, which can be queried through the "ibm,get-systems-parameter"
> rtas-call with the CEDE_LATENCY_TOKEN. The hypervisor maps these
> extended CEDE states to appropriate platform idle-states in order to
> provide energy-savings as well as shifting power to the active
> units. On existing pseries LPARs today we have extended CEDE with
> latency-hints {1,2} supported.
> 
> In Patches 1-3 of this patchset, we add the code to parse the CEDE
> latency records provided by the hypervisor. We use this information to
> determine the wakeup latency of the regular CEDE (which we have been
> so far hardcoding to 10us while experimentally it is much lesser ~
> 1us), by looking at the wakeup latency provided by the hypervisor for
> Extended CEDE states. Since the platform currently advertises Extended
> CEDE 1 to have wakeup latency of 2us, we can be sure that the wakeup
> latency of the regular CEDE is no more than this.
> 
> Patch 4 (currently marked as RFC), expose the extended CEDE states
> parsed above to the cpuidle framework, provided that they can wakeup
> on an interrupt. On current platforms only Extended CEDE 1 fits the
> bill, but this is going to change in future platforms where even
> Extended CEDE 2 may be responsive to external interrupts.
> 
> Patch 5 (currently marked as RFC), filters out Extended CEDE 1 since
> it offers no added advantage over the normal CEDE.
> 
> With Patches 1-3, we see an improvement in the single-threaded
> performance on ebizzy.
> 
> 2 ebizzy threads bound to the same big-core. 25% improvement in the
> avg records/s (higher the better) with patches 1-3.
> x without_patches
> * with_patches
>     N           Min           Max        Median           Avg        Stddev
> x  10       2491089       5834307       5398375       4244335     1596244.9
> *  10       2893813       5834474       5832448     5327281.3     1055941.4
> 
> We do not observe any major regression in either the context_switch2
> benchmark or the schbench 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
> 
> 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
> 
> 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

This patch series mainly cleans up the CEDE latency discovery and
prepares to add different cpuidle states in virtualised environment.
This helps in improving SMT folding speeds and also power savings and
power shifting with newer platform firmware.

The current benefit is primarily from faster SMT folding and resulting
single performance achieved by updating the platform firmware provided
heuristics in the cpuidle states.

--Vaidy

Re: [PATCH 1/5] cpuidle-pseries: Set the latency-hint before entering CEDE

[Vaidyanathan Srinivasan]

* Gautham R Shenoy <ego@linux.vnet.ibm.com> [2020-07-07 16:41:35]:

> From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
> 
> As per the PAPR, each H_CEDE call is associated with a latency-hint to
> be passed in the VPA field "cede_latency_hint". The CEDE states that
> we were implicitly entering so far is CEDE with latency-hint = 0.
> 
> This patch explicitly sets the latency hint corresponding to the CEDE
> state that we are currently entering. While at it, we save the
> previous hint, to be restored once we wakeup from CEDE. This will be
> required in the future when we expose extended-cede states through the
> cpuidle framework, where each of them will have a different
> cede-latency hint.
> 
> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>

Reviewed-by: Vaidyanathan Srinivasan <svaidy@linux.ibm.com>


> ---
>  drivers/cpuidle/cpuidle-pseries.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
> index 4a37252..39d4bb6 100644
> --- a/drivers/cpuidle/cpuidle-pseries.c
> +++ b/drivers/cpuidle/cpuidle-pseries.c
> @@ -105,19 +105,27 @@ static void check_and_cede_processor(void)
>  	}
>  }
> 
> +#define NR_CEDE_STATES		1  /* CEDE with latency-hint 0 */
> +#define NR_DEDICATED_STATES	(NR_CEDE_STATES + 1) /* Includes snooze */
> +
> +u8 cede_latency_hint[NR_DEDICATED_STATES];
>  static int dedicated_cede_loop(struct cpuidle_device *dev,
>  				struct cpuidle_driver *drv,
>  				int index)
>  {
> +	u8 old_latency_hint;
> 
>  	pseries_idle_prolog();
>  	get_lppaca()->donate_dedicated_cpu = 1;
> +	old_latency_hint = get_lppaca()->cede_latency_hint;
> +	get_lppaca()->cede_latency_hint = cede_latency_hint[index];
> 
>  	HMT_medium();
>  	check_and_cede_processor();
> 
>  	local_irq_disable();
>  	get_lppaca()->donate_dedicated_cpu = 0;
> +	get_lppaca()->cede_latency_hint = old_latency_hint;
> 
>  	pseries_idle_epilog();
> 
> @@ -149,7 +157,7 @@ static int shared_cede_loop(struct cpuidle_device *dev,
>  /*
>   * States for dedicated partition case.
>   */
> -static struct cpuidle_state dedicated_states[] = {
> +static struct cpuidle_state dedicated_states[NR_DEDICATED_STATES] = {
>  	{ /* Snooze */
>  		.name = "snooze",
>  		.desc = "snooze",


Saving and restoring the current cede hint value helps in maintaining
compatibility with other parts of the kernel.  Over long term we can
make cpuidle driver deterministically set the CEDE hint at each
invocation of H_CEDE call so that we do not have to do multiple
redundant save-restore.

This is a reasonable start to cleanup this cupidle subsystem on PAPR
guests. 

--Vaidy

Re: [PATCH 2/5] cpuidle-pseries: Add function to parse extended CEDE records

[Vaidyanathan Srinivasan]

* Gautham R Shenoy <ego@linux.vnet.ibm.com> [2020-07-07 16:41:36]:

> From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
> 
> 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 POWER9 LPAR, demonstrating the output of parsing the
> extended CEDE latency parameters.
> 
> [    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 <ego@linux.vnet.ibm.com>

Reviewed-by: Vaidyanathan Srinivasan <svaidy@linux.ibm.com>

>
> ---
>  drivers/cpuidle/cpuidle-pseries.c | 129 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 127 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
> index 39d4bb6..c13549b 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>
> 
>  struct cpuidle_driver pseries_idle_driver = {
>  	.name             = "pseries_idle",
> @@ -105,9 +106,120 @@ static void check_and_cede_processor(void)
>  	}
>  }
> 
> -#define NR_CEDE_STATES		1  /* CEDE with latency-hint 0 */
> +struct xcede_latency_records {
> +	u8  latency_hint;
> +	u64 wakeup_latency_tb_ticks;
> +	u8  responsive_to_irqs;
> +};
> +
> +/*
> + * 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))
> +
> +#define CEDE_LATENCY_TOKEN		45
> +
> +#define NR_CEDE_STATES		(MAX_XCEDE_STATES + 1) /* CEDE with latency-hint 0 */
>  #define NR_DEDICATED_STATES	(NR_CEDE_STATES + 1) /* Includes snooze */
> 
> +struct xcede_latency_records xcede_records[MAX_XCEDE_STATES];
> +unsigned int nr_xcede_records;
> +char xcede_parameters[XCEDE_LATENCY_PARAM_MAX_LENGTH];
> +
> +static int parse_cede_parameters(void)
> +{
> +	int ret = -1, i;
> +	u16 payload_length;
> +	u8 xcede_record_size;
> +	u32 total_xcede_records_size;
> +	char *payload;
> +
> +	memset(xcede_parameters, 0, XCEDE_LATENCY_PARAM_MAX_LENGTH);
> +
> +	ret = rtas_call(rtas_token("ibm,get-system-parameter"), 3, 1,
> +			NULL, CEDE_LATENCY_TOKEN, __pa(xcede_parameters),
> +			XCEDE_LATENCY_PARAM_MAX_LENGTH);
> +
> +	if (ret) {
> +		pr_err("xcede: Error parsing CEDE_LATENCY_TOKEN\n");
> +		return ret;
> +	}
> +
> +	payload_length = be16_to_cpu(*(__be16 *)(&xcede_parameters[0]));
> +	payload = &xcede_parameters[2];
> +
> +	/*
> +	 * 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.
> +	 */
> +	xcede_record_size = (u8)payload[0] + 1;

This is standard PAPR interface that has been defined long time ago.
However, new H_CEDE hints that map to new platform features will
appear in the same interface and Linux needs to prepare and be ready
to check and exploit the new hints if they are useful for the given
setup.


> +
> +	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_length - 2;
> +	nr_xcede_records = total_xcede_records_size / xcede_record_size;
> +
> +	payload++;
> +	for (i = 0; i < nr_xcede_records; i++) {
> +		struct xcede_latency_records *record = &xcede_records[i];
> +
> +		record->latency_hint = (u8)payload[0];
> +		record->wakeup_latency_tb_ticks  =
> +			be64_to_cpu(*(__be64 *)(&payload[1]));
> +		record->responsive_to_irqs = (u8)payload[9];
> +		payload += xcede_record_size;
> +		pr_info("xcede : Record %d : hint = %u, latency =0x%llx tb-ticks, Wake-on-irq = %u\n",
> +			i, record->latency_hint,
> +			record->wakeup_latency_tb_ticks,
> +			record->responsive_to_irqs);
> +	}
> +
> +	return 0;
> +}
> +
>  u8 cede_latency_hint[NR_DEDICATED_STATES];
>  static int dedicated_cede_loop(struct cpuidle_device *dev,
>  				struct cpuidle_driver *drv,
> @@ -238,6 +350,19 @@ static int pseries_cpuidle_driver_init(void)
>  	return 0;
>  }
> 
> +static int add_pseries_idle_states(void)
> +{
> +	int nr_states = 2; /* By default we have snooze, CEDE */
> +
> +	if (parse_cede_parameters())
> +		return nr_states;
> +
> +	pr_info("cpuidle : Skipping the %d Extended CEDE idle states\n",
> +		nr_xcede_records);
> +
> +	return nr_states;

More logic will be added to this function in the subsequent patches to
actually make use of the information that is obtained from the platform
firmware.

--Vaidy

Re: [PATCH 3/5] cpuidle-pseries : Fixup exit latency for CEDE(0)

[Vaidyanathan Srinivasan]

* Gautham R Shenoy <ego@linux.vnet.ibm.com> [2020-07-07 16:41:37]:

> From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
> 
> 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. The new
> value is 1us lesser than the smallest wakeup latency among the
> Extended CEDE states.
> 
> Benchmark results:
> 
> 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
> 
> 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
> 
> 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 <ego@linux.vnet.ibm.com>

Reviewed-by: Vaidyanathan Srinivasan <svaidy@linux.ibm.com>


> ---
>  drivers/cpuidle/cpuidle-pseries.c | 34 ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
> index c13549b..502f906 100644
> --- a/drivers/cpuidle/cpuidle-pseries.c
> +++ b/drivers/cpuidle/cpuidle-pseries.c
> @@ -353,12 +353,42 @@ static int pseries_cpuidle_driver_init(void)
>  static int add_pseries_idle_states(void)
>  {
>  	int nr_states = 2; /* By default we have snooze, CEDE */
> +	int i;
> +	u64 min_latency_us = dedicated_states[1].exit_latency; /* CEDE latency */
> 
>  	if (parse_cede_parameters())
>  		return nr_states;
> 
> -	pr_info("cpuidle : Skipping the %d Extended CEDE idle states\n",
> -		nr_xcede_records);
> +	for (i = 0; i < nr_xcede_records; i++) {
> +		u64 latency_tb = xcede_records[i].wakeup_latency_tb_ticks;
> +		u64 latency_us = tb_to_ns(latency_tb) / NSEC_PER_USEC;
> +
> +		if (latency_us < min_latency_us)
> +			min_latency_us = latency_us;
> +	}
> +
> +	/*
> +	 * 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 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);
> +	}


As per PAPR spec the CEDE hints are in increasing order of exit
latency.  Hence a given state's exit latency cannot exceed the one
following it.  The quirk is such that the first one (hint 0) is
implicit and hence we have to use the above logic to extract its
characteristics.

--Vaidy

Re: [PATCH 4/5] cpuidle-pseries : Include extended CEDE states in cpuidle framework

[Vaidyanathan Srinivasan]

* Gautham R Shenoy <ego@linux.vnet.ibm.com> [2020-07-07 16:41:38]:

> From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
> 
> This patch exposes those extended CEDE states to the cpuidle framework
> which are responsive to external interrupts and do not need an H_PROD.
> 
> Since as per the PAPR, all the extended CEDE states are non-responsive
> to timers, we indicate this to the cpuidle subsystem via the
> CPUIDLE_FLAG_TIMER_STOP flag for all those extende CEDE states which
> can wake up on external interrupts.
> 
> With the patch, we are able to see the extended CEDE state with
> latency hint = 1 exposed via the cpuidle framework.
> 
> 	$ cpupower idle-info
> 	CPUidle driver: pseries_idle
> 	CPUidle governor: menu
> 	analyzing CPU 0:
> 
> 	Number of idle states: 3
> 	Available idle states: snooze CEDE XCEDE1
> 	snooze:
> 	Flags/Description: snooze
> 	Latency: 0
> 	Usage: 33429446
> 	Duration: 27006062
> 	CEDE:
> 	Flags/Description: CEDE
> 	Latency: 1
> 	Usage: 10272
> 	Duration: 110786770
> 	XCEDE1:
> 	Flags/Description: XCEDE1
> 	Latency: 12
> 	Usage: 26445
> 	Duration: 1436433815
> 
> Benchmark results:
> TLDR: Over all we do not see any additional benefit from having XCEDE1 over
> CEDE.
> 
> ebizzy :
> 2 threads bound to a big-core. With this patch, we see a 3.39%
> regression compared to with only CEDE0 latency fixup.
> x With only CEDE0 latency fixup
> * With CEDE0 latency fixup + CEDE1
>     N           Min           Max        Median           Avg        Stddev
> x  10       2893813       5834474       5832448     5327281.3     1055941.4
> *  10       2907329       5834923       5831398     5146614.6     1193874.8
> 
> context_switch2:
> With the context_switch2 there are no observable regressions in the
> results.
> 
> context_switch2 CPU0 CPU1 (Same Big-core, different small-cores).
> No difference with and without patch.
> x without_patch
> * with_patch
>     N           Min           Max        Median           Avg        Stddev
> x 500        343644        348778        345444     345584.02     1035.1658
> * 500        344310        347646        345776     345877.22     802.19501
> 
> context_switch2 CPU0 CPU8 (different big-cores). Minor 0.05% improvement
> with patch
> x without_patch
> * with_patch
>     N           Min           Max        Median           Avg        Stddev
> x 500        287562        288756        288162     288134.76     262.24328
> * 500        287874        288960        288306     288274.66     187.57034
> 
> schbench:
> No regressions observed with schbench
> 
> Without 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
> 
> With Patch:
> Latency percentiles (usec)
> 	50.0th: 30
> 	75.0th: 40
> 	90.0th: 51
> 	95.0th: 59
> 	*99.0th: 13616
> 	99.5th: 14512
> 	99.9th: 15696
> 	min=0, max=15996
> 
> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>

Reviewed-by: Vaidyanathan Srinivasan <svaidy@linux.ibm.com>

> ---
>  drivers/cpuidle/cpuidle-pseries.c | 50 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
> index 502f906..6f893cd 100644
> --- a/drivers/cpuidle/cpuidle-pseries.c
> +++ b/drivers/cpuidle/cpuidle-pseries.c
> @@ -362,9 +362,59 @@ static int add_pseries_idle_states(void)
>  	for (i = 0; i < nr_xcede_records; i++) {
>  		u64 latency_tb = xcede_records[i].wakeup_latency_tb_ticks;
>  		u64 latency_us = tb_to_ns(latency_tb) / NSEC_PER_USEC;
> +		char name[CPUIDLE_NAME_LEN];
> +		unsigned int latency_hint = xcede_records[i].latency_hint;
> +		u64 residency_us;
> +
> +		if (!xcede_records[i].responsive_to_irqs) {
> +			pr_info("cpuidle : Skipping XCEDE%d. Not responsive to IRQs\n",
> +				latency_hint);
> +			continue;
> +		}
> 
>  		if (latency_us < min_latency_us)
>  			min_latency_us = latency_us;
> +		snprintf(name, CPUIDLE_NAME_LEN, "XCEDE%d", latency_hint);
> +
> +		/*
> +		 * As per the section 14.14.1 of PAPR version 2.8.1
> +		 * says that alling H_CEDE with the value of the cede
> +		 * latency specifier set greater than zero allows the
> +		 * processor timer facility to be disabled (so as not
> +		 * to cause gratuitous wake-ups - the use of H_PROD,
> +		 * or other external interrupt is required to wake the
> +		 * processor in this case).
> +		 *
> +		 * So, inform the cpuidle-subsystem that the timer
> +		 * will be stopped for these states.
> +		 *
> +		 * Also, bump up the latency by 10us, since cpuidle
> +		 * would use timer-offload framework which will need
> +		 * to send an IPI to wakeup a CPU whose timer has
> +		 * expired.
> +		 */
> +		if (latency_hint > 0) {
> +			dedicated_states[nr_states].flags = CPUIDLE_FLAG_TIMER_STOP;
> +			latency_us += 10;
> +		}
> +
> +		/*
> +		 * Thumb rule : Reside in the XCEDE state for at least
> +		 * 10x the time required to enter and exit that state.
> +		 */
> +		residency_us = latency_us * 10;
> +
> +		strlcpy(dedicated_states[nr_states].name, (const char *)name,
> +			CPUIDLE_NAME_LEN);
> +		strlcpy(dedicated_states[nr_states].desc, (const char *)name,
> +			CPUIDLE_NAME_LEN);
> +		dedicated_states[nr_states].exit_latency = latency_us;
> +		dedicated_states[nr_states].target_residency = residency_us;
> +		dedicated_states[nr_states].enter = &dedicated_cede_loop;
> +		cede_latency_hint[nr_states] = latency_hint;
> +		pr_info("cpuidle : Added %s. latency-hint = %d\n",
> +			name, latency_hint);
> +		nr_states++;

This patch demonstrates the various use cases of the previous patches
in the series that helps interface with the platform firmware better.
On current platforms these benefits are very limited, but the
framework built by the previous patches helps Linux exploit new and
enhanced idle states that will be available on newer platform and
firmware.

--Vaidy

Re: [PATCH 5/5] cpuidle-pseries: Block Extended CEDE(1) which adds no additional value.

[Vaidyanathan Srinivasan]

* Gautham R Shenoy <ego@linux.vnet.ibm.com> [2020-07-07 16:41:39]:

> From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
> 
> The Extended CEDE state with latency-hint = 1 is only different from
> normal CEDE (with latency-hint = 0) in that a CPU in Extended CEDE(1)
> does not wakeup on timer events. Both CEDE and Extended CEDE(1) map to
> the same hardware idle state. Since we already get SMT folding from
> the normal CEDE, the Extended CEDE(1) doesn't provide any additional
> value. This patch blocks Extended CEDE(1).
> 
> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>

Reviewed-by: Vaidyanathan Srinivasan <svaidy@linux.ibm.com>

> ---
>  drivers/cpuidle/cpuidle-pseries.c | 57 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 54 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
> index 6f893cd..be0b8b2 100644
> --- a/drivers/cpuidle/cpuidle-pseries.c
> +++ b/drivers/cpuidle/cpuidle-pseries.c
> @@ -350,6 +350,43 @@ static int pseries_cpuidle_driver_init(void)
>  	return 0;
>  }
> 
> +#define XCEDE1_HINT	1
> +#define ERR_NO_VALUE_ADD	(-1)
> +#define ERR_NO_EE_WAKEUP	(-2)
> +
> +/*
> + * Returns 0 if the Extende CEDE state with @hint is not blocked in
> + * cpuidle framework.
> + *
> + * Returns ERR_NO_EE_WAKEUP if the Extended CEDE state is blocked due
> + * to not being responsive to external interrupts.
> + *
> + * Returns ERR_NO_VALUE_ADD if the Extended CEDE state does not provide
> + * added value addition over the normal CEDE.
> + */
> +static int cpuidle_xcede_blocked(u8 hint, u64 latency_us, u8 responsive_to_irqs)
> +{
> +
> +	/*
> +	 * We will only allow extended CEDE states that are responsive
> +	 * to irqs do not require an H_PROD to be woken up.
> +	 */
> +	if (!responsive_to_irqs)
> +		return ERR_NO_EE_WAKEUP;
> +
> +	/*
> +	 * We already obtain SMT folding benefits from CEDE (which is
> +	 * CEDE with hint 0). Furthermore, CEDE is also responsive to
> +	 * timer-events, while XCEDE1 requires an external
> +	 * interrupt/H_PROD to be woken up. Hence, block XCEDE1 since
> +	 * it adds no further value.
> +	 */
> +	if (hint == XCEDE1_HINT)
> +		return ERR_NO_VALUE_ADD;
> +
> +	return 0;
> +}
> +
>  static int add_pseries_idle_states(void)
>  {
>  	int nr_states = 2; /* By default we have snooze, CEDE */
> @@ -365,15 +402,29 @@ static int add_pseries_idle_states(void)
>  		char name[CPUIDLE_NAME_LEN];
>  		unsigned int latency_hint = xcede_records[i].latency_hint;
>  		u64 residency_us;
> +		int rc;
> +
> +		if (latency_us < min_latency_us)
> +			min_latency_us = latency_us;
> +
> +		rc = cpuidle_xcede_blocked(latency_hint, latency_us,
> +					   xcede_records[i].responsive_to_irqs);
> 
> -		if (!xcede_records[i].responsive_to_irqs) {
> +		if (rc) {
> +			switch (rc) {
> +			case ERR_NO_VALUE_ADD:
> +				pr_info("cpuidle : Skipping XCEDE%d. No additional value-add\n",
> +					latency_hint);
> +				break;
> +			case ERR_NO_EE_WAKEUP:
>  			pr_info("cpuidle : Skipping XCEDE%d. Not responsive to IRQs\n",
>  				latency_hint);
> +			break;
> +			}
> +
>  			continue;
>  		}
> 
> -		if (latency_us < min_latency_us)
> -			min_latency_us = latency_us;
>  		snprintf(name, CPUIDLE_NAME_LEN, "XCEDE%d", latency_hint);
> 
>  		/*


We need these heuristics to select/reject idle states exposed by
platform firmware to Linux primarily because not all states are really
useful to Linux on a given setup.

--Vaidy

Re: [PATCH 0/5] cpuidle-pseries: Parse extended CEDE information for idle.

[Rafael J. Wysocki]

On Tue, Jul 7, 2020 at 1:32 PM Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:
>
> Hi,
>
> On Tue, Jul 07, 2020 at 04:41:34PM +0530, Gautham R. Shenoy wrote:
> > From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
> >
> > Hi,
> >
> >
> >
> >
> > Gautham R. Shenoy (5):
> >   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)
> >   cpuidle-pseries : Include extended CEDE states in cpuidle framework
> >   cpuidle-pseries: Block Extended CEDE(1) which adds no additional
> >     value.
>
> Forgot to mention that these patches are on top of Nathan's series to
> remove extended CEDE offline and bogus topology update code :
> https://lore.kernel.org/linuxppc-dev/20200612051238.1007764-1-nathanl@linux.ibm.com/

OK, so this is targeted at the powerpc maintainers, isn't it?

Re: [PATCH 0/5] cpuidle-pseries: Parse extended CEDE information for idle.

[Gautham R Shenoy]

Hello Rafael,

On Mon, Jul 27, 2020 at 04:14:12PM +0200, Rafael J. Wysocki wrote:
> On Tue, Jul 7, 2020 at 1:32 PM Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:
> >
> > Hi,
> >
> > On Tue, Jul 07, 2020 at 04:41:34PM +0530, Gautham R. Shenoy wrote:
> > > From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
> > >
> > > Hi,
> > >
> > >
> > >
> > >
> > > Gautham R. Shenoy (5):
> > >   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)
> > >   cpuidle-pseries : Include extended CEDE states in cpuidle framework
> > >   cpuidle-pseries: Block Extended CEDE(1) which adds no additional
> > >     value.
> >
> > Forgot to mention that these patches are on top of Nathan's series to
> > remove extended CEDE offline and bogus topology update code :
> > https://lore.kernel.org/linuxppc-dev/20200612051238.1007764-1-nathanl@linux.ibm.com/
> 
> OK, so this is targeted at the powerpc maintainers, isn't it?

Yes, the code is powerpc specific.

Also, I noticed that Nathan's patches have been merged by Michael
Ellerman in the powerpc/merge tree. I will rebase and post a v2 of
this patch series.

--
Thanks and Regards
gautham.