[PATCH 0/3] powerpc (powernv and pseries) cpuidle driver improvmeents

[PATCH 0/3] powerpc (powernv and pseries) cpuidle driver improvmeents

[Nicholas Piggin]

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(-)

-- 
2.11.0

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

[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 <ego@linux.vnet.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 drivers/cpuidle/cpuidle-powernv.c | 4 +++-
 drivers/cpuidle/cpuidle-pseries.c | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
index 45eaf06462ae..77bc50ad9f57 100644
--- a/drivers/cpuidle/cpuidle-powernv.c
+++ b/drivers/cpuidle/cpuidle-powernv.c
@@ -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;
 }
 
diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
index 166ccd711ec9..7b12bb2ea70f 100644
--- a/drivers/cpuidle/cpuidle-pseries.c
+++ b/drivers/cpuidle/cpuidle-pseries.c
@@ -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()) {
-- 
2.11.0

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

[Michael Ellerman]

On Wed, 2017-06-14 at 13:02:39 UTC, Nicholas Piggin wrote:
> 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 <ego@linux.vnet.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Series applied to powerpc next, thanks.

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

cheers

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

[Rafael J. Wysocki]

On Thu, Jun 29, 2017 at 2:21 PM, Michael Ellerman
<patch-notifications@ellerman.id.au> wrote:
> On Wed, 2017-06-14 at 13:02:39 UTC, Nicholas Piggin wrote:
>> 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 <ego@linux.vnet.ibm.com>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>
> 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?

Thanks,
Rafael

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

[Michael Ellerman]

"Rafael J. Wysocki" <rafael@kernel.org> writes:

> On Thu, Jun 29, 2017 at 2:21 PM, Michael Ellerman
> <patch-notifications@ellerman.id.au> wrote:
>> On Wed, 2017-06-14 at 13:02:39 UTC, Nicholas Piggin wrote:
>>> 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 <ego@linux.vnet.ibm.com>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>
>> 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

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

[Rafael J. Wysocki]

On Fri, Jun 30, 2017 at 5:45 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> "Rafael J. Wysocki" <rafael@kernel.org> writes:
>
>> On Thu, Jun 29, 2017 at 2:21 PM, Michael Ellerman
>> <patch-notifications@ellerman.id.au> wrote:
>>> On Wed, 2017-06-14 at 13:02:39 UTC, Nicholas Piggin wrote:
>>>> 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 <ego@linux.vnet.ibm.com>
>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>
>>> 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 :)

I've dropped it, no problem.

Thanks,
Rafael

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

[Michael Ellerman]

"Rafael J. Wysocki" <rafael@kernel.org> writes:

> On Fri, Jun 30, 2017 at 5:45 AM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>> "Rafael J. Wysocki" <rafael@kernel.org> writes:
>>
>>> On Thu, Jun 29, 2017 at 2:21 PM, Michael Ellerman
>>> <patch-notifications@ellerman.id.au> wrote:
>>>> On Wed, 2017-06-14 at 13:02:39 UTC, Nicholas Piggin wrote:
>>>>> 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 <ego@linux.vnet.ibm.com>
>>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>>
>>>> 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 :)
>
> I've dropped it, no problem.

Thanks.

cheers

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

[Nicholas Piggin]

Ensure these don't get put into bouncing cachelines.

Reviewed-by: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 drivers/cpuidle/cpuidle-powernv.c | 10 +++++-----
 drivers/cpuidle/cpuidle-pseries.c |  8 ++++----
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
index 77bc50ad9f57..abf2ffcd4a0a 100644
--- a/drivers/cpuidle/cpuidle-powernv.c
+++ b/drivers/cpuidle/cpuidle-powernv.c
@@ -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,
diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
index 7b12bb2ea70f..a404f352d284 100644
--- a/drivers/cpuidle/cpuidle-pseries.c
+++ b/drivers/cpuidle/cpuidle-pseries.c
@@ -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)
 {
-- 
2.11.0

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

[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 <svaidy@linux.vnet.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 drivers/cpuidle/cpuidle-powernv.c | 11 +++++++++--
 drivers/cpuidle/cpuidle-pseries.c | 11 +++++++++--
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
index abf2ffcd4a0a..722b81b03593 100644
--- a/drivers/cpuidle/cpuidle-powernv.c
+++ b/drivers/cpuidle/cpuidle-powernv.c
@@ -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;
 }
diff --git a/drivers/cpuidle/cpuidle-pseries.c b/drivers/cpuidle/cpuidle-pseries.c
index a404f352d284..e9b3853d93ea 100644
--- a/drivers/cpuidle/cpuidle-pseries.c
+++ b/drivers/cpuidle/cpuidle-pseries.c
@@ -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);
 
-- 
2.11.0