qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v4 5/5] hpet 'driftfix': add code in hpet_timer(


From: Zachary Amsden
Subject: Re: [Qemu-devel] [PATCH v4 5/5] hpet 'driftfix': add code in hpet_timer() to compensate delayed callbacks and coalesced interrupts
Date: Wed, 11 May 2011 18:58:55 -0700
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.10) Gecko/20100621 Fedora/3.0.5-1.fc13 Thunderbird/3.0.5

On 05/09/2011 12:03 AM, Ulrich Obergfell wrote:
Loss of periodic timer interrupts caused by delayed callbacks and by
interrupt coalescing is compensated by gradually injecting additional
interrupts during subsequent timer intervals, starting at a rate of
one additional interrupt per interval. The injection of additional
interrupts is based on a backlog of unaccounted HPET clock periods
(new HPETTimer field 'ticks_not_accounted'). The backlog increases
due to delayed callbacks and coalesced interrupts, and it decreases
if an interrupt was injected successfully. If the backlog increases
while compensation is still in progress, the rate at which additional
interrupts are injected is increased too. A limit is imposed on the
backlog and on the rate.

Injecting additional timer interrupts to compensate lost interrupts
can alleviate long term time drift. However, on a short time scale,
this method can have the side effect of making virtual machine time
intermittently pass slower and faster than real time (depending on
the guest's time keeping algorithm). Compensation is disabled by
default and can be enabled for guests where this behaviour may be
acceptable.

Signed-off-by: Ulrich Obergfell<address@hidden>
---
  hw/hpet.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
  1 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/hw/hpet.c b/hw/hpet.c
index e57c654..519fc6b 100644
--- a/hw/hpet.c
+++ b/hw/hpet.c
@@ -31,6 +31,7 @@
  #include "hpet_emul.h"
  #include "sysbus.h"
  #include "mc146818rtc.h"
+#include<assert.h>

  //#define HPET_DEBUG
  #ifdef HPET_DEBUG
@@ -41,6 +42,9 @@

  #define HPET_MSI_SUPPORT        0

+#define MAX_TICKS_NOT_ACCOUNTED     (uint64_t)500000000 /* 5 sec */
+#define MAX_IRQ_RATE                (uint32_t)10
+
  struct HPETState;
  typedef struct HPETTimer {  /* timers */
      uint8_t tn;             /*timer number*/
@@ -324,14 +328,35 @@ static const VMStateDescription vmstate_hpet = {
      }
  };

+static void hpet_timer_driftfix_reset(HPETTimer *t)
+{
+    if (t->state->driftfix&&  timer_is_periodic(t)) {
+        t->ticks_not_accounted = t->prev_period = t->period;

This is rather confusing. Clearly, ticks_not_accounted isn't actually ticks not accounted, it's a different variable entirely which is based on the current period.

If it were actually ticks_not_accounted, it should be reset to zero.

+        t->irq_rate = 1;
+        t->divisor = 1;
+    }
+}
+
+static bool hpet_timer_has_tick_backlog(HPETTimer *t)
+{
+    uint64_t backlog = 0;
+
+    if (t->ticks_not_accounted>= t->period + t->prev_period) {
+        backlog = t->ticks_not_accounted - (t->period + t->prev_period);
+    }
+    return (backlog>= t->period);
+}
+
  /*
   * timer expiration callback
   */
  static void hpet_timer(void *opaque)
  {
      HPETTimer *t = opaque;
+    HPETState *s = t->state;
      uint64_t diff;
-
+    int irq_delivered = 0;
+    uint32_t period_count = 0;
      uint64_t period = t->period;
      uint64_t cur_tick = hpet_get_ticks(t->state);

@@ -339,13 +364,37 @@ static void hpet_timer(void *opaque)
          if (t->config&  HPET_TN_32BIT) {
              while (hpet_time_after(cur_tick, t->cmp)) {
                  t->cmp = (uint32_t)(t->cmp + t->period);
+                t->ticks_not_accounted += t->period;
+                period_count++;
              }
          } else {
              while (hpet_time_after64(cur_tick, t->cmp)) {
                  t->cmp += period;
+                t->ticks_not_accounted += period;
+                period_count++;
              }
          }
          diff = hpet_calculate_diff(t, cur_tick);
+        if (s->driftfix) {
+            if (t->ticks_not_accounted>  MAX_TICKS_NOT_ACCOUNTED) {
+                t->ticks_not_accounted = t->period + t->prev_period;
+            }
+            if (hpet_timer_has_tick_backlog(t)) {
+                if (t->irq_rate == 1 || period_count>  1) {
+                    t->irq_rate++;
+                    t->irq_rate = MIN(t->irq_rate, MAX_IRQ_RATE);
+                }
+                if (t->divisor == 0) {
+                    assert(period_count);
+                }
+                if (period_count) {
+                    t->divisor = t->irq_rate;
+                }
+                diff /= t->divisor--;

Why subtracting from the divisor? Shouldn't the divisor and irq_rate change in lockstep?

+            } else {
+                t->irq_rate = 1;
+            }
+        }
          qemu_mod_timer(t->qemu_timer,
                         qemu_get_clock_ns(vm_clock) + 
(int64_t)ticks_to_ns(diff));
      } else if (t->config&  HPET_TN_32BIT&&  !timer_is_periodic(t)) {
@@ -356,7 +405,22 @@ static void hpet_timer(void *opaque)
              t->wrap_flag = 0;
          }
      }
-    update_irq(t, 1);
+    if (s->driftfix&&  timer_is_periodic(t)&&  period != 0) {
+        if (t->ticks_not_accounted>= t->period + t->prev_period) {
+            irq_delivered = update_irq(t, 1);
+            if (irq_delivered) {
+                t->ticks_not_accounted -= t->prev_period;
+                t->prev_period = t->period;
+            } else {
+                if (period_count) {
+                    t->irq_rate++;
+                    t->irq_rate = MIN(t->irq_rate, MAX_IRQ_RATE);
+                }
+            }
+        }
+    } else {
+        update_irq(t, 1);
+    }
  }

Overall I think the intention is correct, and the code may be as well, but anything with this amount of subtle manipulations and math tricks needs to be more well commented for others to understand it.

I'll try to parse it again later, hopefully with better luck,

Zach



reply via email to

[Prev in Thread] Current Thread [Next in Thread]