[Qemu-devel] Re: [PATCH 2/3] Fix time drift problem under high load when PIT is in use.

[Qemu-devel] Re: [PATCH 2/3] Fix time drift problem under high load when PIT is in use.

[Dor Laor]

On Sun, 2008-06-29 at 16:59 +0300, Gleb Natapov wrote:
> Count the number of interrupts that was lost due to interrupt coalescing
> and re-inject them back when possible. This fixes time drift problem when
> pit is used as a time source.
> 
> Signed-off-by: Gleb Natapov <gleb@qumranet.com>
> ---
> 
>  hw/i8254.c |   20 +++++++++++++++++++-
>  1 files changed, 19 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/i8254.c b/hw/i8254.c
> index 4813b03..c4f0f46 100644
> --- a/hw/i8254.c
> +++ b/hw/i8254.c
> @@ -61,6 +61,8 @@ static PITState pit_state;
>  
>  static void pit_irq_timer_update(PITChannelState *s, int64_t current_time);
>  
> +static uint32_t pit_irq_coalesced;

The pit has 3 channels, it should be a channel field.

Also every time the pit frequency changes the above field should be
compensated with * (new_freq/old_freq). 
For example, if the guest was running with 1000hz clock and the
pit_irq_coalesced value is 100 currently, a frequency change to 100hz
should reduce pit_irq_coalesced to 10.

Except that, its high time we stop drifting :)

> +
>  static int pit_get_count(PITChannelState *s)
>  {
>      uint64_t d;
> @@ -369,12 +371,28 @@ static void pit_irq_timer_update(PITChannelState *s, int64_t current_time)
>          return;
>      expire_time = pit_get_next_transition_time(s, current_time);
>      irq_level = pit_get_out1(s, current_time);
> -    qemu_set_irq(s->irq, irq_level);
> +    if(irq_level) {
> +        if(!qemu_irq_raise(s->irq))
> +            pit_irq_coalesced++;
> +    } else {
> +        qemu_irq_lower(s->irq);
> +        if(pit_irq_coalesced > 0) {
> +            if(qemu_irq_raise(s->irq))
> +                pit_irq_coalesced--;
> +            qemu_irq_lower(s->irq);
> +        }
> +    }
> +
>  #ifdef DEBUG_PIT
>      printf("irq_level=%d next_delay=%f\n",
>             irq_level,
>             (double)(expire_time - current_time) / ticks_per_sec);
>  #endif
> +    if(pit_irq_coalesced && expire_time != -1) {
> +        uint32_t div = ((pit_irq_coalesced >> 10) & 0x7f) + 2;
> +        expire_time -= ((expire_time - current_time) / div);
> +    }
> +
>      s->next_transition_time = expire_time;
>      if (expire_time != -1)
>          qemu_mod_timer(s->irq_timer, expire_time);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Qemu-devel] [PATCH 0/3] Fix guest time drift under heavy load.

[Gleb Natapov]

Resending one more time. There was no response last time I've sent it.

Qemu device emulation for timers might be inaccurate and
causes coalescing of several IRQs into one. It happens when the
load on the host is high and the guest did not manage to ack the
previous IRQ. The first patch in the series fixes this by changing
qemu_irq subsystem to return IRQ delivery status information.
If device is notified that IRQs where lost it can regenerate them
as needed. The following two patches add IRQ regeneration to PIC and RTC
devices.

---

Gleb Natapov (3):
      Fix time drift problem under high load when RTC is in use.
      Fix time drift problem under high load when PIT is in use.
      Change qemu_set_irq() to return status information.


 hw/apic.c        |  103 +++++++++++++++++++++++++++++++++++-------------------
 hw/esp.c         |    4 ++
 hw/fdc.c         |    4 ++
 hw/i8254.c       |   20 ++++++++++
 hw/i8259.c       |   19 +++++++---
 hw/ide.c         |    8 +++-
 hw/irq.c         |   10 +++--
 hw/irq.h         |   22 +++++++-----
 hw/max7310.c     |    4 ++
 hw/mc146818rtc.c |   11 +++++-
 hw/pc.c          |    6 ++-
 hw/pc.h          |    2 +
 hw/pci.c         |    8 +++-
 hw/ssd0323.c     |    4 ++
 hw/twl92230.c    |    8 +++-
 15 files changed, 159 insertions(+), 74 deletions(-)

