Re: [PATCH] Add a timer mode that disables pending missed ticks

[Dave Winchell <dwinchell@virtualiron.com>]

Hi Keir,

You can help me analyze the Linux code.
Here is the code from sles (linux-2.6.5-7.244) x86_64.


static irqreturn_t timer_interrupt(int irq, void *dev_id, struct pt_regs 
*regs)

        spin_lock(&i8253_lock);
        outb_p(0x00, 0x43);
        delay = inb_p(0x40);
        delay |= inb(0x40) << 8;
        spin_unlock(&i8253_lock);
        delay = LATCH - 1 - delay;

     rdtscll_sync(&tsc);
       
        offset = (((tsc - vxtime.last_tsc) *
               vxtime.tsc_quot) >> 32) - (USEC_PER_SEC / HZ);

        if (offset < 0)
            offset = 0;

        if (offset > (USEC_PER_SEC / HZ)) {
            lost = offset / (USEC_PER_SEC / HZ);
            offset %= (USEC_PER_SEC / HZ);
        }

        monotonic_base += (tsc - vxtime.last_tsc)*1000000/cpu_khz ;

        vxtime.last_tsc = tsc - vxtime.quot * delay / vxtime.tsc_quot;

        if ((((tsc - vxtime.last_tsc) *
              vxtime.tsc_quot) >> 32) < offset)
            vxtime.last_tsc = tsc -
                (((long) offset << 32) / vxtime.tsc_quot) - 1;
   

    if (lost > 0) {
        if (report_lost_ticks) {
            printk(KERN_WARNING "time.c: Lost %ld timer "
                   "tick(s)! ", lost);
            print_symbol("rip %s)\n", regs->rip);
        }
        jiffies += lost;
    }
    do_timer(regs);


Now as 'offset' gets modified it represents the remainder in the
lost calculation. Our data indicates that when offset is close to zero,
timekeeping is more accurate. So this leads us to this line of code:

        if ((((tsc - vxtime.last_tsc) *
              vxtime.tsc_quot) >> 32) < offset)
            vxtime.last_tsc = tsc -
                (((long) offset << 32) / vxtime.tsc_quot) - 1;

Thus the problem seems to be the way the code switches from adjusting 
last_tsc
by the delay which is the normal mode when interrupts are timely or SYNCed
to handling offsets larger than the delay.

I'm concerned about the way delay is blown off in the final calculation

            vxtime.last_tsc = tsc -
                (((long) offset << 32) / vxtime.tsc_quot) - 1;


If an interrupt is right on time, and the delay is the same as last time,
then the offset should be zero in my mind. In the code above, offset will
equal delay for this example.

What do you think?

thanks,
Dave



Keir Fraser wrote:

>Oh dear, that was a silly typo on my part. Thanks for tracking it down!
>
>Well, I'm surprised that SYNC vs ASYNC differs, if the guest is really
>tracking time via the TSC value. But it sounds like the actual time-tracking
>algorithm in the guest must be a bit more complicated than that? I'd be very
>happy to help with any further analysis.
>
> -- Keir
>
>On 7/11/07 14:39, "Dave Winchell" <dwinchell@virtualiron.com> wrote:
>
>  
>
>>Keir,
>>
>>Attached is a fix for pt_process_missed_ticks().
>>Without this fix, systems run very erratically and some
>>guests panic on boot complaining that timer interrupts
>>are not working. As you can imagine.
>>
>>Also, I have some longer term measurements of the
>>accuracy of the sync and async methods.
>>The hardware is an eight cpu AMD box. Two eight
>>vcpu guests, rh4u4-64 and sles9sp3-64.
>>All vcpus running loads.
>>
>>Method   Test duration          Clock errors
>>
>>SYNC       56000 sec         6.4, 6.7 sec   (.012%)
>>ASYNC      52000 sec         13, 19 sec     (.036%)
>>
>>More testing should be done to validate the significance
>>of this difference.
>>
>>regards,
>>Dave
>>
>>
>>Dave Winchell wrote:
>>
>>    
>>
>>>Keir Fraser wrote:
>>>
>>>      
>>>
>>>>On 3/11/07 21:17, "Dave Winchell" <dwinchell@virtualiron.com> wrote:
>>>>
>>>> 
>>>>
>>>>        
>>>>
>>>>>Thanks for applying the fixes in the last submit.
>>>>>In moving the test for no_missed_tick_accounting into
>>>>>pt_process_missed_ticks()
>>>>>you forgot to add the scheduling part.
>>>>>  
>>>>>          
>>>>>
>>>>Actually it was deliberate, but clearly it was one code
>>>>simplification too
>>>>far: thanks for spotting it! I'll go the async route, but we do need
>>>>to set
>>>>pending_intr_nr to 1. We can't leave that out -- the point of the async
>>>>route is to send a tick to the vcpu immediately, since it hasn't had
>>>>one for
>>>>more than a tick period. If we wait for the timeout to do that then
>>>>we have
>>>>to wait a whole extra period after the vcpu is re-scheduled.
>>>>
>>>>Attached is my proposed patch. I think it's quite neat. :-)
>>>> 
>>>>
>>>>        
>>>>
>>>It looks good to me.
>>>
>>>thanks,
>>>Dave
>>>
>>>      
>>>
>>>>-- Keir
>>>>
>>>> 
>>>>
>>>>        
>>>>
>>diff -r dfe9c0c10a2c xen/arch/x86/hvm/vpt.c
>>--- a/xen/arch/x86/hvm/vpt.c Mon Nov 05 13:23:55 2007 +0000
>>+++ b/xen/arch/x86/hvm/vpt.c Wed Nov 07 08:55:45 2007 -0500
>>@@ -59,7 +59,7 @@ static void pt_process_missed_ticks(stru
>>     if ( mode_is(pt->vcpu->domain, no_missed_tick_accounting) )
>>     {
>>         pt->pending_intr_nr = 1;
>>-        pt->scheduled = now + pt->scheduled;
>>+        pt->scheduled = now + pt->period;
>>     }
>>     else
>>     {
>>    
>>
>
>
>  
>