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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
