Re: [PATCH 5/5] KVM: x86: Using TSC deadline may cause multiple interrupts by user writes

[Paolo Bonzini <pbonzini@redhat.com>]

Il 10/10/2014 14:51, Nadav Amit ha scritto:
>>> 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)?

Ehm, there is a missing ! in that first "if".

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

The overhead is about two atomic operations (70 clock cycles?).  Still,
there are plenty of other micro-optimizations possible:

1) instead of incrementing timer->pending, set it to 1

2) change it to test_and_set_bit and only set PENDING_TIMER if the
result was zero

3) non-atomically test PENDING_TIMER before (atomically) clearing it

4) return bool from kvm_inject_apic_timer_irqs and only clear
PENDING_TIMER if a timer interrupt was actually injected.

(1) or (2) would remove one atomic operation when reprogramming a passed
deadline with the same value.  (3) or (4) would remove one atomic
operation in the case where the cause of the exit is not an expired
timer.  Any takers?

> Perhaps it would be enough not to reprogram the timer if tscdeadline value does not change (by either guest or host).

Yes, and that would just be your patch without host_initiated.

Paolo