On Oct 10, 2014, at 12:45 PM, Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
> Il 10/10/2014 03:55, Nadav Amit ha scritto:
>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>>> index b8345dd..51428dd 100644
>>> --- a/arch/x86/kvm/lapic.c
>>> +++ b/arch/x86/kvm/lapic.c
>>> @@ -1096,9 +1096,12 @@ static void start_apic_timer(struct kvm_lapic *apic)
>>> if (likely(tscdeadline > guest_tsc)) {
>>> ns = (tscdeadline - guest_tsc) * 1000000ULL;
>>> do_div(ns, this_tsc_khz);
>>> + hrtimer_start(&apic->lapic_timer.timer,
>>> + ktime_add_ns(now, ns), HRTIMER_MODE_ABS);
>>> + } else {
>>> + atomic_inc(&ktimer->pending);
>>> + kvm_make_request(KVM_REQ_PENDING_TIMER, vcpu);
>>> }
>>> - hrtimer_start(&apic->lapic_timer.timer,
>>> - ktime_add_ns(now, ns), HRTIMER_MODE_ABS);
>>>
>>> local_irq_restore(flags);
>>> }
>>> @@ -1355,9 +1358,6 @@ void kvm_set_lapic_tscdeadline_msr(struct kvm_vcpu *vcpu, u64 data)
>>> return;
>>>
>>> hrtimer_cancel(&apic->lapic_timer.timer);
>>> - /* Inject here so clearing tscdeadline won't override new value */
>>> - if (apic_has_pending_timer(vcpu))
>>> - kvm_inject_apic_timer_irqs(vcpu);
>>> apic->lapic_timer.tscdeadline = data;
>>> start_apic_timer(apic);
>>> }
>>
>> Perhaps I am missing something, but I don’t see how it solves the problem I encountered.
>> Recall the scenario:
>> 1. A TSC deadline timer interrupt is pending.
>> 2. TSC deadline was still not cleared (which happens during vcpu_run).
>> 3. Userspace uses KVM_GET_MSRS/KVM_SET_MSRS to load the same deadline msr.
>>
>> It appears that in such scenario the guest would still get spurious
>> interrupt for no reason, as ktimer->pending may already be increased in
>> apic_timer_fn.
>
> If ktimer->pending == 2 you still get only one interrupt, not two. Am I
> misunderstanding your objection?
You understood me, and I was wrong...
>
>> Second, I think that the solution I proposed would perform better.
>> Currently, there are many unnecessary cancellations and setups of the
>> timer. This solution does not resolve this problem.
>
> I think it does. You do not get an hrtimer_start if tscdeadline <=
> guest_tsc. To avoid useless cancels, either check hrtimer_is_enqueued
> before calling hrtimer_cancel, or go straight to the source and avoid
> taking the lock in the easy cases:
>
> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
> index 1c2fe7de2842..6ce725007424 100644
> --- a/kernel/time/hrtimer.c
> +++ b/kernel/time/hrtimer.c
> @@ -1043,10 +1043,17 @@ int hrtimer_try_to_cancel(struct hrtimer *timer)
> {
> struct hrtimer_clock_base *base;
> unsigned long flags;
> - int ret = -1;
> + unsigned long state = timer->state;
> + int ret;
> +
> + if (state & HRTIMER_STATE_ENQUEUED)
> + return 0;
> + if (state & HRTIMER_STATE_CALLBACK)
> + return -1;
>
> base = lock_hrtimer_base(timer, &flags);
>
> + ret = -1;
> if (!hrtimer_callback_running(timer))
> ret = remove_hrtimer(timer, base);
Wouldn’t this change would cause cancellations never to succeed (the first check would always be true if the timer is active)?
>
>
>> Last, I think that having less interrupts on deadline changes is not
>> completely according to the SDM which says: "If software disarms the
>> timer or postpones the deadline, race conditions may result in the
>> delivery of a spurious timer interrupt.” It never says interrupts may
>> be lost if you reprogram the deadline before you check it expired.
>
> But the case when you rewrite the same value to the MSR is neither
> disarming nor postponing. You would be getting two interrupts for the
> same event. That is why I agree with Radim that checking host_initiated
> is wrong.
I understand, and Radim's solution seems functionally fine, now that I am fully awake and understand it.
I still think that if tscdeadline > guest_tsc, then reprogramming the deadline with the same value, as QEMU does, would result in unwarranted overhead.
Perhaps it would be enough not to reprogram the timer if tscdeadline value does not change (by either guest or host).
Thanks,
Nadav
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
