On 24/05/16 13:51, Paolo Bonzini wrote:
On 23/05/2016 16:30, Alan Jenkins wrote:
https://github.com/torvalds/linux/compare/master...sourcejedi:kvm_lockup_fix?expand=1
Here the comparison (and the expired_advance_ns field) is only used to
issue the warning.
No, because expired_advance_ns is used as the limit for the busy-wait.
Could you change the patch to use __delay instead of
ndelay and
No, because the limit is in ns, not host TSC ticks. Also, we were
passing _guest_ TSC ticks to __delay(), which looks like a second
potential cause of lockups because guest TSC ticks are scaled.
omit the warning?
Certainly, and I have a positive suggestion to make the code more
agreeable if it doesn't need WARN_ON(). Thanks for reviewing this. I'll
send a revised version of the patch after testing.
The field expired_advance_ns was an attempt to prevent warnings (and
premature timer interrupts) in case lapic_timer_advance_ns is tuned
downwards, while we're running. This was a third potential case I
noticed, which could have caused a lockup.
The problem was I didn't want to make a user-triggerable WARN_ON. But if
we don't want WARN_ON at all, then maybe it's ok to compare the module
parameter racily, so we don't need the extra field. There's no existing
documentation which I could update, to say that reducing
lapic_timer_advance_ns is not supported, right? (Because even with
checks, it means the guest deadline timer can fire when the TSC is
showing a time prior to the deadline, so it could break guest assumptions).
I wanted to log _a_ warning because I'm writing another patch there (if
you've seen the parent commit) which is supposed to prevent it ever
happening. A better alternative would be to have the workaround first
(and for -stable), without the warning. (I assume you still dislike
WARN_ON with the other patch, but I would try to sell you on at least
printk_once(KERN_ERROR :).
+ if (guest_tsc < tsc_deadline) {
+ unsigned long this_tsc_khz = vcpu->arch.virtual_tsc_khz;
+ u64 delay_ns;
+
+ delay_ns = (tsc_deadline - guest_tsc) * 1000000ULL;
+ do_div(delay_ns, this_tsc_khz);
+
+ if (delay_ns > apic->lapic_timer.expired_advance_ns) {
+ vcpu_err(vcpu,
+ "timer for tsc_deadline fired too far in advance: %lluns\n",
+ delay_ns);
+ WARN_ON(1);
+
+ /* avoid lockup. emulated timer will fire early */
+ delay_ns = apic->lapic_timer.expired_advance_ns;
+ }
+
+ ndelay(delay_ns);
+ }
Thanks,
Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