-- 
		Gleb.

[Qemu-devel] [PATCH 2/3] Fix time drift problem under high load when PIT is in use.

[Gleb Natapov]

Count the number of interrupts that was lost due to interrupt coalescing
and re-inject them back when possible. This fixes time drift problem when
pit is used as a time source.

Signed-off-by: Gleb Natapov <gleb@qumranet.com>
---

 hw/i8254.c |   20 +++++++++++++++++++-
 1 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/hw/i8254.c b/hw/i8254.c
index 4813b03..c4f0f46 100644
--- a/hw/i8254.c
+++ b/hw/i8254.c
@@ -61,6 +61,8 @@ static PITState pit_state;
 
 static void pit_irq_timer_update(PITChannelState *s, int64_t current_time);
 
+static uint32_t pit_irq_coalesced;
+
 static int pit_get_count(PITChannelState *s)
 {
     uint64_t d;
@@ -369,12 +371,28 @@ static void pit_irq_timer_update(PITChannelState *s, int64_t current_time)
         return;
     expire_time = pit_get_next_transition_time(s, current_time);
     irq_level = pit_get_out1(s, current_time);
-    qemu_set_irq(s->irq, irq_level);
+    if(irq_level) {
+        if(!qemu_irq_raise(s->irq))
+            pit_irq_coalesced++;
+    } else {
+        qemu_irq_lower(s->irq);
+        if(pit_irq_coalesced > 0) {
+            if(qemu_irq_raise(s->irq))
+                pit_irq_coalesced--;
+            qemu_irq_lower(s->irq);
+        }
+    }
+
 #ifdef DEBUG_PIT
     printf("irq_level=%d next_delay=%f\n",
            irq_level,
            (double)(expire_time - current_time) / ticks_per_sec);
 #endif
+    if(pit_irq_coalesced && expire_time != -1) {
+        uint32_t div = ((pit_irq_coalesced >> 10) & 0x7f) + 2;
+        expire_time -= ((expire_time - current_time) / div);
+    }
+
     s->next_transition_time = expire_time;
     if (expire_time != -1)
         qemu_mod_timer(s->irq_timer, expire_time);

[Qemu-devel] Re: [PATCH 2/3] Fix time drift problem under high load when PIT is in use.

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2262 bytes --]

Gleb Natapov wrote:
> Count the number of interrupts that was lost due to interrupt coalescing
> and re-inject them back when possible. This fixes time drift problem when
> pit is used as a time source.
> 
> Signed-off-by: Gleb Natapov <gleb@qumranet.com>
> ---
> 
>  hw/i8254.c |   20 +++++++++++++++++++-
>  1 files changed, 19 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/i8254.c b/hw/i8254.c
> index 4813b03..c4f0f46 100644
> --- a/hw/i8254.c
> +++ b/hw/i8254.c
> @@ -61,6 +61,8 @@ static PITState pit_state;
>  
>  static void pit_irq_timer_update(PITChannelState *s, int64_t current_time);
>  
> +static uint32_t pit_irq_coalesced;
> +
>  static int pit_get_count(PITChannelState *s)
>  {
>      uint64_t d;
> @@ -369,12 +371,28 @@ static void pit_irq_timer_update(PITChannelState *s, int64_t current_time)
>          return;
>      expire_time = pit_get_next_transition_time(s, current_time);
>      irq_level = pit_get_out1(s, current_time);
> -    qemu_set_irq(s->irq, irq_level);
> +    if(irq_level) {
> +        if(!qemu_irq_raise(s->irq))
> +            pit_irq_coalesced++;
> +    } else {
> +        qemu_irq_lower(s->irq);
> +        if(pit_irq_coalesced > 0) {
> +            if(qemu_irq_raise(s->irq))
> +                pit_irq_coalesced--;
> +            qemu_irq_lower(s->irq);
> +        }
> +    }

That's graspable for my poor brain: reinject one coalesced IRQ right
after the falling edge of an in-time delivery...

> +
>  #ifdef DEBUG_PIT
>      printf("irq_level=%d next_delay=%f\n",
>             irq_level,
>             (double)(expire_time - current_time) / ticks_per_sec);
>  #endif
> +    if(pit_irq_coalesced && expire_time != -1) {
> +        uint32_t div = ((pit_irq_coalesced >> 10) & 0x7f) + 2;
> +        expire_time -= ((expire_time - current_time) / div);
> +    }
> +

... but could you comment on this algorithm? I think I got what it does:
splitting up the next regular period in short intervals. But there are a
bit too many magic numbers involved. Where do they come from? What
happens if pit_irq_coalesced becomes large (or: how large can it become
without risking an IRQ storm on the guest)? A few comments would help, I
guess.

Thanks,
Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] Re: [PATCH 2/3] Fix time drift problem under high load when PIT is in use.

