[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.

[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.

[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