From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH 5/5] KVM: x86: Using TSC deadline may cause multiple interrupts by user writes Date: Fri, 10 Oct 2014 15:55:07 +0200 Message-ID: <5437E53B.40000@redhat.com> References: <1412287806-16016-1-git-send-email-namit@cs.technion.ac.il> <1412287806-16016-6-git-send-email-namit@cs.technion.ac.il> <20141006205737.GC2722@potion.brq.redhat.com> <4E3FA8A7-6CEF-4077-AD91-9AAE1AF86FEF@gmail.com> <20141008100619.GA20422@potion.brq.redhat.com> <99CA15AA-DDC1-4E95-A168-7AA640C5A3B9@gmail.com> <5437AAB0.4060908@redhat.com> <42A4DE6D-496C-4D2B-960D-5CABE1C6934F@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , Nadav Amit , joro@8bytes.org, kvm@vger.kernel.org To: Nadav Amit Return-path: Received: from mail-wg0-f47.google.com ([74.125.82.47]:51906 "EHLO mail-wg0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751917AbaJJNzU (ORCPT ); Fri, 10 Oct 2014 09:55:20 -0400 Received: by mail-wg0-f47.google.com with SMTP id x13so3927362wgg.18 for ; Fri, 10 Oct 2014 06:55:18 -0700 (PDT) In-Reply-To: <42A4DE6D-496C-4D2B-960D-5CABE1C6934F@gmail.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 t= he >>> timer. This solution does not resolve this problem. >> >> I think it does. You do not get an hrtimer_start if tscdeadline <=3D >> guest_tsc. To avoid useless cancels, either check hrtimer_is_enqueu= ed >> before calling hrtimer_cancel, or go straight to the source and avoi= d >> 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 *ti= mer) >> { >> struct hrtimer_clock_base *base; >> unsigned long flags; >> - int ret =3D -1; >> + unsigned long state =3D timer->state; >> + int ret; >> + >> + if (state & HRTIMER_STATE_ENQUEUED) >> + return 0; >> + if (state & HRTIMER_STATE_CALLBACK) >> + return -1; >> >> base =3D lock_hrtimer_base(timer, &flags); >> >> + ret =3D -1; >> if (!hrtimer_callback_running(timer)) >> ret =3D remove_hrtimer(timer, base); > Wouldn=E2=80=99t this change would cause cancellations never to succe= ed (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 no= t >>> completely according to the SDM which says: "If software disarms th= e >>> timer or postpones the deadline, race conditions may result in the >>> delivery of a spurious timer interrupt.=E2=80=9D It never says inte= rrupts 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 t= he >> same event. That is why I agree with Radim that checking host_initi= ated >> is wrong. >=20 > 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 unwarrante= d 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 passe= d 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