* [PATCH 2/3] cpuidle: powernv: Don't continually set thread priority in snooze_loop()
2017-04-03 21:54 [PATCH 1/3] cpuidle: powernv: Don't bounce between low and very low thread priority Anton Blanchard
@ 2017-04-03 21:54 ` Anton Blanchard
2017-04-04 4:06 ` Vaidyanathan Srinivasan
2017-04-03 21:54 ` [PATCH 3/3] cpuidle: powernv: Avoid a branch in the core snooze_loop() loop Anton Blanchard
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Anton Blanchard @ 2017-04-03 21:54 UTC (permalink / raw)
To: benh, paulus, mpe, svaidy, ego, rjw, daniel.lezcano, npiggin
Cc: linuxppc-dev, linux-pm
From: Anton Blanchard <anton@samba.org>
The powerpc64 kernel exception handlers have preserved thread priorities
for a long time now, so there is no need to continually set it.
Just set it once on entry and once exit.
Signed-off-by: Anton Blanchard <anton@samba.org>
---
drivers/cpuidle/cpuidle-powernv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
index 9d9f164894eb..8c991c254b95 100644
--- a/drivers/cpuidle/cpuidle-powernv.c
+++ b/drivers/cpuidle/cpuidle-powernv.c
@@ -56,8 +56,8 @@ static int snooze_loop(struct cpuidle_device *dev,
snooze_exit_time = get_tb() + snooze_timeout;
ppc64_runlatch_off();
+ HMT_very_low();
while (!need_resched()) {
- HMT_very_low();
if (snooze_timeout_en && get_tb() > snooze_exit_time)
break;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 2/3] cpuidle: powernv: Don't continually set thread priority in snooze_loop()
2017-04-03 21:54 ` [PATCH 2/3] cpuidle: powernv: Don't continually set thread priority in snooze_loop() Anton Blanchard
@ 2017-04-04 4:06 ` Vaidyanathan Srinivasan
0 siblings, 0 replies; 9+ messages in thread
From: Vaidyanathan Srinivasan @ 2017-04-04 4:06 UTC (permalink / raw)
To: Anton Blanchard
Cc: benh, paulus, mpe, ego, rjw, daniel.lezcano, npiggin,
linuxppc-dev, linux-pm
* Anton Blanchard <anton@ozlabs.org> [2017-04-04 07:54:13]:
> From: Anton Blanchard <anton@samba.org>
>
> The powerpc64 kernel exception handlers have preserved thread priorities
> for a long time now, so there is no need to continually set it.
>
> Just set it once on entry and once exit.
>
> Signed-off-by: Anton Blanchard <anton@samba.org>
Reviewed-by: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
> ---
> drivers/cpuidle/cpuidle-powernv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
> index 9d9f164894eb..8c991c254b95 100644
> --- a/drivers/cpuidle/cpuidle-powernv.c
> +++ b/drivers/cpuidle/cpuidle-powernv.c
> @@ -56,8 +56,8 @@ static int snooze_loop(struct cpuidle_device *dev,
>
> snooze_exit_time = get_tb() + snooze_timeout;
> ppc64_runlatch_off();
> + HMT_very_low();
> while (!need_resched()) {
> - HMT_very_low();
> if (snooze_timeout_en && get_tb() > snooze_exit_time)
> break;
> }
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] cpuidle: powernv: Avoid a branch in the core snooze_loop() loop
2017-04-03 21:54 [PATCH 1/3] cpuidle: powernv: Don't bounce between low and very low thread priority Anton Blanchard
2017-04-03 21:54 ` [PATCH 2/3] cpuidle: powernv: Don't continually set thread priority in snooze_loop() Anton Blanchard
@ 2017-04-03 21:54 ` Anton Blanchard
2017-04-03 23:54 ` Nicholas Piggin
2017-04-04 4:10 ` Vaidyanathan Srinivasan
2017-04-03 23:52 ` [PATCH 1/3] cpuidle: powernv: Don't bounce between low and very low thread priority Nicholas Piggin
2017-04-04 4:04 ` Vaidyanathan Srinivasan
3 siblings, 2 replies; 9+ messages in thread
From: Anton Blanchard @ 2017-04-03 21:54 UTC (permalink / raw)
To: benh, paulus, mpe, svaidy, ego, rjw, daniel.lezcano, npiggin
Cc: linuxppc-dev, linux-pm
From: Anton Blanchard <anton@samba.org>
When in the snooze_loop() we want to take up the least amount of
resources. On my version of gcc (6.3), we end up with an extra
branch because it predicts snooze_timeout_en to be false, whereas it
is almost always true.
Use likely() to avoid the branch and be a little nicer to the
other non idle threads on the core.
Signed-off-by: Anton Blanchard <anton@samba.org>
---
drivers/cpuidle/cpuidle-powernv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
index 8c991c254b95..251a60bfa8ee 100644
--- a/drivers/cpuidle/cpuidle-powernv.c
+++ b/drivers/cpuidle/cpuidle-powernv.c
@@ -58,7 +58,7 @@ static int snooze_loop(struct cpuidle_device *dev,
ppc64_runlatch_off();
HMT_very_low();
while (!need_resched()) {
- if (snooze_timeout_en && get_tb() > snooze_exit_time)
+ if (likely(snooze_timeout_en) && get_tb() > snooze_exit_time)
break;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 3/3] cpuidle: powernv: Avoid a branch in the core snooze_loop() loop
2017-04-03 21:54 ` [PATCH 3/3] cpuidle: powernv: Avoid a branch in the core snooze_loop() loop Anton Blanchard
@ 2017-04-03 23:54 ` Nicholas Piggin
2017-04-04 4:10 ` Vaidyanathan Srinivasan
1 sibling, 0 replies; 9+ messages in thread
From: Nicholas Piggin @ 2017-04-03 23:54 UTC (permalink / raw)
To: Anton Blanchard
Cc: benh, paulus, mpe, svaidy, ego, rjw, daniel.lezcano,
linuxppc-dev, linux-pm
On Tue, 4 Apr 2017 07:54:14 +1000
Anton Blanchard <anton@ozlabs.org> wrote:
> From: Anton Blanchard <anton@samba.org>
>
> When in the snooze_loop() we want to take up the least amount of
> resources. On my version of gcc (6.3), we end up with an extra
> branch because it predicts snooze_timeout_en to be false, whereas it
> is almost always true.
>
> Use likely() to avoid the branch and be a little nicer to the
> other non idle threads on the core.
Patches 2 and 3 look fine. Should they be replicated to cpuidle-pseries.c
as well?
Thanks,
Nick
>
> Signed-off-by: Anton Blanchard <anton@samba.org>
> ---
> drivers/cpuidle/cpuidle-powernv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
> index 8c991c254b95..251a60bfa8ee 100644
> --- a/drivers/cpuidle/cpuidle-powernv.c
> +++ b/drivers/cpuidle/cpuidle-powernv.c
> @@ -58,7 +58,7 @@ static int snooze_loop(struct cpuidle_device *dev,
> ppc64_runlatch_off();
> HMT_very_low();
> while (!need_resched()) {
> - if (snooze_timeout_en && get_tb() > snooze_exit_time)
> + if (likely(snooze_timeout_en) && get_tb() > snooze_exit_time)
> break;
> }
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 3/3] cpuidle: powernv: Avoid a branch in the core snooze_loop() loop
2017-04-03 21:54 ` [PATCH 3/3] cpuidle: powernv: Avoid a branch in the core snooze_loop() loop Anton Blanchard
2017-04-03 23:54 ` Nicholas Piggin
@ 2017-04-04 4:10 ` Vaidyanathan Srinivasan
1 sibling, 0 replies; 9+ messages in thread
From: Vaidyanathan Srinivasan @ 2017-04-04 4:10 UTC (permalink / raw)
To: Anton Blanchard
Cc: benh, paulus, mpe, ego, rjw, daniel.lezcano, npiggin,
linuxppc-dev, linux-pm
* Anton Blanchard <anton@ozlabs.org> [2017-04-04 07:54:14]:
> From: Anton Blanchard <anton@samba.org>
>
> When in the snooze_loop() we want to take up the least amount of
> resources. On my version of gcc (6.3), we end up with an extra
> branch because it predicts snooze_timeout_en to be false, whereas it
> is almost always true.
By default snooze_timeout_en is true. It will become false only when
we do not want to exit the snooze loop and that will be when snooze is
the only idle state available in the platform, which is a rare case.
> Use likely() to avoid the branch and be a little nicer to the
> other non idle threads on the core.
>
> Signed-off-by: Anton Blanchard <anton@samba.org>
Reviewed-by: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
> ---
> drivers/cpuidle/cpuidle-powernv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
> index 8c991c254b95..251a60bfa8ee 100644
> --- a/drivers/cpuidle/cpuidle-powernv.c
> +++ b/drivers/cpuidle/cpuidle-powernv.c
> @@ -58,7 +58,7 @@ static int snooze_loop(struct cpuidle_device *dev,
> ppc64_runlatch_off();
> HMT_very_low();
> while (!need_resched()) {
> - if (snooze_timeout_en && get_tb() > snooze_exit_time)
> + if (likely(snooze_timeout_en) && get_tb() > snooze_exit_time)
> break;
> }
>
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] cpuidle: powernv: Don't bounce between low and very low thread priority
2017-04-03 21:54 [PATCH 1/3] cpuidle: powernv: Don't bounce between low and very low thread priority Anton Blanchard
2017-04-03 21:54 ` [PATCH 2/3] cpuidle: powernv: Don't continually set thread priority in snooze_loop() Anton Blanchard
2017-04-03 21:54 ` [PATCH 3/3] cpuidle: powernv: Avoid a branch in the core snooze_loop() loop Anton Blanchard
@ 2017-04-03 23:52 ` Nicholas Piggin
2017-04-04 4:13 ` Vaidyanathan Srinivasan
2017-04-04 4:04 ` Vaidyanathan Srinivasan
3 siblings, 1 reply; 9+ messages in thread
From: Nicholas Piggin @ 2017-04-03 23:52 UTC (permalink / raw)
To: Anton Blanchard
Cc: benh, paulus, mpe, svaidy, ego, rjw, daniel.lezcano,
linuxppc-dev, linux-pm
On Tue, 4 Apr 2017 07:54:12 +1000
Anton Blanchard <anton@ozlabs.org> wrote:
> From: Anton Blanchard <anton@samba.org>
>
> The core of snooze_loop() continually bounces between low and very
> low thread priority. Changing thread priorities is an expensive
> operation that can negatively impact other threads on a core.
>
> All CPUs that can run PowerNV support very low priority, so we can
> avoid the change completely.
This looks good. I have HMT_lowest() which does alt feature patching
we can use for pseries and default idle code.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] cpuidle: powernv: Don't bounce between low and very low thread priority
2017-04-03 23:52 ` [PATCH 1/3] cpuidle: powernv: Don't bounce between low and very low thread priority Nicholas Piggin
@ 2017-04-04 4:13 ` Vaidyanathan Srinivasan
0 siblings, 0 replies; 9+ messages in thread
From: Vaidyanathan Srinivasan @ 2017-04-04 4:13 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Anton Blanchard, benh, paulus, mpe, ego, rjw, daniel.lezcano,
linuxppc-dev, linux-pm
* Nicholas Piggin <npiggin@gmail.com> [2017-04-04 09:52:07]:
> On Tue, 4 Apr 2017 07:54:12 +1000
> Anton Blanchard <anton@ozlabs.org> wrote:
>
> > From: Anton Blanchard <anton@samba.org>
> >
> > The core of snooze_loop() continually bounces between low and very
> > low thread priority. Changing thread priorities is an expensive
> > operation that can negatively impact other threads on a core.
> >
> > All CPUs that can run PowerNV support very low priority, so we can
> > avoid the change completely.
>
> This looks good. I have HMT_lowest() which does alt feature patching
> we can use for pseries and default idle code.
Alternatively, if we are going to set priority only once in various
other places, HMT_low(); HMT_very_low(); should not add to extra
cycles. Let me code that up.
--Vaidy
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] cpuidle: powernv: Don't bounce between low and very low thread priority
2017-04-03 21:54 [PATCH 1/3] cpuidle: powernv: Don't bounce between low and very low thread priority Anton Blanchard
` (2 preceding siblings ...)
2017-04-03 23:52 ` [PATCH 1/3] cpuidle: powernv: Don't bounce between low and very low thread priority Nicholas Piggin
@ 2017-04-04 4:04 ` Vaidyanathan Srinivasan
3 siblings, 0 replies; 9+ messages in thread
From: Vaidyanathan Srinivasan @ 2017-04-04 4:04 UTC (permalink / raw)
To: Anton Blanchard
Cc: benh, paulus, mpe, ego, rjw, daniel.lezcano, npiggin,
linuxppc-dev, linux-pm
* Anton Blanchard <anton@ozlabs.org> [2017-04-04 07:54:12]:
> From: Anton Blanchard <anton@samba.org>
>
> The core of snooze_loop() continually bounces between low and very
> low thread priority. Changing thread priorities is an expensive
> operation that can negatively impact other threads on a core.
>
> All CPUs that can run PowerNV support very low priority, so we can
> avoid the change completely.
>
> Signed-off-by: Anton Blanchard <anton@samba.org>
Reviewed-by: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
> ---
> drivers/cpuidle/cpuidle-powernv.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
> index cda8f62d555b..9d9f164894eb 100644
> --- a/drivers/cpuidle/cpuidle-powernv.c
> +++ b/drivers/cpuidle/cpuidle-powernv.c
> @@ -57,7 +57,6 @@ static int snooze_loop(struct cpuidle_device *dev,
> snooze_exit_time = get_tb() + snooze_timeout;
> ppc64_runlatch_off();
> while (!need_resched()) {
> - HMT_low();
> HMT_very_low();
> if (snooze_timeout_en && get_tb() > snooze_exit_time)
> break;
HMT_low() is legacy and can be removed for powernv platforms.
--Vaidy
^ permalink raw reply [flat|nested] 9+ messages in thread