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
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(-)
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
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(-)
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
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(-)
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
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 <[email protected]>
> Signed-off-by: Nicholas Piggin <[email protected]>
Series applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/3fc5ee927ff4ffed6aa2fcd44d2fbf
cheers
On Thu, Jun 29, 2017 at 2:21 PM, Michael Ellerman
<[email protected]> 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 <[email protected]>
>> Signed-off-by: Nicholas Piggin <[email protected]>
>
> 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
"Rafael J. Wysocki" <[email protected]> writes:
> On Thu, Jun 29, 2017 at 2:21 PM, Michael Ellerman
> <[email protected]> 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 <[email protected]>
>>> Signed-off-by: Nicholas Piggin <[email protected]>
>>
>> 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
On Fri, Jun 30, 2017 at 5:45 AM, Michael Ellerman <[email protected]> wrote:
> "Rafael J. Wysocki" <[email protected]> writes:
>
>> On Thu, Jun 29, 2017 at 2:21 PM, Michael Ellerman
>> <[email protected]> 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 <[email protected]>
>>>> Signed-off-by: Nicholas Piggin <[email protected]>
>>>
>>> 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
"Rafael J. Wysocki" <[email protected]> writes:
> On Fri, Jun 30, 2017 at 5:45 AM, Michael Ellerman <[email protected]> wrote:
>> "Rafael J. Wysocki" <[email protected]> writes:
>>
>>> On Thu, Jun 29, 2017 at 2:21 PM, Michael Ellerman
>>> <[email protected]> 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 <[email protected]>
>>>>> Signed-off-by: Nicholas Piggin <[email protected]>
>>>>
>>>> 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