Re: add back pending timer irqs for kernel APIC timer

Dong, Eddie
2007-08-13T13:19:04Z

Avi Kivity wrote:
> What does "pt" mean in this context? "pending timer"?
>
> Suggested descriptive names below.

Periodic timer. Comments added.

>> if (apic_lvtt_period(apic)) {
>> - u32 offset;
>> - u32 tmict = apic_get_reg(apic, APIC_TMICT);
>> -
>> - offset = APIC_BUS_CYCLE_NS * apic->timer.divide_count *
tmict;
>> -
>> result = 1;
>> apic->timer.dev.expires = ktime_add_ns(
>> apic->timer.dev.expires,
>> apic->timer.period);
>> }
>> -
>> return result;
>> }
>>
>
> While this improves throughput, doesn't it decrease responsiveness?
> Suppose the guest is sleeping and there is no activity except
> for lapic
> interrupts. Won't the wakeup get deferred indefinitely?

didn't catch here. do u mean the guest is in HLT ? There is hole here
for HLT case.
Will fix.

>
>>
>> +void kvm_pt_update_irq(struct kvm_vcpu *vcpu)
>> +{
>> + struct kvm_lapic *apic = vcpu->apic;
>> +
>> + if (apic && atomic_read(&apic->timer.pending) > 0) {
>>
>
> Extra braces. Also this smells like a race. What if timer.pending
> is updated just after this?

kvm_pt_update_irq is within intr_assist where host IRQ is disabled.
It can be updated only after we return to guest.

>
>> + if (inject_apic_timer_irq(apic))
>> + atomic_dec(&apic->timer.pending);
>> + }
>> +}
>> +EXPORT_SYMBOL_GPL(kvm_pt_update_irq);
>>
>
> How about "kvm_inject_pending_timer_irqs"

Sure. But we will have PIT too, so maybe kvm_inject_apic_timer_irqs?

>
>> +
>> +void kvm_pt_intr_post(struct kvm_vcpu *vcpu, int vec) +{
>> + struct kvm_lapic *apic = vcpu->apic;
>> +
>> + if (apic && apic_lvt_vector(apic, APIC_LVTT) == vec)
>> + apic->timer.last_update = ktime_add_ns(
>> + apic->timer.last_update,
>> + apic->timer.period);
>> +}
>> +EXPORT_SYMBOL_GPL(kvm_pt_intr_post);
>>
>
> kvm_update_pending_timer_irq?

only periodic timer has this issue, one shot time is ok.
update is used as kvm_pt_update_irq, here is the post action.

BTW, can u rebase the branch? I saw some error in today's branch.

Will send patch after you rebased.

thx,eddie