Sheng Yang wrote: > On Friday 09 January 2009 00:36:06 Alexander Graf wrote: > >> While booting Linux in VMware ESX, I encountered a strange effect >> in the in-kernel lapic implementation: time went backwards! >> >> While this should never occur, because of that the while loop that >> is done after the invalid calculations caused my host system to hang. >> >> In order to make debugging easier, let's replace this as suggested >> with a modulo function and not run into the danger of looping forever. >> >> To replace the nice hint this bug gave me that the values are broken, >> I added a printk message so people encountering this can at least >> see that something is fishy. >> >> Of course, the real issue needs to be fixed as well! I'm open to ideas >> why now < last_update! >> >> (Thanks to Kevin for his help in debugging this) >> >> Signed-off-by: Alexander Graf <[email protected]> >> Signed-off-by: Kevin Wolf <[email protected]> >> --- >> > > Hi Alexander > > I'm a little suspect here: > > >> if (unlikely(ktime_to_ns(now) <= >> ktime_to_ns(apic->timer.last_update))) { >> /* Wrap around */ >> passed = ktime_add(( { >> (ktime_t) { >> .tv64 = KTIME_MAX - >> (apic->timer.last_update).tv64}; } >> ), now); >> apic_debug("time elapsed\n"); >> } else >> passed = ktime_sub(now, apic->timer.last_update); >> > > And now apic timer base is hr_timer with CLOCK_MONOTONIC, and get_time() is > really ktime_get() which is almost impossible to wrap around. If it's > overflow, at least we need a warning. I think this piece of code due to clock > source change. > > So I doubt: due to some reason, now <= apic->timer.last_update, which cause a > big wrap around operation. > > And the most suspect: > > >> void kvm_apic_timer_intr_post(struct kvm_vcpu *vcpu, int vec) >> { >> struct kvm_lapic *apic = vcpu->arch.apic; >> >> if (apic && apic_lvt_vector(apic, APIC_LVTT) == vec) >> apic->timer.last_update = ktime_add_ns( >> apic->timer.last_update, >> apic->timer.period); >> } >> > > Not sure what's happening, have you tried this? (In fact, I am little willing > to replace all apic->timer.dev.base->get_time() with more explicit > ktime_get() > as in pit.) >
Yes, this code is the culprit. Using that patch of yours now is always > last_update. I'd still like to see that while loop go away ;-). Alex > -- > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index afac68c..414e7e0 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -1115,9 +1115,7 @@ void kvm_apic_timer_intr_post(struct kvm_vcpu *vcpu, > int > vec) > struct kvm_lapic *apic = vcpu->arch.apic; > > if (apic && apic_lvt_vector(apic, APIC_LVTT) == vec) > - apic->timer.last_update = ktime_add_ns( > - apic->timer.last_update, > - apic->timer.period); > + apic->timer.last_update = apic->timer.dev.base->get_time(); > } > > int kvm_get_apic_interrupt(struct kvm_vcpu *vcpu) > > > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
