2017-10-06 22:21 GMT+08:00 Paolo Bonzini <pbonzini@xxxxxxxxxx>: > On 06/10/2017 16:01, Wanpeng Li wrote: >> + if (!apic->lapic_timer.period) >> + return; >> + >> + now = ktime_get(); >> + remaining = ktime_sub(apic->lapic_timer.target_expiration, now); >> + if (ktime_to_ns(remaining) < 0) >> + remaining = 0; >> + delta = mod_64(ktime_to_ns(remaining), apic->lapic_timer.period); > > I think this shouldn't be happening. If it does, I'm not sure the mod > is the right thing to do, so I'd just use ktime_to_ns(remaining). > > So perhaps let's simplify all this to: > > ns_remaining_old = ktime_to_ns(remaining); > ns_remaining_new = mul_u64_u32_div(ns_remaining_old, > apic->divide_count, old_divisor); > > because below you're calling nsec_to_cycles but remaining is not expressed > in nanoseconds. > >> + if (!delta) >> + return; >> + >> + delta = delta * apic->divide_count / old_divisor; >> + >> + limit_periodic_timer_frequency(apic); > > This should be done before all the "if"s (which should not be there in v8, > but you should still call it before "now = ktime_get();"). Just sent out v8 to handle all the comments. Regards, Wanpeng Li > > Paolo > >> + apic->lapic_timer.tscdeadline += nsec_to_cycles(apic->vcpu, delta) - >> + nsec_to_cycles(apic->vcpu, remaining); >> + apic->lapic_timer.target_expiration = ktime_add_ns(now, delta); >> +} >
