On Tue, May 03, 2011 at 07:08:25PM -0300, Glauber Costa wrote:
> On Tue, 2011-05-03 at 16:03 -0300, Marcelo Tosatti wrote:
> > 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@xxxxxxxxxx>
> > > ---
> > > 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) ?
>
> Actually, some systems, like Windows 2000-and-something change this
> period quite heavily under multimedia workloads.
I see. Still, using the period at the previous timer handler instance in
the decision whether to inject an interrupt to the guest makes no sense
to me.
> > Other than that, shouldnt reset accounting variables to init state on
> > write to GLOBAL_ENABLE_CFG / writes to main counter?
What i meant to ask here is whether the reinjection state (including
ticks_not_accounted) should be reset whenever a different period is
programmed.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
