Re: [PATCH] x86: apic: add APIC Timer periodic/oneshot mode latency test

["Radim Krčmář" <rkrcmar@redhat.com>]

2016-10-26 14:35+0800, Wanpeng Li:
> 2016-10-25 19:55 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>:
>> 2016-10-17 16:28+0800, Wanpeng Li:
> [...]
>>> +static void apic_timer_isr(isr_regs_t *regs)
>>> +{
>>> +    u64 now = rdtsc();
>>> +    ++tdt_count;
>>> +
>>> +    if (table_idx < TABLE_SIZE && tdt_count > 1)
>>> +        table[table_idx++] = now - exptime;
>>> +
>>> +    if (breakmax && tdt_count > 1 && (now - exptime) > breakmax) {
>>> +        hitmax = 1;
>>> +        apic_write(APIC_EOI, 0);
>>> +        return;
>>> +    }
>>> +
>>> +    exptime = now+delta;
>>
>> Two problems on this line:
>>
>> 1) with periodic timer, the delta is added to last expiration time, not
>>    to now().
>>    What you are measuring now is the stability of the period -- very
>>    important as well, but if we used now() only once, you could also
>>    measure the latency.
>>
>> 2) delta is in nanoseconds while exptime is in cycles.  Type error.
> 
> There needs a method to convert the value which is loaded to
> initial-counter register to TSC.

Yes, you need to know the conversion between APIC timer frequency and
TSC frequency.  APIC timer in KVM uses nanoseconds, but TSC is variable.

>>
>>> +
>>> +    if (mode == APIC_LVT_TIMER_ONE_SHOT)
>>> +        /* Set "Initial Counter Register", which starts the timer */
>>> +        apic_write(APIC_TMICT, delta);
>>
>> This is not deadline, so it would be better to read now() for exptime
>> right before the apic_write, so as little time as possible passes in
>> between.
> 
> Do you mean something like this?
> 
> static void apic_timer_isr(isr_regs_t *regs)
> {
>     uint64_t now = rdtsc();
> 
>     if (breakmax && (now - exptime) > breakmax) {

Note that you must have now and exptime in the same unit to have
meaningful operations on.  (especially the "exptime += delta" would bad
without conversion).

>         hitmax = 1;
>         apic_write(APIC_EOI, 0);
>         return;
>     }
> 
>     exptime += delta;


The exptime for oneshot is 'exptime = now + delta', only periodic can do
exptime += delta, so I'd rather read the time again just before writing
the delta

>     if (mode == APIC_LVT_TIMER_ONE_SHOT) {
          time_t now = now();
>         /* Set "Initial Counter Register", which starts the timer */
>         apic_write(APIC_TMICT, delta);

          exptime = now + delta;
      }

>     if (table_idx < TABLE_SIZE)
>         table[table_idx++] = now - exptime;
> 
>     apic_write(APIC_EOI, 0);
> }
> 
> static void test_apic_timer(int mode)
> {
>     handle_irq(APIC_LVT_TIMER_VECTOR, apic_timer_isr);
>     irq_enable();
> 
>          /* Periodic mode */
>     apic_write(APIC_LVTT, mode | APIC_LVT_TIMER_VECTOR);
>     /* Divider == 1 */
>     apic_write(APIC_TDCR, 0x0000000b);
>     exptime = rdtsc() + delta;

Yep, if we convert rdtsc() to the same unit that delta uses.

Thanks.

>     apic_write(APIC_TMICT, delta);
> }
> 
> Regards,
> Wanpeng Li