powerpc (powernv and pseries) cpuidle driver improvmeents

PATCH In review Nicholas Piggin 4 participants 9 messages Jun 14, 2017 – Jul 4, 2017

Subsystems (2)

PowerPC CPU idle time management framework

lore

Share & tools

lore

Nicholas Piggin

author reviewer

2017-06-14 13:02 UTC #0 lore raw

cover powerpc (powernv and pseries) cpuidle driver improvmeents

Hi,

These are a few small improvements that came from doing an optimisation pass over powerpc cpu idle paths.

Michael reminded me to cc the cpuidle maintainers. I think he will take the patches through the powerpc tree, but any suggestion or ack or nack would be welcome.

Thanks,
Nick

Nicholas Piggin (3):
cpuidle: powerpc: cpuidle set polling before enabling irqs
cpuidle: powerpc: read mostly for common globals
cpuidle: powerpc: no memory barrier after break from idle

 drivers/cpuidle/cpuidle-powernv.c | 25 +++++++++++++++++--------
 drivers/cpuidle/cpuidle-pseries.c | 22 +++++++++++++++-------
 2 files changed, 32 insertions(+), 15 deletions(-)

Signature · 1 line

2.11.0

Nicholas Piggin

author reviewer

2017-06-14 13:02 UTC #1 lore raw

PATCH 1/3 cpuidle: powerpc: cpuidle set polling before enabling irqs

local_irq_enable can cause interrupts to be taken which could take significant amount of processing time. The idle process should set its polling flag before this, so another process that wakes it during this time will not have to send an IPI.

Expand the TIF_POLLING_NRFLAG coverage to as large as possible.

Reviewed-by Gautham R. Shenoy <[email protected]>

Signed-off-by Nicholas Piggin <[email protected]>

---
 drivers/cpuidle/cpuidle-powernv.c | 4 +++-
 drivers/cpuidle/cpuidle-pseries.c | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

drivers/cpuidle/cpuidle-powernv.c +3 -1