[Gleb Natapov]

On Sun, Jun 29, 2008 at 04:40:43PM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > Count the number of interrupts that was lost due to interrupt coalescing
> > and re-inject them back when possible. This fixes time drift problem when
> > pit is used as a time source.
> > 
> > Signed-off-by: Gleb Natapov <gleb@qumranet.com>
> > ---
> > 
> >  hw/i8254.c |   20 +++++++++++++++++++-
> >  1 files changed, 19 insertions(+), 1 deletions(-)
> > 
> > diff --git a/hw/i8254.c b/hw/i8254.c
> > index 4813b03..c4f0f46 100644
> > --- a/hw/i8254.c
> > +++ b/hw/i8254.c
> > @@ -61,6 +61,8 @@ static PITState pit_state;
> >  
> >  static void pit_irq_timer_update(PITChannelState *s, int64_t current_time);
> >  
> > +static uint32_t pit_irq_coalesced;
> > +
> >  static int pit_get_count(PITChannelState *s)
> >  {
> >      uint64_t d;
> > @@ -369,12 +371,28 @@ static void pit_irq_timer_update(PITChannelState *s, int64_t current_time)
> >          return;
> >      expire_time = pit_get_next_transition_time(s, current_time);
> >      irq_level = pit_get_out1(s, current_time);
> > -    qemu_set_irq(s->irq, irq_level);
> > +    if(irq_level) {
> > +        if(!qemu_irq_raise(s->irq))
> > +            pit_irq_coalesced++;
> > +    } else {
> > +        qemu_irq_lower(s->irq);
> > +        if(pit_irq_coalesced > 0) {
> > +            if(qemu_irq_raise(s->irq))
> > +                pit_irq_coalesced--;
> > +            qemu_irq_lower(s->irq);
> > +        }
> > +    }
> 
> That's graspable for my poor brain: reinject one coalesced IRQ right
> after the falling edge of an in-time delivery...
This works because I speed up timer that calls this function. I can
create another timer just for injection of lost interrupts instead of
doing it here.

> 
> > +
> >  #ifdef DEBUG_PIT
> >      printf("irq_level=%d next_delay=%f\n",
> >             irq_level,
> >             (double)(expire_time - current_time) / ticks_per_sec);
> >  #endif
> > +    if(pit_irq_coalesced && expire_time != -1) {
> > +        uint32_t div = ((pit_irq_coalesced >> 10) & 0x7f) + 2;
> > +        expire_time -= ((expire_time - current_time) / div);
> > +    }
> > +
> 
> ... but could you comment on this algorithm? I think I got what it does:
> splitting up the next regular period in short intervals. But there are a
> bit too many magic numbers involved. Where do they come from? What
> happens if pit_irq_coalesced becomes large (or: how large can it become
> without risking an IRQ storm on the guest)? A few comments would help, I
> guess.
> 

The numbers are come from empirical data :) The algorithm divide next
time interval by value that depends on number of coalesced interrupts.
The divider value can be any number between 2 and 129. The formula is:
div = (pit_irq_coalesced / A) % B + 2
I chose A and B so that bit operation could be used instead of
arithmetic.

--
			Gleb.