On 12/09/19 02:34, Wanpeng Li wrote:
>>> - timer_advance_ns -= min((u32)ns,
>>> - timer_advance_ns / LAPIC_TIMER_ADVANCE_ADJUST_STEP);
>>> + timer_advance_ns -= ns;
Looking more closely, this assignment...
>>> } else {
>>> /* too late */
>>> ns = advance_expire_delta * 1000000ULL;
>>> do_div(ns, vcpu->arch.virtual_tsc_khz);
>>> - timer_advance_ns += min((u32)ns,
>>> - timer_advance_ns / LAPIC_TIMER_ADVANCE_ADJUST_STEP);
>>> + timer_advance_ns += ns;
... and this one are dead code now. However...
>>> }
>>>
>>> + timer_advance_ns = (apic->lapic_timer.timer_advance_ns *
>>> + (LAPIC_TIMER_ADVANCE_ADJUST_STEP - 1) + advance_expire_delta) /
>>> + LAPIC_TIMER_ADVANCE_ADJUST_STEP;
... you should instead remove this new assignment and just make the
assignments above just
timer_advance -= ns / LAPIC_TIMER_ADVANCE_ADJUST_STEP;
and
timer_advance -= ns / LAPIC_TIMER_ADVANCE_ADJUST_STEP;
In fact this whole last assignment is buggy, since advance_expire_delta
is in TSC units rather than nanoseconds.
>>> if (abs(advance_expire_delta) < LAPIC_TIMER_ADVANCE_ADJUST_DONE)
>>> apic->lapic_timer.timer_advance_adjust_done = true;
>>> if (unlikely(timer_advance_ns > 5000)) {
>> This looks great. But instead of patch 2, why not remove
>> timer_advance_adjust_done altogether?
> It can fluctuate w/o stop.
Possibly because of the wrong calculation of timer_advance_ns?
Paolo