index 45eaf06462ae..77bc50ad9f57 100644
@@ -51,9 +51,10 @@ static int snooze_loop(struct cpuidle_device *dev,
 {
 	u64 snooze_exit_time;
 
-	local_irq_enable();
 	set_thread_flag(TIF_POLLING_NRFLAG);
 
+	local_irq_enable();
+
 	snooze_exit_time = get_tb() + snooze_timeout;
 	ppc64_runlatch_off();
 	HMT_very_low();

@@ -66,6 +67,7 @@ static int snooze_loop(struct cpuidle_device *dev,
 	ppc64_runlatch_on();
 	clear_thread_flag(TIF_POLLING_NRFLAG);
 	smp_mb();
+
 	return index;
 }

drivers/cpuidle/cpuidle-pseries.c +2 -1

index 166ccd711ec9..7b12bb2ea70f 100644
@@ -62,9 +62,10 @@ static int snooze_loop(struct cpuidle_device *dev,
 	unsigned long in_purr;
 	u64 snooze_exit_time;
 
+	set_thread_flag(TIF_POLLING_NRFLAG);
+
 	idle_loop_prolog(&in_purr);
 	local_irq_enable();
-	set_thread_flag(TIF_POLLING_NRFLAG);
 	snooze_exit_time = get_tb() + snooze_timeout;
 
 	while (!need_resched()) {

Signature · 1 line

2.11.0

Michael Ellerman

2017-06-29 12:21 UTC #2 lore raw

Re: [1/3] cpuidle: powerpc: cpuidle set polling before enabling irqs

Quoted 9 lines · Nicholas Piggin

local_irq_enable can cause interrupts to be taken which could take significant amount of processing time. The idle process should set its polling flag before this, so another process that wakes it during this time will not have to send an IPI.
Expand the TIF_POLLING_NRFLAG coverage to as large as possible.
Reviewed-by Gautham R. Shenoy <[email protected]>
Signed-off-by Nicholas Piggin <[email protected]>

Series applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/3fc5ee927ff4ffed6aa2fcd44d2fbf

cheers

Rafael J. Wysocki

maintainer

2017-06-29 20:38 UTC #3 lore raw

Re: [1/3] cpuidle: powerpc: cpuidle set polling before enabling irqs

Quoted 6 lines · Michael Ellerman

Quoted 5 lines · Nicholas Piggin
local_irq_enable can cause interrupts to be taken which could take significant amount of processing time. The idle process should set its polling flag before this, so another process that wakes it during this time will not have to send an IPI.

Quoted 8 lines

Quoted 4 lines
Expand the TIF_POLLING_NRFLAG coverage to as large as possible.
Reviewed-by Gautham R. Shenoy <[email protected]>
Signed-off-by Nicholas Piggin <[email protected]>
Series applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/3fc5ee927ff4ffed6aa2fcd44d2fbf

I've applied it too, so I guess I should drop it?

Thanks,
Rafael

Michael Ellerman

reviewer

2017-06-30 03:45 UTC #4 lore raw

Re: [1/3] cpuidle: powerpc: cpuidle set polling before enabling irqs

Quoted 13 lines · Rafael J. Wysocki

Quoted 11 lines · Michael Ellerman
Quoted 9 lines · Nicholas Piggin
local_irq_enable can cause interrupts to be taken which could take significant amount of processing time. The idle process should set its polling flag before this, so another process that wakes it during this time will not have to send an IPI.
Expand the TIF_POLLING_NRFLAG coverage to as large as possible.
Reviewed-by Gautham R. Shenoy <[email protected]>
Signed-off-by Nicholas Piggin <[email protected]>

Quoted 7 lines

Quoted 3 lines
Series applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/3fc5ee927ff4ffed6aa2fcd44d2fbf
OK
I've applied it too, so I guess I should drop it?

Erk sorry. I hadn't heard anything so I picked it up.

If you can drop it that would be good, but if not git will probably work it out mostly :)

cheers

Rafael J. Wysocki

maintainer

2017-06-30 12:51 UTC #5 lore raw

Re: [1/3] cpuidle: powerpc: cpuidle set polling before enabling irqs

Quoted 19 lines · Michael Ellerman

Quoted 17 lines · Rafael J. Wysocki
Quoted 14 lines · Michael Ellerman
Quoted 9 lines · Nicholas Piggin
local_irq_enable can cause interrupts to be taken which could take significant amount of processing time. The idle process should set its polling flag before this, so another process that wakes it during this time will not have to send an IPI.
Expand the TIF_POLLING_NRFLAG coverage to as large as possible.
Reviewed-by Gautham R. Shenoy <[email protected]>
Signed-off-by Nicholas Piggin <[email protected]>
Series applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/3fc5ee927ff4ffed6aa2fcd44d2fbf

Quoted 8 lines

Quoted 3 lines
OK
I've applied it too, so I guess I should drop it?
Erk sorry. I hadn't heard anything so I picked it up.
If you can drop it that would be good, but if not git will probably work it out mostly :)

I've dropped it, no problem.

Thanks,
Rafael

Michael Ellerman

reviewer

2017-07-04 02:59 UTC #6 lore raw

Re: [1/3] cpuidle: powerpc: cpuidle set polling before enabling irqs

Quoted 22 lines · Rafael J. Wysocki

Quoted 21 lines · Michael Ellerman
Quoted 19 lines · Rafael J. Wysocki
Quoted 6 lines · Michael Ellerman
Quoted 5 lines · Nicholas Piggin
local_irq_enable can cause interrupts to be taken which could take significant amount of processing time. The idle process should set its polling flag before this, so another process that wakes it during this time will not have to send an IPI.
Quoted 8 lines
Quoted 4 lines
Expand the TIF_POLLING_NRFLAG coverage to as large as possible.
Reviewed-by Gautham R. Shenoy <[email protected]>
Signed-off-by Nicholas Piggin <[email protected]>
Series applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/3fc5ee927ff4ffed6aa2fcd44d2fbf
OK

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
I've applied it too, so I guess I should drop it?
Erk sorry. I hadn't heard anything so I picked it up.
If you can drop it that would be good, but if not git will probably work it out mostly :)
I've dropped it, no problem.

Thanks.

cheers

Nicholas Piggin

author reviewer

2017-06-14 13:02 UTC #7 lore raw

PATCH 2/3 cpuidle: powerpc: read mostly for common globals

In reply to Nicholas Piggin

Ensure these don't get put into bouncing cachelines.

Reviewed-by Vaidyanathan Srinivasan <[email protected]>

Reviewed-by Gautham R. Shenoy <[email protected]>

Signed-off-by Nicholas Piggin <[email protected]>

---
 drivers/cpuidle/cpuidle-powernv.c | 10 +++++-----
 drivers/cpuidle/cpuidle-pseries.c |  8 ++++----
 2 files changed, 9 insertions(+), 9 deletions(-)

drivers/cpuidle/cpuidle-powernv.c +5 -5

index 77bc50ad9f57..abf2ffcd4a0a 100644
@@ -32,18 +32,18 @@ static struct cpuidle_driver powernv_idle_driver = {
 	.owner            = THIS_MODULE,
 };
 
-static int max_idle_state;
-static struct cpuidle_state *cpuidle_state_table;
+static int max_idle_state __read_mostly;
+static struct cpuidle_state *cpuidle_state_table __read_mostly;
 
 struct stop_psscr_table {
 	u64 val;
 	u64 mask;
 };
 
-static struct stop_psscr_table stop_psscr_table[CPUIDLE_STATE_MAX];
+static struct stop_psscr_table stop_psscr_table[CPUIDLE_STATE_MAX] __read_mostly;
 
-static u64 snooze_timeout;
-static bool snooze_timeout_en;
+static u64 snooze_timeout __read_mostly;
+static bool snooze_timeout_en __read_mostly;
 
 static int snooze_loop(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv,

drivers/cpuidle/cpuidle-pseries.c +4 -4

index 7b12bb2ea70f..a404f352d284 100644
@@ -25,10 +25,10 @@ struct cpuidle_driver pseries_idle_driver = {
 	.owner            = THIS_MODULE,
 };
 
-static int max_idle_state;
-static struct cpuidle_state *cpuidle_state_table;
-static u64 snooze_timeout;
-static bool snooze_timeout_en;
+static int max_idle_state __read_mostly;
+static struct cpuidle_state *cpuidle_state_table __read_mostly;
+static u64 snooze_timeout __read_mostly;
+static bool snooze_timeout_en __read_mostly;
 
 static inline void idle_loop_prolog(unsigned long *in_purr)
 {

Signature · 1 line

2.11.0

Nicholas Piggin

author reviewer

2017-06-14 13:02 UTC #8 lore raw

PATCH 3/3 cpuidle: powerpc: no memory barrier after break from idle

In reply to Nicholas Piggin

A memory barrier is not required after the task wakes up, only if we clear the polling flag before waking. The case where we have work to do is the important one, so optimise for it.

Reviewed-by Vaidyanathan Srinivasan <[email protected]>

Signed-off-by Nicholas Piggin <[email protected]>

---
 drivers/cpuidle/cpuidle-powernv.c | 11 +++++++++--
 drivers/cpuidle/cpuidle-pseries.c | 11 +++++++++--
 2 files changed, 18 insertions(+), 4 deletions(-)

drivers/cpuidle/cpuidle-powernv.c +9 -2

index abf2ffcd4a0a..722b81b03593 100644
@@ -59,14 +59,21 @@ static int snooze_loop(struct cpuidle_device *dev,
 	ppc64_runlatch_off();
 	HMT_very_low();
 	while (!need_resched()) {
-		if (likely(snooze_timeout_en) && get_tb() > snooze_exit_time)
+		if (likely(snooze_timeout_en) && get_tb() > snooze_exit_time) {
+			/*
+			 * Task has not woken up but we are exiting the polling
+			 * loop anyway. Require a barrier after polling is
+			 * cleared to order subsequent test of need_resched().
+			 */
+			clear_thread_flag(TIF_POLLING_NRFLAG);
+			smp_mb();
 			break;
+		}
 	}
 
 	HMT_medium();
 	ppc64_runlatch_on();
 	clear_thread_flag(TIF_POLLING_NRFLAG);
-	smp_mb();
 
 	return index;
 }

drivers/cpuidle/cpuidle-pseries.c +9 -2

index a404f352d284..e9b3853d93ea 100644
@@ -71,13 +71,20 @@ static int snooze_loop(struct cpuidle_device *dev,
 	while (!need_resched()) {
 		HMT_low();
 		HMT_very_low();
-		if (snooze_timeout_en && get_tb() > snooze_exit_time)
+		if (likely(snooze_timeout_en) && get_tb() > snooze_exit_time) {
+			/*
+			 * Task has not woken up but we are exiting the polling
+			 * loop anyway. Require a barrier after polling is
+			 * cleared to order subsequent test of need_resched().
+			 */
+			clear_thread_flag(TIF_POLLING_NRFLAG);
+			smp_mb();
 			break;
+		}
 	}
 
 	HMT_medium();
 	clear_thread_flag(TIF_POLLING_NRFLAG);
-	smp_mb();
 
 	idle_loop_epilog(in_purr);

Signature · 1 line

2.11.0