[Qemu-devel] [PATCH 1.7 v2 0/2] PPC: Fix BookE timer performance regression

[Qemu-devel] [PATCH 1.7 v2 0/2] PPC: Fix BookE timer performance regression

[Alexander Graf]

Thanks to the new timer infrastructure we are now able to trigger timer events
and ridiculous granularities in sub-microsecond orders.

However, the BookE targets were quite unhappy about that change, showing up
to x10 slowdown on a simple Linux guest bootup test.

This patch set makes the constant timer facilities in BookE more lazy and less
fine grained than they could be. That way we're at least as good as we were in
QEMU 1.6.

---

v1 -> v2:

  - add parenthesis
  - use QEMUTimer *
  - s/microsecond/millisecond/g

Alexander Graf (2):
  PPC: Make BookE FIT/WDT timers more lazy
  PPC: BookE: Make FIT/WDT timers at best millisecond grained

 hw/ppc/ppc_booke.c | 49 ++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 44 insertions(+), 5 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 1.7 v2 1/2] PPC: Make BookE FIT/WDT timers more lazy

[Alexander Graf]

Today we fire FIT and WDT timer events every time the respective bit
position in TB flips from 0 -> 1.

However, there is no need to do this if the end result would be that
we're changing a TSR bit that is set to 1 to 1 again. No guest visible
change would have occured.

So whenever we see that the TSR bit to our timer is already set, don't
even bother to update the timer that would potentially fire it off.

However, we do need to make sure that we update our timer that notifies
us of the TB flip when the respective TSR bit gets unset. In that case
we do care about the flip and need to notify the guest again. So add
a callback into our timer handlers when TSR bits get unset.

This improves performance for me when the guest is busy processing things.

Signed-off-by: Alexander Graf <agraf@suse.de>

---

v1 -> v2:

  - add parenthesis
  - use QEMUTimer *
---
 hw/ppc/ppc_booke.c | 43 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 38 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/ppc_booke.c b/hw/ppc/ppc_booke.c
index 8bbfc72..56c4196 100644
--- a/hw/ppc/ppc_booke.c
+++ b/hw/ppc/ppc_booke.c
@@ -128,7 +128,8 @@ static uint8_t booke_get_wdt_target(CPUPPCState *env, ppc_tb_t *tb_env)
 static void booke_update_fixed_timer(CPUPPCState         *env,
                                      uint8_t           target_bit,
                                      uint64_t          *next,
-                                     struct QEMUTimer *timer)
+                                     QEMUTimer         *timer,
+                                     int               tsr_bit)
 {
     ppc_tb_t *tb_env = env->tb_env;
     uint64_t delta_tick, ticks = 0;
@@ -136,6 +137,14 @@ static void booke_update_fixed_timer(CPUPPCState         *env,
     uint64_t period;
     uint64_t now;
 
+    if (!(env->spr[SPR_BOOKE_TSR] & tsr_bit)) {
+        /*
+         * Don't arm the timer again when the guest has the current
+         * interrupt still pending. Wait for it to ack it.
+         */
+        return;
+    }
+
     now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
     tb  = cpu_ppc_get_tb(tb_env, now, tb_env->tb_offset);
     period = 1ULL << target_bit;
@@ -167,6 +176,7 @@ static void booke_update_fixed_timer(CPUPPCState         *env,
         (*next)++;
     }
 
+    /* Fire the next timer */
     timer_mod(timer, *next);
 }
 
@@ -200,7 +210,8 @@ static void booke_fit_cb(void *opaque)
     booke_update_fixed_timer(env,
                              booke_get_fit_target(env, tb_env),
                              &booke_timer->fit_next,
-                             booke_timer->fit_timer);
+                             booke_timer->fit_timer,
+                             TSR_FIS);
 }
 
 static void booke_wdt_cb(void *opaque)
@@ -220,15 +231,35 @@ static void booke_wdt_cb(void *opaque)
     booke_update_fixed_timer(env,
                              booke_get_wdt_target(env, tb_env),
                              &booke_timer->wdt_next,
-                             booke_timer->wdt_timer);
+                             booke_timer->wdt_timer,
+                             TSR_WIS);
 }
 
 void store_booke_tsr(CPUPPCState *env, target_ulong val)
 {
     PowerPCCPU *cpu = ppc_env_get_cpu(env);
+    ppc_tb_t *tb_env = env->tb_env;
+    booke_timer_t *booke_timer = tb_env->opaque;
 
     env->spr[SPR_BOOKE_TSR] &= ~val;
     kvmppc_clear_tsr_bits(cpu, val);
+
+    if (val & TSR_FIS) {
+        booke_update_fixed_timer(env,
+                                 booke_get_fit_target(env, tb_env),
+                                 &booke_timer->fit_next,
+                                 booke_timer->fit_timer,
+                                 TSR_FIS);
+    }
+
+    if (val & TSR_WIS) {
+        booke_update_fixed_timer(env,
+                                 booke_get_wdt_target(env, tb_env),
+                                 &booke_timer->wdt_next,
+                                 booke_timer->wdt_timer,
+                                 TSR_WIS);
+    }
+
     booke_update_irq(cpu);
 }
 
