From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH v2 01/14] KVM: x86: change PIT discard tick policy Date: Thu, 18 Feb 2016 18:55:05 +0100 Message-ID: <56C60579.5040003@redhat.com> References: <1455736496-374-1-git-send-email-rkrcmar@redhat.com> <1455736496-374-2-git-send-email-rkrcmar@redhat.com> <56C5EDA9.9030204@redhat.com> <20160218165608.GC18904@potion.brq.redhat.com> <56C6004F.2050105@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, Yuki Shibuya , Rik van Riel To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= Return-path: In-Reply-To: <56C6004F.2050105@redhat.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On 18/02/2016 18:33, Paolo Bonzini wrote: > On 18/02/2016 17:56, Radim Kr=C4=8Dm=C3=A1=C5=99 wrote: >> 2016-02-18 17:13+0100, Paolo Bonzini: >>> On 17/02/2016 20:14, Radim Kr=C4=8Dm=C3=A1=C5=99 wrote: >>>> Discard policy uses ack_notifiers to prevent injection of PIT inte= rrupts >>>> before EOI from the last one. >>>> >>>> This patch changes the policy to always try to deliver the interru= pt, >>>> which makes a difference when its vector is in ISR. >>>> Old implementation would drop the interrupt, but proposed one inje= cts to >>>> IRR, like real hardware would. >>> >>> This seems like what libvirt calls the "merge" policy: >> >> Oops, I never looked beyond QEMU after seeing that the naming in lib= virt >> doesn't even match ... >> >> I think the policy that KVM implements (which I call discard) is "de= lay" >> in libvirt. (https://libvirt.org/formatdomain.html#elementsTime) >=20 > Suppose the scheduled ticks are at times 0, 20, 40, 60, 80. The EOI = for > time 0 is only delivered at time 42, other EOIs are timely. >=20 > The resulting injections are: >=20 > - for discard: 0, 60, 80. >=20 > - for catchup, which QEMU calls slew: 0, 42, 51, 60, 80. >=20 > - for merge: 0, 20 (in IRR, delivered at 42), 60, 80. >=20 > For delay I *think* it would be 0, 42, 62, 82, 102. Wrong: for delay it is something like 0, 42, 43, 60, 80. Your patch does the right thing, QEMU is wrong in calling the policy "discard" where it should have been "merge". In fact both i8254 and RT= C use the same wrong nomenclature. And it is indeed superfluous to use ack notifiers to implement the default policy, as the default policy is already baked into the i8259. Sorry, it shows that I'm swamped as I'm messing up things a bit lately. At least I have you to correct me. :) Paolo