| Message ID | 1665579268-7336-1-git-send-email-lirongqing@baidu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] KVM: x86: Don't reset deadline to period when timer is in one shot mode | expand |
+Jim, Peter, and Wanpeng On Wed, Oct 12, 2022, Li RongQing wrote: > In one-shot mode, the APIC timer stops counting when the timer > reaches zero, so don't reset deadline to period for one shot mode > > Signed-off-by: Li RongQing <lirongqing@baidu.com> > --- > arch/x86/kvm/lapic.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 9dda989..bf39027 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -1840,8 +1840,12 @@ static bool set_target_expiration(struct kvm_lapic *apic, u32 count_reg) > if (unlikely(count_reg != APIC_TMICT)) { > deadline = tmict_to_ns(apic, > kvm_lapic_get_reg(apic, count_reg)); > - if (unlikely(deadline <= 0)) > - deadline = apic->lapic_timer.period; > + if (unlikely(deadline <= 0)) { > + if (apic_lvtt_period(apic)) > + deadline = apic->lapic_timer.period; > + else > + deadline = 0; > + } This is not the standard "count has reached zero" path, it's the "vCPU is migrated and the timer needs to be resumed on the destination" path. Zeroing the deadline here will not squash the timer, IIUC it will cause the timer to immediately fire. That said, I think the patch is actually correct even though the shortlog+changelog are wrong. If the timer expired while the vCPU was migrated, KVM _should_ fire the timer ASAP. AFAICT, nothing else in KVM will detect the expired timer, e.g. if the timer expired on the source, apic_get_tmcct() on the source should have returned zero. The only wrinkle I can see is the bug called out in the commit that added this code (the wonderfully (extreme sarcasm) titled commit 24647e0a39b6, "KVM: x86: Return updated timer current count register from KVM_GET_LAPIC") : Note: When a one-shot timer expires, the code in arch/x86/kvm/lapic.c does : not zero the value of the LAPIC initial count register (emulating HW : behavior). If no other timer is run and pending prior to a subsequent : KVM_GET_LAPIC call, the returned register set will include the expired : one-shot initial count. On a subsequent KVM_SET_LAPIC call the code will : see a non-zero initial count and start a new one-shot timer using the : expired timer's count. This is a prior existing bug and will be addressed : in a separate patch. Thanks to jmattson@google.com for this find. I don't see any evidence that that bug was ever fixed. But again, that's an orthogonal bug. There is commit 2735886c9ef1 ("KVM: LAPIC: Keep stored TMCCT register value 0 after KVM_SET_LAPIC"), but I'm struggling to see how that's anything but a glorified nop. > else if (unlikely(deadline > apic->lapic_timer.period)) { > pr_info_ratelimited( > "kvm: vcpu %i: requested lapic timer restore with " > -- > 2.9.4 >
> -----Original Message----- > From: Sean Christopherson <seanjc@google.com> > Sent: Thursday, October 13, 2022 12:06 AM > To: Li,Rongqing <lirongqing@baidu.com> > Cc: kvm@vger.kernel.org; Peter Shier <pshier@google.com>; Jim Mattson > <jmattson@google.com>; Wanpeng Li <wanpengli@tencent.com> > Subject: Re: [PATCH][RFC] KVM: x86: Don't reset deadline to period when timer > is in one shot mode > > +Jim, Peter, and Wanpeng > > On Wed, Oct 12, 2022, Li RongQing wrote: > > In one-shot mode, the APIC timer stops counting when the timer reaches > > zero, so don't reset deadline to period for one shot mode > > > > Signed-off-by: Li RongQing <lirongqing@baidu.com> > > --- > > arch/x86/kvm/lapic.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index > > 9dda989..bf39027 100644 > > --- a/arch/x86/kvm/lapic.c > > +++ b/arch/x86/kvm/lapic.c > > @@ -1840,8 +1840,12 @@ static bool set_target_expiration(struct kvm_lapic > *apic, u32 count_reg) > > if (unlikely(count_reg != APIC_TMICT)) { > > deadline = tmict_to_ns(apic, > > kvm_lapic_get_reg(apic, count_reg)); > > - if (unlikely(deadline <= 0)) > > - deadline = apic->lapic_timer.period; > > + if (unlikely(deadline <= 0)) { > > + if (apic_lvtt_period(apic)) > > + deadline = apic->lapic_timer.period; > > + else > > + deadline = 0; > > + } > > This is not the standard "count has reached zero" path, it's the "vCPU is > migrated and the timer needs to be resumed on the destination" path. > Zeroing the deadline here will not squash the timer, IIUC it will cause the timer > to immediately fire. > > That said, I think the patch is actually correct Should we set deadline to 0 when it is expired whether the timer is one shot mode or period mode ? >even though the shortlog+changelog are wrong. I will rewrite it Thanks -Li
On Mon, Oct 17, 2022, Li,Rongqing wrote: > > +Jim, Peter, and Wanpeng > > > > On Wed, Oct 12, 2022, Li RongQing wrote: > > > In one-shot mode, the APIC timer stops counting when the timer reaches > > > zero, so don't reset deadline to period for one shot mode > > > > > > Signed-off-by: Li RongQing <lirongqing@baidu.com> > > > --- > > > arch/x86/kvm/lapic.c | 8 ++++++-- > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index > > > 9dda989..bf39027 100644 > > > --- a/arch/x86/kvm/lapic.c > > > +++ b/arch/x86/kvm/lapic.c > > > @@ -1840,8 +1840,12 @@ static bool set_target_expiration(struct kvm_lapic > > *apic, u32 count_reg) > > > if (unlikely(count_reg != APIC_TMICT)) { > > > deadline = tmict_to_ns(apic, > > > kvm_lapic_get_reg(apic, count_reg)); > > > - if (unlikely(deadline <= 0)) > > > - deadline = apic->lapic_timer.period; > > > + if (unlikely(deadline <= 0)) { > > > + if (apic_lvtt_period(apic)) > > > + deadline = apic->lapic_timer.period; > > > + else > > > + deadline = 0; > > > + } > > > > This is not the standard "count has reached zero" path, it's the "vCPU is > > migrated and the timer needs to be resumed on the destination" path. > > Zeroing the deadline here will not squash the timer, IIUC it will cause the timer > > to immediately fire. > > > > That said, I think the patch is actually correct > > Should we set deadline to 0 when it is expired whether the timer is one shot > mode or period mode ? I'm not sure I follow the question. Are you asking if this path should set the deadline to '0' even if the timer is in periodic mode?
> -----Original Message----- > From: Sean Christopherson <seanjc@google.com> > Sent: Wednesday, October 19, 2022 11:54 PM > To: Li,Rongqing <lirongqing@baidu.com> > Cc: kvm@vger.kernel.org; Peter Shier <pshier@google.com>; Jim Mattson > <jmattson@google.com>; Wanpeng Li <wanpengli@tencent.com> > Subject: Re: [PATCH][RFC] KVM: x86: Don't reset deadline to period when timer > is in one shot mode > > On Mon, Oct 17, 2022, Li,Rongqing wrote: > > > +Jim, Peter, and Wanpeng > > > > > > On Wed, Oct 12, 2022, Li RongQing wrote: > > > > In one-shot mode, the APIC timer stops counting when the timer > > > > reaches zero, so don't reset deadline to period for one shot mode > > > > > > > > Signed-off-by: Li RongQing <lirongqing@baidu.com> > > > > --- > > > > arch/x86/kvm/lapic.c | 8 ++++++-- > > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index > > > > 9dda989..bf39027 100644 > > > > --- a/arch/x86/kvm/lapic.c > > > > +++ b/arch/x86/kvm/lapic.c > > > > @@ -1840,8 +1840,12 @@ static bool set_target_expiration(struct > > > > kvm_lapic > > > *apic, u32 count_reg) > > > > if (unlikely(count_reg != APIC_TMICT)) { > > > > deadline = tmict_to_ns(apic, > > > > kvm_lapic_get_reg(apic, count_reg)); > > > > - if (unlikely(deadline <= 0)) > > > > - deadline = apic->lapic_timer.period; > > > > + if (unlikely(deadline <= 0)) { > > > > + if (apic_lvtt_period(apic)) > > > > + deadline = apic->lapic_timer.period; > > > > + else > > > > + deadline = 0; > > > > + } > > > > > > This is not the standard "count has reached zero" path, it's the > > > "vCPU is migrated and the timer needs to be resumed on the destination" > path. > > > Zeroing the deadline here will not squash the timer, IIUC it will > > > cause the timer to immediately fire. > > > > > > That said, I think the patch is actually correct > > > > Should we set deadline to 0 when it is expired whether the timer is > > one shot mode or period mode ? > > I'm not sure I follow the question. Are you asking if this path should set the > deadline to '0' even if the timer is in periodic mode? Yes -Li
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index 9dda989..bf39027 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -1840,8 +1840,12 @@ static bool set_target_expiration(struct kvm_lapic *apic, u32 count_reg) if (unlikely(count_reg != APIC_TMICT)) { deadline = tmict_to_ns(apic, kvm_lapic_get_reg(apic, count_reg)); - if (unlikely(deadline <= 0)) - deadline = apic->lapic_timer.period; + if (unlikely(deadline <= 0)) { + if (apic_lvtt_period(apic)) + deadline = apic->lapic_timer.period; + else + deadline = 0; + } else if (unlikely(deadline > apic->lapic_timer.period)) { pr_info_ratelimited( "kvm: vcpu %i: requested lapic timer restore with "
In one-shot mode, the APIC timer stops counting when the timer reaches zero, so don't reset deadline to period for one shot mode Signed-off-by: Li RongQing <lirongqing@baidu.com> --- arch/x86/kvm/lapic.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)