Re: [Qemu-devel] [PATCH v3 5/5] hpet 'driftfix': add code in hpet_timer() to compensate delayed callbacks and coalesced interrupts

[Marcelo Tosatti <mtosatti@redhat.com>]

On Thu, Apr 28, 2011 at 04:25:00PM +0200, 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 <uobergfe@redhat.com>
> ---
>  hw/hpet.c |   63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 61 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/hpet.c b/hw/hpet.c
> index 35466ae..92d5f58 100644
> --- a/hw/hpet.c
> +++ b/hw/hpet.c
> @@ -32,6 +32,7 @@
>  #include "sysbus.h"
>  #include "mc146818rtc.h"
>  #include "sysemu.h"
> +#include <assert.h>
>  
>  //#define HPET_DEBUG
>  #ifdef HPET_DEBUG
> @@ -42,6 +43,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*/
> @@ -326,28 +330,63 @@ static const VMStateDescription vmstate_hpet = {
>      }
>  };
>  
> +static bool hpet_timer_has_tick_backlog(HPETTimer *t)
> +{
> +    uint64_t 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 irq_count = 0;
>      uint64_t period = t->period;
>      uint64_t cur_tick = hpet_get_ticks(t->state);
>  
> +    if (s->driftfix && !t->ticks_not_accounted) {
> +        t->ticks_not_accounted = t->prev_period = t->period;
> +    }
>      if (timer_is_periodic(t) && period != 0) {
>          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;
> +                irq_count++;
>              }
>          } else {
>              while (hpet_time_after64(cur_tick, t->cmp)) {
>                  t->cmp += period;
> +                t->ticks_not_accounted += period;
> +                irq_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 || irq_count > 1) {
> +                    t->irq_rate++;
> +                    t->irq_rate = MIN(t->irq_rate, MAX_IRQ_RATE);
> +                }
> +                if (t->divisor == 0) {
> +                    assert(irq_count);
> +                }
> +                if (irq_count) {
> +                    t->divisor = t->irq_rate;
> +                }
> +                diff /= t->divisor--;
> +            } 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)) {
> @@ -358,7 +397,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 (irq_count) {
> +                    t->irq_rate++;
> +                    t->irq_rate = MIN(t->irq_rate, MAX_IRQ_RATE);
> +                }
> +            }
> +        }
> +    } else {
> +        update_irq(t, 1);
> +    }
>  }

Hi Ulrich,

Whats prev_period for, since in practice the period will not change
between interrupts (OS programs comparator once, or perhaps twice during
bootup) ?

Other than that, shouldnt reset accounting variables to init state on
write to GLOBAL_ENABLE_CFG / writes to main counter?