diff mbox

[2/5] mc146818rtc: fix clock lost after scaling coalesced irq

Message ID 20170412095111.11728-3-xiaoguangrong@tencent.com (mailing list archive)
State New, archived
Headers show

Commit Message

Xiao Guangrong April 12, 2017, 9:51 a.m. UTC 
From: Xiao Guangrong <xiaoguangrong@tencent.com>

If the period is changed by re-configuring RegA, the coalesced irq
will be scaled to reflect the new period, however, it calculates
the new interrupt number like this:
    s->irq_coalesced = (s->irq_coalesced * s->period) / period;

There are some clocks will be lost if they are not enough to be
squeezed to a single new period that will cause the VM clock slower

In order to fix the issue, we calculate the interrupt window based
on the precise clock rather than period, then the clocks lost during
period is scaled can be compensated properly

Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 hw/timer/mc146818rtc.c | 39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

Comments

Paolo Bonzini May 3, 2017, 3:15 p.m. UTC | #1 
On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote:
> +            int current_irq_coalesced = s->irq_coalesced;
> +
> +            s->irq_coalesced = (current_irq_coalesced * s->period) / period;
> +
> +            /*
> +             * calculate the lost clock after it is scaled which should be
> +             * compensated in the next interrupt.
> +             */
> +            lost_clock += current_irq_coalesced * s->period -
> +                            s->irq_coalesced * period;

This is:

   lost_clock = current_irq_coalesced * s->period -
	(current_irq_coalesced * s->period) / period * period;

i.e.

   /* When switching from a shorter to a longer period, scale down the
    * missing ticks since we expect the OS handler to treat the delayed
    * ticks as longer.  Any leftovers are put back into next_irq_clock.
    *
    * When switching to a shorter period, scale up the missing ticks
    * since we expect the OS handler to treat the delayed ticks as
    * shorter.
    */
   lost_clock = (s->irq_coalesced * s->period) % period;
   s->irq_coalesced = (s->irq_coalesced * s->period) / period;

Is this correct?

Paolo

> +            DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld clocks "
> +                      "are compensated.\n",
> +                      current_irq_coalesced, s->irq_coalesced, lost_clock);
>          }
>          s->period = period;
>  #endif
> @@ -170,7 +193,7 @@ static void periodic_timer_update(RTCState *s, int64_t current_time)
>          cur_clock =
>              muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
>  
> -        next_irq_clock = (cur_clock & ~(period - 1)) + period;
> +        next_irq_clock = cur_clock + period - lost_clock;
>          s->next_periodic_time = muldiv64(next_irq_clock, NANOSECONDS_PER_SECOND,
>                                           RTC_CLOCK_RATE) + 1;
>          timer_mod(s->periodic_timer, s->next_periodic_time);
> --
Xiao Guangrong May 4, 2017, 2:51 a.m. UTC | #2 
On 05/03/2017 11:15 PM, Paolo Bonzini wrote:
> 
> 
> On 12/04/2017 11:51, guangrong.xiao@gmail.com wrote:
>> +            int current_irq_coalesced = s->irq_coalesced;
>> +
>> +            s->irq_coalesced = (current_irq_coalesced * s->period) / period;
>> +
>> +            /*
>> +             * calculate the lost clock after it is scaled which should be
>> +             * compensated in the next interrupt.
>> +             */
>> +            lost_clock += current_irq_coalesced * s->period -
>> +                            s->irq_coalesced * period;
> 
> This is:
> 
>     lost_clock = current_irq_coalesced * s->period -
> 	(current_irq_coalesced * s->period) / period * period;
> 
> i.e.
> 
>     /* When switching from a shorter to a longer period, scale down the
>      * missing ticks since we expect the OS handler to treat the delayed
>      * ticks as longer.  Any leftovers are put back into next_irq_clock.
>      *
>      * When switching to a shorter period, scale up the missing ticks
>      * since we expect the OS handler to treat the delayed ticks as
>      * shorter.
>      */
>     lost_clock = (s->irq_coalesced * s->period) % period;
>     s->irq_coalesced = (s->irq_coalesced * s->period) / period;
> 
> Is this correct?
> 

Yes, it is correct, it looks smarter, will apply it in the next version.

Thanks!
diff mbox

Patch

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 749e206..649678c 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -146,23 +146,46 @@  static void rtc_coalesced_timer(void *opaque)
 }
 #endif
 
+static int period_code_to_clock(int period_code)
+{
+    /* periodic timer is disabled. */
+    if (!period_code) {
+        return 0;
+    }
+
+    if (period_code <= 2) {
+        period_code += 7;
+    }
+
+    /* period in 32 Khz cycles */
+    return 1 << (period_code - 1);
+}
+
 /* handle periodic timer */
 static void periodic_timer_update(RTCState *s, int64_t current_time)
 {
     int period_code, period;
-    int64_t cur_clock, next_irq_clock;
+    int64_t cur_clock, next_irq_clock, lost_clock = 0;
 
     period_code = s->cmos_data[RTC_REG_A] & 0x0f;
     if (period_code != 0
         && (s->cmos_data[RTC_REG_B] & REG_B_PIE)) {
-        if (period_code <= 2)
-            period_code += 7;
-        /* period in 32 Khz cycles */
-        period = 1 << (period_code - 1);
+        period = period_code_to_clock(period_code);
 #ifdef TARGET_I386
         if (period != s->period) {
-            s->irq_coalesced = (s->irq_coalesced * s->period) / period;
-            DPRINTF_C("cmos: coalesced irqs scaled to %d\n", s->irq_coalesced);
+            int current_irq_coalesced = s->irq_coalesced;
+
+            s->irq_coalesced = (current_irq_coalesced * s->period) / period;
+
+            /*
+             * calculate the lost clock after it is scaled which should be
+             * compensated in the next interrupt.
+             */
+            lost_clock += current_irq_coalesced * s->period -
+                            s->irq_coalesced * period;
+            DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, %ld clocks "
+                      "are compensated.\n",
+                      current_irq_coalesced, s->irq_coalesced, lost_clock);
         }
         s->period = period;
 #endif
@@ -170,7 +193,7 @@  static void periodic_timer_update(RTCState *s, int64_t current_time)
         cur_clock =
             muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
 
-        next_irq_clock = (cur_clock & ~(period - 1)) + period;
+        next_irq_clock = cur_clock + period - lost_clock;
         s->next_periodic_time = muldiv64(next_irq_clock, NANOSECONDS_PER_SECOND,
                                          RTC_CLOCK_RATE) + 1;
         timer_mod(s->periodic_timer, s->next_periodic_time);