From mboxrd@z Thu Jan  1 00:00:00 1970
From: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
Subject: Re: [PATCH] x86: apic: add APIC Timer periodic/oneshot mode latency
 test
Date: Wed, 26 Oct 2016 16:13:06 +0200
Message-ID: <20161026141305.GA4212@potion>
References: <1476692917-3554-1-git-send-email-wanpeng.li@hotmail.com>
 <20161025115550.GE2247@potion>
 <CANRm+CxiFmCvP8gAwtKJe6qK2m5YamOBEs_neCL7hw83gQVTdg@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Cc: kvm <kvm@vger.kernel.org>, Wanpeng Li <wanpeng.li@hotmail.com>,
        Paolo Bonzini <pbonzini@redhat.com>
To: Wanpeng Li <kernellwp@gmail.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:53572 "EHLO mx1.redhat.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S932193AbcJZONJ (ORCPT <rfc822;kvm@vger.kernel.org>);
        Wed, 26 Oct 2016 10:13:09 -0400
Content-Disposition: inline
In-Reply-To: <CANRm+CxiFmCvP8gAwtKJe6qK2m5YamOBEs_neCL7hw83gQVTdg@mail.gmail.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

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