@@ -247,12 +278,14 @@ void store_booke_tcr(CPUPPCState *env, target_ulong val)
     booke_update_fixed_timer(env,
                              booke_get_fit_target(env, tb_env),
                              &booke_timer->fit_next,
-                             booke_timer->fit_timer);
+                             booke_timer->fit_timer,
+                             TSR_FIS);
 
     booke_update_fixed_timer(env,
                              booke_get_wdt_target(env, tb_env),
                              &booke_timer->wdt_next,
-                             booke_timer->wdt_timer);
+                             booke_timer->wdt_timer,
+                             TSR_WIS);
 }
 
 static void ppc_booke_timer_reset_handle(void *opaque)
-- 
1.8.1.4

[Qemu-devel] [PATCH 1.7 v2 2/2] PPC: BookE: Make FIT/WDT timers at best millisecond grained

[Alexander Graf]

The default granularity for the FIT timer on 440 is on every 0x1000th
transition of TB from 0 to 1. Translated that means 48828 times a second.

Since interrupts are quite expensive for 440 and we don't really care
about the accuracy of the FIT to that significance, let's force FIT and
WDT to at best millisecond granularity.

This basically restores behavior as it was in QEMU 1.6, where timers
could only deal with millisecond granularities at all.

This patch greatly improves performance with the 440 target and restores
roughly the same performance level that QEMU 1.6 had for me.

Signed-off-by: Alexander Graf <agraf@suse.de>

---

v1 -> v2:

  - s/microseconds/milliseconds/g
---
 hw/ppc/ppc_booke.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/ppc/ppc_booke.c b/hw/ppc/ppc_booke.c
index 56c4196..b421620 100644
--- a/hw/ppc/ppc_booke.c
+++ b/hw/ppc/ppc_booke.c
@@ -174,6 +174,12 @@ static void booke_update_fixed_timer(CPUPPCState         *env,
 
     if (*next == now) {
         (*next)++;
+    } else {
+        /*
+         * There's no point to fake any granularity that's more fine grained
+         * than milliseconds. Anything beyond that just overloads the system.
+         */
+        *next = MAX(*next, now + SCALE_MS);
     }
 
     /* Fire the next timer */
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 1.7 v2 2/2] PPC: BookE: Make FIT/WDT timers at best millisecond grained

[Stefan Weil]

Am 25.11.2013 22:46, schrieb Alexander Graf:
> The default granularity for the FIT timer on 440 is on every 0x1000th
> transition of TB from 0 to 1. Translated that means 48828 times a second.
>
> Since interrupts are quite expensive for 440 and we don't really care
> about the accuracy of the FIT to that significance, let's force FIT and
> WDT to at best millisecond granularity.
>
> This basically restores behavior as it was in QEMU 1.6, where timers
> could only deal with millisecond granularities at all.
>
> This patch greatly improves performance with the 440 target and restores
> roughly the same performance level that QEMU 1.6 had for me.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
>
> ---
>
> v1 -> v2:
>
>   - s/microseconds/milliseconds/g
> ---
>  hw/ppc/ppc_booke.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/ppc/ppc_booke.c b/hw/ppc/ppc_booke.c
> index 56c4196..b421620 100644
> --- a/hw/ppc/ppc_booke.c
> +++ b/hw/ppc/ppc_booke.c
> @@ -174,6 +174,12 @@ static void booke_update_fixed_timer(CPUPPCState         *env,
>  
>      if (*next == now) {
>          (*next)++;
> +    } else {
> +        /*
> +         * There's no point to fake any granularity that's more fine grained
> +         * than milliseconds. Anything beyond that just overloads the system.
> +         */
> +        *next = MAX(*next, now + SCALE_MS);
>      }
>  
>      /* Fire the next timer */

Reviewed-by: Stefan Weil <sw@weilnetz.de>

Looking closer at the code above, I think that the 'if' part (*next)++
could be removed because the 'else' part does a better job with *next =
*next + SCALE_MS when *next == now. This might further improve the
performance. Maybe I am wrong and the guest expects the next timer
interrupt immediately in this case, then delaying it one millisecond
would be bad.

If you decide to make a v3 patch with this modification, you may use my
reviewed-by, too.

I cannot say much to patch 1 because I don't know the BookE hardware.
The code looks good.

Regards,
Stefan